Skip to content

Computing the allocation statistics#1582

Closed
cshung wants to merge 3 commits intodotnet:mainfrom
cshung:public/dev/andrewau/statistics
Closed

Computing the allocation statistics#1582
cshung wants to merge 3 commits intodotnet:mainfrom
cshung:public/dev/andrewau/statistics

Conversation

@cshung
Copy link
Contributor

@cshung cshung commented Nov 5, 2020

In order to debug what is wrong with the POH allocation test case, I added this support in GCPerfSim to count how many bytes we allocated into the individual object heaps.

To start with the review, read the changes in GCPerfSim.cs.

The change accounts for the bytes allocated in individual object heaps in two ways. First, it is done in GetObjectSpec() and then it is done again in the Statistics class which is enabled only for debug, which allows me to be sure that the count is correct when I debug.

The change I did found a bug. In order for this accounting scheme to work, we need to make sure whenever GetObjectSpec() is called, there is exactly one allocation according to the spec happen downstream in JustMakeObject(). Just so it is clear, we need this constraint for another reason, if that is not true, then the allocation ratio arguments won't work.

The various thing that the original code did for ReferenceItemWithSize does not comply with this constraint, and therefore I changed the logic there, it will still create a linked list, but it will allocate exactly once.

The python code is driven by the fact that the GCPerfSim output is actually parsed by the infrastructure, so we need to make sure the infrastructure is aware of these extra outputs, and then py . lint do all the other changes.

@ivdiazsa
Copy link
Contributor

ivdiazsa commented Nov 9, 2020

Looks good! Just the typo I pointed and drop the linter's changes to bench_file.md. For some reason it messes with certain things like removing the example test_executables.

@cshung cshung force-pushed the public/dev/andrewau/statistics branch 2 times, most recently from d08e039 to 821e4ec Compare November 10, 2020 19:04
@cshung cshung marked this pull request as draft November 10, 2020 19:17
@cshung cshung force-pushed the public/dev/andrewau/statistics branch 2 times, most recently from ea1bdc9 to 49bbe26 Compare November 17, 2020 23:40
@cshung cshung force-pushed the public/dev/andrewau/statistics branch from 49bbe26 to c0fe7b1 Compare November 25, 2020 19:19
@cshung cshung force-pushed the public/dev/andrewau/statistics branch from c0fe7b1 to aae0ca3 Compare December 10, 2020 19:13
@cshung cshung force-pushed the public/dev/andrewau/statistics branch from aae0ca3 to 10b7ae9 Compare January 6, 2021 19:27
@Maoni0
Copy link
Member

Maoni0 commented Jan 15, 2021

looks fine to me. a note for future reference, if you could refrain from having a significant chunk of your changes be formatting changes only that'd be much appreciated. I'm all for have formatting changes to make the formatting more consistent but having them mixed with actual functional changes makes CRs more work than it should be. I'm not gonna ask you to separate them out for this PR but please keep it in mind for future PRs. I'm guessing these changes simply come from whatever editor you are using that does the formatting. you can just do that once and submit a PR to cover all formatting changes.

I see this is marked as Draft, please let me know when you think this is ready for checkin.

@cshung cshung force-pushed the public/dev/andrewau/statistics branch 2 times, most recently from f8f5568 to c2170e0 Compare February 3, 2021 20:05
@cshung
Copy link
Contributor Author

cshung commented Feb 3, 2021

Once the formatting change PR is merged, changes to GCPerfSim.cs should be purely functional changes.

@cshung cshung force-pushed the public/dev/andrewau/statistics branch 2 times, most recently from db3eb48 to 7d6ea44 Compare February 4, 2021 03:00
@cshung cshung force-pushed the public/dev/andrewau/statistics branch 3 times, most recently from 7bc2dd2 to ca3f457 Compare February 27, 2021 02:03
@cshung cshung marked this pull request as ready for review February 27, 2021 02:04
@cshung cshung force-pushed the public/dev/andrewau/statistics branch 3 times, most recently from 96e15c3 to 03890f4 Compare March 6, 2021 00:07
@cshung cshung force-pushed the public/dev/andrewau/statistics branch from 03890f4 to 6132643 Compare March 6, 2021 01:46
@cshung cshung force-pushed the public/dev/andrewau/statistics branch from c2bb137 to 965d036 Compare March 15, 2021 22:23
Base automatically changed from master to main March 18, 2021 17:12
@cshung
Copy link
Contributor Author

cshung commented Apr 15, 2021

This PR is subsumed by #1704

@cshung cshung closed this Apr 15, 2021
@cshung cshung deleted the public/dev/andrewau/statistics branch April 15, 2021 21:02
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.

3 participants