1294563 - Yield before compacting GC if we are incremental

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

This got broken when we added a finalize state. We're now yielding at the end of sweeping... right before the zero-length finalize slices instead of before the long compacting slices. Should be trivial to fix.

Comment 1

[Terrence Cole [:terrence]]

Attachment: fix_pre_compacting_interrupt-v0.diff

I think this is probably not necessary because sweepZones should be relatively fast. Probably better to err on the size of low-latency though.

Comment 2

[Terrence Cole [:terrence]]

Looks like we don't even visit the thing for sweeping, so the final shutdown crashes in the pre-barrier as we've already torn down the heap.

Comment 3

[Terrence Cole [:terrence]]

Attachment: no_need_for_rekeyable_on_vmfunction-v1.diff

Let's just take out all the GC primitives since the map is not managed by the GC.

Comment 4

[Jon Coppeard (:jonco) (PTO until 5th Jan)]

(In reply to Terrence Cole [:terrence] from comment #3)
The above looks like a fine patch but I think you meant to open a different bug for it.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8793121 [details] [diff] [review]
no_need_for_rekeyable_on_vmfunction-v1.diff

Review of attachment 8793121 [details] [diff] [review]:
-----------------------------------------------------------------

I reviewed a similar patch as part of Bug 813683, I guess this is not the right attachment for this bug.

Comment 6

[Terrence Cole [:terrence]]

Yeah, wow, I totally crossed the two bugs I was working on yesterday. :-(

Comment 7

[Terrence Cole [:terrence]]

Attachment: fix_pre_compacting_interrupt-v0.diff

I did not mean to overwrite the original patch in this bug.

Comment 8

[Jon Coppeard (:jonco) (PTO until 5th Jan)]

Comment on attachment 8793373 [details] [diff] [review]
fix_pre_compacting_interrupt-v0.diff

Review of attachment 8793373 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah this looks good.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/755e16dfd05a
Fix the pre-compacting interrupt's position; r=jonco

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/755e16dfd05a

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/755e16dfd05a

Comment 12

[Gerry Chang [:gchang]]

Hi :jonco,
Since this bug is a regression and also affects 51, do you consider to uplift this for 51 if this patch is not too risky?

Comment 13

[Jon Coppeard (:jonco) (PTO until 5th Jan)]

Comment on attachment 8793373 [details] [diff] [review]
fix_pre_compacting_interrupt-v0.diff

Approval Request Comment
[Feature/regressing bug #]: Bug 1249367
[User impact if declined]: Possible jank due to garbage collection.
[Describe test coverage new/current, TreeHerder]: On m-c since 23rd September.
[Risks and why]: Low.
[String/UUID change made/needed]: None.

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8793373 [details] [diff] [review]
fix_pre_compacting_interrupt-v0.diff

Fix a regression related to GC. Take it in 51 aurora.

Comment 15

[Jon Coppeard (:jonco) (PTO until 5th Jan)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/828f4175481c030e8f39632f9623105745966411