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.

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

Refactor initialization logic to allow for enabling Memory Randomization #1587

Merged
merged 15 commits into from
Feb 4, 2021

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Nov 10, 2020

Edit: see #1587 (comment)

I am currently working on an experiment with Memory Randomization in BDN (dotnet/BenchmarkDotNet#1587)

The goal is to find benchmarks that are heavily dependent on memory alignment and make it hard for us to reliably detect regressions, discuss the solution, and then apply it.

Currently, I am opening this PR just so others can see the code changes that were needed to make it work and perhaps give it a try.

Next, I am going to create an issue with my findings, we are going to discuss it and see what we do next.

…tion as it's usually the first thing called from GlobalSetup method

which with MemoryRandomization enabled is the first method called right after allocation of random-sized memory by BDN engine
… that allocate less

as Global Setup methods might get called right after random-size memory allocation
…ors (to allow for re-allocation with different alignment)
…oryRandmization is enabled

this is due to having new _arrayBufferWriter every time and allocating a lot of memory
so we don't always allocate a new instance
@adamsitnik adamsitnik mentioned this pull request Nov 10, 2020
@danmoseley
Copy link
Member

This will be interesting to @kunalspathak

@adamsitnik adamsitnik mentioned this pull request Nov 18, 2020
6 tasks
# Conflicts:
#	src/harness/BenchmarkDotNet.Extensions/BenchmarkDotNet.Extensions.csproj
This reverts commit d02c372.
@adamsitnik adamsitnik changed the title [Experiment][No Merge] Memory Randomization Refactor initialization logic to allow for enabling Memory Randomization Jan 27, 2021
@adamsitnik
Copy link
Member Author

I've changed this PR to not enable the Memory Randomization, but just refactor the initialization logic to allow for enabling Memory Randomization in the near future. So this PR should have no effect on the reported time as of now.

Memory Randomization will change the behavior of [GlobalSetup] methods, which will be invoked not just at the beginning of the benchmarking, but once before every benchmark iteration. The actual randomization is going to happen before it, so everything allocated in the setup will have random alignment and this will allow us to get truly random memory alignment for each iteration (so far everything was allocated just once and whatever alignment we got, it remained unchanged for all iterations).

@DrewScoggins @kunalspathak please take a look.

@adamsitnik adamsitnik marked this pull request as ready for review January 27, 2021 12:21
@adamsitnik
Copy link
Member Author

The CI failures are not related to microbenchmarks, so the PR should be ready to merge

@kunalspathak
Copy link
Member

Thanks @adamsitnik for putting this together. Are we also doing anything to avoid allocating inside the benchmark method itself to reduce the effect of GC? Several array benchmarks allocate an array inside the benchmark method.

@kunalspathak
Copy link
Member

@AndyAyersMS - FYI.

@DrewScoggins
Copy link
Member

I am working through this PR file by file. Should have comments later today.

@kunalspathak
Copy link
Member

I skimmed through the PR and looks ok. I will let @DrewScoggins do the sign off.

@adamsitnik
Copy link
Member Author

Are we also doing anything to avoid allocating inside the benchmark method itself to reduce the effect of GC? Several array benchmarks allocate an array inside the benchmark method.

We recommend not to allocate any arrays in the new benchmarks (https://github.com/dotnet/performance/blob/master/docs/microbenchmark-design-guidelines.md#setup). When it comes to the old ones they were left mostly unchanged when I was porting them to BenchmarkDotNet. One of the requirements was to use the existing Reporting System (called BenchView back then). This has enforced the rule of not changing the IDs (type, method, and argument names), and not modifying the benchmarks (unless they had bugs) to not introduce any changes in the reported time (to keep the trends as they were).

If these allocations are a problem, we should move them to setup methods. @kunalspathak would you like to send a PR based on your recent research?

@kunalspathak
Copy link
Member

If these allocations are a problem, we should move them to setup methods. @kunalspathak would you like to send a PR based on your recent research?

Absolutely.

@adamsitnik
Copy link
Member Author

@DrewScoggins ping

@kunalspathak
Copy link
Member

I just realized that there are certain benchmarks which allocates data to static variables.

private static readonly int[] _arrayOf100Integers = Enumerable.Range(0, Size).ToArray();
internal static readonly LinqTestData Array = new LinqTestData(_arrayOf100Integers);
internal static readonly LinqTestData List = new LinqTestData(new List<int>(_arrayOf100Integers));
internal static readonly LinqTestData Range = new LinqTestData(Enumerable.Range(0, Size));
internal static readonly LinqTestData IEnumerable = new LinqTestData(new IEnumerableWrapper<int>(_arrayOf100Integers));
internal static readonly LinqTestData IList = new LinqTestData(new IListWrapper<int>(_arrayOf100Integers));
internal static readonly LinqTestData ICollection = new LinqTestData(new ICollectionWrapper<int>(_arrayOf100Integers));
internal static readonly LinqTestData IOrderedEnumerable = new LinqTestData(_arrayOf100Integers.OrderBy(i => i)); // OrderBy() returns IOrderedEnumerable (OrderedEnumerable is internal)

Should this also need fixup?

Copy link
Member

@DrewScoggins DrewScoggins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@adamsitnik
Copy link
Member Author

@kunalspathak @DrewScoggins thank you for your reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants