-
Notifications
You must be signed in to change notification settings - Fork 10
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
Support VO bit #158
Merged
Merged
Support VO bit #158
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
ed6d602
Supporting VO bit; clearing pin bit in post alloc
udesou 51fa212
Updating julia_version and julia_repo
udesou 1d6cb11
Update julia_version
udesou ecb5878
Inlining post alloc fastpath in C
udesou b06841b
Update julia_version
udesou c88adc1
Updating julia version and mmtk core version; bset VO bit for immorta…
udesou e2d05c5
Adding MMTK_CONSERVATIVE flag; Updating julia_version
udesou 2b9cead
Updating julia_version
udesou b483f04
Applying cargo fmt
udesou a8aada9
Add functions to bulk set VO bit in the binding instead of having to …
udesou 2513171
Using VO_BIT specific constants explicitly
udesou 7ab0311
Update julia_version
udesou 6be20d3
Update julia_repo and julia_version
udesou File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
WITH_MMTK=1 | ||
MMTK_CONSERVATIVE=1 | ||
FORCE_ASSERTIONS=1 | ||
LLVM_ASSERTIONS=1 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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.
Did you include this code when you ran benchmarks in #158 (comment)?
This should be fixed in mmtk-core by bulk clearing the pinning bits. We don't want to clear the bits here for every object.
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, I did include it. Is it fixed in the mmtk-core version we're currently using?
I'll remove that code then and maybe try to re-run the benchmarks in the meantime.
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 have fixed it in mmtk-core. But we should fix it in mmtk-core.
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 also only clear the bits in this function, not in the JIT'd code. So for objects allocated in the JIT'd code, the pinning bits are not cleared properly.
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 think I'm clearing them in the slowpath, not sure if they need to be cleared in the fastpath as well (since they will be bulk cleared for the memory chunk I get in the slowpath).
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.
There is no slowpath or fastpath for
post_alloc
. We either inline them, or not inline them. Both versions should be the same.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.
What I meant was that in the allocation slowpath, you could clear the pin bits by calling
mmtk_post_alloc
, and it would be cleared for the chunk of memory you get between the new cursor and limit. But I checked the Julia PR and I wasn't callingmmtk_post_alloc
there either, since I'm now inlining the code to set the VO bit. So, in fact, that code to bulk zero the pinning bits was not being executed in the results above.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 see you removed the code for the pin bits. Just make sure that we are on the same page. With this PR, the pin bits are not cleared, right? It sounds fine to me. But it is an issue that we will need to fix.
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.
That's right. It should be fixed inside mmtk-core. I'm currently looking into it and will open a PR there instead.