Don't store references in Consumer

[timcassell]

Fixes #1942

Also fixes boxing nullable value types (default(T) == null also returns true for nullable value types, causing the box to occur).

[adamsitnik]

This code is so old that some parts of it "predates me" (word I learned at MS). If I remember correctly, we were doing the volatile writes for some good reasons, I just don't remember why exactly (to prevent from Out-of-order execution?).

@AndreyAkinshin could you please take a look?

[timcassell]

From its usage, it looks like this is just for preventing dead code elimination, and a convenient method for consuming iterables (like the IntroDeferredExecution sample).

[AndreyAkinshin]

As Adam well noted, we use volatile writes to prevent out-of-order execution. The suggested change can have unpredictable effects on the measurements in nanobenchmarking. Another severe issue is inlining: Consumer methods are aggressively inlined so that the instruction pointer stays inside the scope of the current method. DeadCodeEliminationHelper has NoInlining policy, which forces the instruction pointer to jump to a random place. It also can be harmful for nanobenchmarks.

As far as I know, we use DeadCodeEliminationHelper only in the jitting phase (we want to make sure that all methods are fully jitted, while we don't care about the accuracy of the measurements). It should not be used in actual measurements.

[timcassell]

@AndreyAkinshin That consume method is repeated in the overhead measurement, so I don't see how it will negatively affect nano benchmarks. And if a no inlined method is called, it also doesn't rearrange order of execution, right?

The other option would be to immediately overwrite the field with null, which I thought was less desirable than calling DeadCodeEliminationHelper.

And the third option is to null the field after the loop, but that could still affect the GC during the loop.

Another thing I thought about was to call GC.KeepAlive, but I wasn't too sure about it for this purpose.

How do you suggest we fix the issue?

[AndreyAkinshin]

The other option would be to immediately overwrite the field with null

I like this option. @adamsitnik what do you think?

[timcassell]

@adamsitnik If you agree, I have updated the branch with that change, so you can re-open this PR if you wish (or I can open a new PR).

[timcassell]

#2191