Skip to content

Conversation

@peterzhu2118
Copy link

If we use the dynamic heap with NoGC, it will trigger a GC when space_full is true. This will cause a Rust panic of entered unreachable code: GC triggered in nogc because collection_required returns true.

If we use the dynamic heap with NoGC, it will trigger a GC when `space_full`
is true. This will cause a Rust panic of `entered unreachable code: GC triggered in nogc`
because `collection_required` returns true.
@qinsoon qinsoon requested a review from wks January 16, 2025 02:57
@qinsoon
Copy link
Member

qinsoon commented Jan 16, 2025

The current behavior of NoGC with dynamic heap sizes is that MMTk panics when the heap meets the current heap size (not the maximum heap size). This is not ideal.

With this PR, NoGC will keep allocating, regardless of the heap sizes (dynamic or fixed). I don't think it is correct either.

I think the expected behavior for NoGC with dynamic heap sizes is that MMTk keeps allocating until it reaches the maximum heap size. We need to do a bit more refactoring to allow that.

@qinsoon
Copy link
Member

qinsoon commented Jan 16, 2025

Maybe we can implement a special GC trigger specifically for NoGC with dynamic heap size to handle this. We cannot really use MemBalancer, as MemBalancer needs GC stats to adjust heap sizes, and we don't have GC stats in NoGC.

@wks
Copy link
Collaborator

wks commented Jan 16, 2025

I agree with @qinsoon. If we simply let collection_required return false, it will keep allocating until running out of physical memory, bypassing the GC trigger completely, and disregarding any heap size set by the user.

And "dynamic" heap size doesn't make sense for NoGC. There could be several solutions.

  1. Disallow any form of dynamic heap size when using NoGC. Simply panic if the user sets plan to NoGC and the GCTrigger to dynamic.
  2. Make MemBalancer set the initial heap size to the maximum if the plan is NoGC. This will gracefully degrade it to a fixed heap size (equal to the max heap size) anyway without crashing.

I prefer the second approach.

@qinsoon
Copy link
Member

qinsoon commented Jan 16, 2025

I agree with @qinsoon. If we simply let collection_required return false, it will keep allocating until running out of physical memory, bypassing the GC trigger completely, and disregarding any heap size set by the user.

And "dynamic" heap size doesn't make sense for NoGC. There could be several solutions.

  1. Disallow any form of dynamic heap size when using NoGC. Simply panic if the user sets plan to NoGC and the GCTrigger to dynamic.
  2. Make MemBalancer set the initial heap size to the maximum if the plan is NoGC. This will gracefully degrade it to a fixed heap size (equal to the max heap size) anyway without crashing.

I prefer the second approach.

We can just check here and if the plan is NoGC, we just create a FixedHeapSizeTrigger with the maximum heap size with a warning.

GCTriggerSelector::DynamicHeapSize(min, max) => Box::new(MemBalancerTrigger::new(
conversions::bytes_to_pages_up(min),
conversions::bytes_to_pages_up(max),
)),

This is probably cleaner, as we do not need to 'hack' mem balancer.

@peterzhu2118
Copy link
Author

Thanks for fixing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants