8259776: Remove ParallelGC non-CAS oldgen allocation#2101
8259776: Remove ParallelGC non-CAS oldgen allocation#2101kimbarrett wants to merge 4 commits intoopenjdk:masterfrom
Conversation
|
👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into |
|
@kimbarrett The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| // Allocation. We report all successful allocations to the size policy | ||
| // Note that the perm gen does not use this method, and should not! | ||
| HeapWord* allocate(size_t word_size); | ||
| HeapWord* allocate(size_t word_size) { |
There was a problem hiding this comment.
Before this change there has been a small semantical difference between allocate() and par_allocate(). The former also updated the size policy, which seem to be missing now in ParallelScavengeHeap::mem_allocate_old_gen() and ParallelScavengeHeap::failed_mem_allocate().
There was a problem hiding this comment.
Oops, I forgot about that and got overly enthusiastic about code deletion. Thanks for spotting it.
| if (!should_alloc_in_eden(size) || GCLocker::is_active_and_needs_gc()) { | ||
| // Size is too big for eden, or gc is locked out. | ||
| return old_gen()->allocate(size); | ||
| return old_gen()->allocate_and_record(size); |
There was a problem hiding this comment.
I would have kind of preferred if allocate_and_record were a helper method here in ParallelScavengeHeap since the recording seems to be entirely a thing of the PSH and not of the old gen, and the implementation of that method just calls back in here, but I am good with this too.
|
@kimbarrett This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
|
Mailing list message from Kim Barrett on hotspot-gc-dev:
I like this suggestion. |
|
/integrate |
openjdk
bot
added
integrated
|
@kimbarrett Pushed as commit 06348df. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
3 participants
Please review this change to ParallelGC oldgen allocation. There were two
variants, one using CAS on the _top member of the mutable space, the other
requiring locking or other forms of mutual exclusion.
We don't need both variants. The non-CAS variant is only used in a few
places, where the cost of an extra CAS doesn't matter. What does matter is
that having two variants, which must not be used concurrently, makes the
code larger, more complex, and harder to verify. (This change came out of
analyzing JDK-8259271. No problems were found (or expected), so this change
is not expected to impact that bug. But because of the two variants, the
possibility of unexpected interact needed to be examined.)
The non-CAS allocation support has been removed, with PSOldGen::allocate now
implemented using the CAS-based allocation. The cas_ prefix naming
convention is retained for the internals for clarity.
While looking at this, noticed and removed a couple of lingering references
to the class AdjoiningGenerations, which no longer exists after JDK-8243146.
Testing:
mach5 tier1-5
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2101/head:pull/2101$ git checkout pull/2101