Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consider changing Consume to not hold onto references for very long #1942

Closed
AndyAyersMS opened this issue Mar 11, 2022 · 1 comment · Fixed by #2191
Closed

Consider changing Consume to not hold onto references for very long #1942

AndyAyersMS opened this issue Mar 11, 2022 · 1 comment · Fixed by #2191
Assignees
@timcassell
Milestone
v0.13.5

Comments

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Mar 11, 2022 •
edited
Loading

Currently some of the Consume methods stash the reference passed to them in a local field, keeping the object alive as long as the consume object itself remains live.

BenchmarkDotNet/src/BenchmarkDotNet/Engines/Consumer.cs

Lines 98 to 109 in b78ec96

[MethodImpl(MethodImplOptions.AggressiveInlining)]
[PublicAPI]
public void Consume(string stringValue) => Volatile.Write(ref stringHolder, stringValue);
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[PublicAPI]
public void Consume(object objectValue) => Volatile.Write(ref objectHolder, objectValue);
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[PublicAPI]
public void Consume<T>(T objectValue) where T : class // class constraint prevents from boxing structs
=> Volatile.Write(ref objectHolder, objectValue);

If that object roots a large object graph or large array allocation it can alter GC behavior and impact benchmark timing.

Consider either immediately over-writing the local field with null or nulling out this field at the end of measurement intervals.
@timcassell
Copy link
Collaborator

I actually ran into this while attempting to measure survived memory in #1596. I ended up adding a Clear method that I ran at the end of the benchmark. Unfortunately that PR never got merged (I guess cause .Net Core still hasn't fixed the measurement error).

@timcassell timcassell mentioned this issue Jul 9, 2022
Open
@timcassell timcassell mentioned this issue Oct 27, 2022
Closed
@timcassell timcassell mentioned this issue Nov 8, 2022
Merged
@AndreyAkinshin AndreyAkinshin added this to the v0.13.5 milestone Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@timcassell timcassell

Labels
None yet
Projects
None yet
Milestone
v0.13.5
3 participants
@AndreyAkinshin @AndyAyersMS @timcassell