Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix root scanning #165
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
Fix root scanning #165
Changes from all commits
fa1a26e
084a127
f6e121d
55f1317
07f180d
d359b0b
7ce923f
334e0ac
8aadcbc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this as a separate global variable. Whenever you change this
SIZE
, you have to acquire a lock to change theCODE_CACHE_ROOTS
. And in the only place where you read theSIZE
, you also acquire a lock to read theCODE_CACHE_ROOTS
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also in
mmtk_register_nmethod()
, you modify theSIZE
before acquiring a lock to theROOTS
, which will make theSIZE
and theROOTS
inconsistent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
SIZE
is not necessary, it's just an optimization. BecauseCODE_CACHE_ROOTS
is a set ofVec<Address>
s, at root scanning time these vectors have to be merged together as a single buffer. Record totalSIZE
ahead of time and use it later for the merged buffer allocation may prevent frequent vector size expansion.The consistency is ensured by separating the writes and reads.
SIZE
is only updated during mutator phases, and reads only happen in pauses.Apart from tracking
SIZE
, I also tried putting each vector in the cached roots (HashMap<Address, Vec<Address>>
) to a different work packet. There's a slight performance drop by doing so, since this may yield too many vector clones, too many work packets, and most of the vectors are small.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just calculate the total size of the
Vec<Address>
before creating the vec.My point is that if you have acquired the lock to
CODE_CACHE_ROOTS
whenever you accessCODE_CACHE_ROOTS_SIZE
, then there is little point having a separate count.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid calculating the size in a loop may slow down the GC time. If I can calculate the total size cheaply ahead of time, simply using the pre-calculated size here is the fastest way.
This makes me realize that if I move the
SIZE
increments after the lock, I can do relaxed instead of atomic increments. This solves the consistency issue, has higher the increments performance, while still having a precise pre-calculated size counter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you always access the
CODE_CACHE_ROOTS_SIZE
while holding the lock ofCODE_CACHE_ROOTS
, you can merge them into oneMutex<T>
, like this:When using, you lock the mutex and have access to both
hash_map
andsize
: