Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Conversation

JosephTremoulet
Copy link

Declare static field finalizerCompletedOnce volatile -- this test has a
side-effect-free busy-loop which checks that static variable for a change it
expects a finalizer to make to it; this static field must be volatile to
ensure the jit doesn't hoist the load from the loop.

@JosephTremoulet
Copy link
Author

@dotnet/jit-contrib PTAL. This test fix was checked in as part of #6889, but that was subsequently backed-out (in #7118) because the source changes there exposed issue #7147 (in dotnet/corefx#11524). It looks like this same test issue is causing failures for #7159, so should be checked back in (leaving the source changes of #6889 still backed-out since #7147 is still open).

@ericeil
Copy link

ericeil commented Sep 13, 2016

You may also want to make use of the SpinWait type here: https://github.com/dotnet/coreclr/pull/7162/files#diff-e96b761dfb051cbacb4e6f83577c8825R20. That loop may otherwise starve the finalizer thread, particularly if the machine is busy with other work as well.

Otherwise, LGTM.

@JosephTremoulet
Copy link
Author

@ericeil thanks for the suggestion; updated to use SpinWait.

@pgavlin
Copy link

pgavlin commented Sep 13, 2016

LGTM.

@JosephTremoulet
Copy link
Author

The test failed with a different error -- the test got to "Main end" but not "Finalizer end". I couldn't consistently repro this locally, but I did find that in optimized builds we'd entirely remove the allocation in the loop in the finalizer. So I've updated this change again, to include a call to GC.KeepAlive to avoid removing the allocation in that loop.

@JosephTremoulet
Copy link
Author

@ericeil -- this test was timing out in the CI runs with the SpinWait added. I verified locally that the test wasn't getting past the first loop in a reasonable amount of time because nothing was getting finalized. I think this was just due to the GC pressure being lower due to the time added between allocations by the spin waits. So, I've updated the change again, to include an explicit call to GC.Collect, to encourage finalizer work to get queued quicker, and in my local runs this makes the test succeed quickly. PTAL.

@ericeil
Copy link

ericeil commented Sep 14, 2016

this test was timing out in the CI runs with the SpinWait added.

I may have lead you astray... It didn't occur to me yesterday that the allocation in that loop would force a GC, which would necessarily "yield" to the finalizer thread (so it can reach a GC safe point, if nothing else). It makes sense that the SpinWait would slow this down too much also. I think I'd just go back to the original simple loop.

@JosephTremoulet
Copy link
Author

he allocation in that loop would force a GC, which would necessarily "yield"

Ah, ok. Updated again, with the SpinWait stuff removed but the GC.KeepAlive calls left in.

@JosephTremoulet
Copy link
Author

@dotnet-bot test Linux ARM Emulator Cross Debug Build

Declare static field `finalizerCompletedOnce` volatile -- this test has a
side-effect-free busy-loop which checks that static variable for a change it
expects a finalizer to make to it; this static field must be volatile to
ensure the jit doesn't hoist the load from the loop.

Call GC.KeepAlive on the objects constructed in the various allocation
loops in this test, to make sure that the entire allocation isn't
optimized away.
@JosephTremoulet JosephTremoulet merged commit 25e7be4 into dotnet:master Sep 15, 2016
@JosephTremoulet JosephTremoulet deleted the VolatileTest branch September 15, 2016 17:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants