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
8267703: runtime/cds/appcds/cacheObject/HeapFragmentationTest.java crashed with OutOfMemory #4225
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -94,10 +94,14 @@ uint G1FullCollector::calc_active_workers() { | ||
uint current_active_workers = heap->workers()->active_workers(); | ||
uint active_worker_limit = WorkerPolicy::calc_active_workers(max_worker_count, current_active_workers, 0); | ||
|
||
// Finally consider the amount of used regions. | ||
uint used_worker_limit = MAX2(heap->num_used_regions(), 1u); | ||
|
||
// Update active workers to the lower of the limits. | ||
uint worker_count = MIN2(heap_waste_worker_limit, active_worker_limit); | ||
log_debug(gc, task)("Requesting %u active workers for full compaction (waste limited workers: %u, adaptive workers: %u)", | ||
worker_count, heap_waste_worker_limit, active_worker_limit); | ||
uint worker_count = MIN3(heap_waste_worker_limit, active_worker_limit, used_worker_limit); | ||
log_debug(gc, task)("Requesting %u active workers for full compaction (waste limited workers: %u, " | ||
"adaptive workers: %u, used limited workers: %u)", | ||
worker_count, heap_waste_worker_limit, active_worker_limit, used_worker_limit); | ||
worker_count = heap->workers()->update_active_workers(worker_count); | ||
log_info(gc, task)("Using %u workers of %u for full compaction", worker_count, max_worker_count); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's pre-existing, but this will change the number of active workers for the rest of the garbage collection. That made some sense previously as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was one of the reasons I went with using "just used regions" and skipping the part that each worker will handle a set of regions. In most cases looking at used regions will not limit the workers much, and if it does we don't have much work to do. I've done some benchmarking and not seen any significant regressions with this patch. The biggest problem was not using enough workers for the bitmap-work. Calculating workers per phase might be a good improvement to consider, but that would require some more refactoring. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay. If you think it would take too long, please file an issue for per-phase thread sizing in parallel gc then (if there isn't). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll file an RFE. |
||
|
||
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.
Should we actually end up here with zero used regions? It does not hurt, but seems superfluous
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.
No we should not, but I went with the super-safe option and added the
MAX2(...)
. If you like, I can remove it :)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 would prefer to either remove it or assert it. It confused at least me more than it seemed to be worth.
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'll add an assert.