Skip to content

Mark-compact GC: update STACK_TOP when growing mark stack#2673

Merged
mergify[bot] merged 4 commits intomasterfrom
osa1/mark_stack_bug
Jul 26, 2021
Merged

Mark-compact GC: update STACK_TOP when growing mark stack#2673
mergify[bot] merged 4 commits intomasterfrom
osa1/mark_stack_bug

Conversation

@osa1
Copy link
Contributor

@osa1 osa1 commented Jul 26, 2021

When growing mark stack we should update STACK_TOP otherwise subsequent
pushes will think that the stack is full and call grow_stack again.

Discovered while debugging strange allocation pattern in #2650 with the
compacting GC. It turns out once the mark stack is full ever subsequent push
(until stack is smaller than the initial size again) doubles the stack size,
causing redundant allocations.

Regression test added.

@osa1 osa1 requested a review from crusso July 26, 2021 07:15
@osa1 osa1 mentioned this pull request Jul 26, 2021
@dfinity-ci
Copy link

This PR does not affect the produced WebAssembly code.

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a bug fix?

@osa1 osa1 marked this pull request as ready for review July 26, 2021 10:02
@osa1
Copy link
Contributor Author

osa1 commented Jul 26, 2021

@crusso

Is this a bug fix?

Yes, updated the PR description.

@osa1 osa1 added the automerge-squash When ready, merge (using squash) label Jul 26, 2021
@@ -13,13 +13,13 @@ pub const INIT_STACK_SIZE: Words<u32> = Words(64);
static mut STACK_BLOB_PTR: *mut Blob = null_mut();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping that one private deliberately?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not used in tests so no need to make it pub. Should I make it pub for consistency?

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM.

Does this explain the weird GC cliff Andreas observed in your earlier PR? Probably not, but might be worth checking

@osa1
Copy link
Contributor Author

osa1 commented Jul 26, 2021

Does this explain the weird GC cliff Andreas observed in your earlier PR? Probably not, but might be worth checking

This fixes that, yes.

@crusso
Copy link
Contributor

crusso commented Jul 26, 2021

Yeah, sorry, that was actually clear from your updated PR description ;->

@mergify mergify bot merged commit d5e2253 into master Jul 26, 2021
@mergify mergify bot deleted the osa1/mark_stack_bug branch July 26, 2021 10:42
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jul 26, 2021
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