-
Notifications
You must be signed in to change notification settings - Fork 73
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
Make GC triggering and heap resizing consistent #1266
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
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
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
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.
The following is not an issue from this PR, but I think it is worth bringing it up and discussing it.
Rounding up to the nearest page is okay. The real issue is that we may double count metadata memory.
Consider the case that we have 8 data pages, and the ratio between data and side metedata is 8:1. We actually have 1 side metadata page. If we call this function with 8 pages, we get 1 page -- good. But if we call this function 4 times with 2 pages each, we get 4 side metadata pages.
To estimate the side metadata pages for spaces, we go through each space, and calculate the side metadata usage for each spec in the space. The more spaces and the specs we have, the larger error we have for the calculated side metadata pages. Also if we divide the space into smaller ranges and call this function for each range, we get larger errors. This PR uses this function for pending pages, which is a relatively small memory range (compared to spaces). Luckily we do not add up the estimated pending pages, so probably it is fine.
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.
This is actually expected. Spaces do not overlap (Their ranges don't overlap in Map64. In Map32 with discontiguous spaces, spaces do not overlap at chunk granularity). Therefore side metadata for different spaces will not overlap. So if there are four different spaces, each haing 2 pages of data memory, and the data-meta ratio is 8:1, it will still need 4 pages of metadata, one for each space, despite that each space only needs 0.25 pages of metadata.
Yes. Pending pages are usually small, and it is almost certainly that the result will be an over-estimation. But in reality, if the allocated pending pages happen to be in a region where no side metadata has been mapped, yet, it will allocate one page in each kind of side metadata. So this kind of over-estimation covers the worst-case scenario.
But it is still arguable whether over-estimating at page granularity is the best choice. The operating system does paging at page granularity, which can be seen in
/proc/pid/pagemap
, as we discussed before. But MMTk does mmap at chunk granularity. Since each chunk is 4MB, that'll over-estimate the memory use to much. Page granularity happens to just work, as long as GC triggering and heap resizing are consistent with the estimation.