1130226 - Rename some GC trigger symbols for sanity

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 1_rename_gcIfNeeded-v0.diff

The gcreason MAYBEGC appears to be called this simply because it is the reason we use in maybeGC, unlike our others that actually reflect the, you know, the *reason*. The method gcIfNeeded makes a mockery of the root word "need": 'nuf said. The variable maxMallocBytes is highly misleading, if not arguably an outright lie, even if the publicly exposed API is MAX_MALLOC_BYTES. We can do better, suggestions welcome.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 8560176 [details] [diff] [review]
1_rename_gcIfNeeded-v0.diff

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

Failed to qref. A moment.

Comment 2

[Terrence Cole [:terrence]]

Attachment: 1_rename_gcIfNeeded-v1.diff

Comment 3

[Terrence Cole [:terrence]]

Attachment: 2_rename_MAYBEGC-v0.diff

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8560195 [details] [diff] [review]
2_rename_MAYBEGC-v0.diff

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

Much better!

Comment 5

[Terrence Cole [:terrence]]

Attachment: 3_rename_mallocBytes-v0.diff

Comment 6

[Terrence Cole [:terrence]]

Attachment: 4_fixup_comments-v0.diff

Fixup the triggers comment to follow the renaming we just did.

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8560686 [details] [diff] [review]
4_fixup_comments-v0.diff

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

::: js/src/gc/GCRuntime.h
@@ +443,5 @@
> + *      done a GC, we start an incremenal, all-zones, shrinking GC.
> + *
> + *          Assumptions:
> + *            -> Our triggers are incomplete.
> + *

\o/ Nice description.

Comment 8

[Terrence Cole [:terrence]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3e4d79297c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/02b800e34f0f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/16ddf000d4ba
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/032f3ac2b21d

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac3e4d79297c
https://hg.mozilla.org/mozilla-central/rev/02b800e34f0f
https://hg.mozilla.org/mozilla-central/rev/16ddf000d4ba
https://hg.mozilla.org/mozilla-central/rev/032f3ac2b21d

Comment 10

[Guilherme Lima]

Can this be the responsible for the regression on Splay and Splay-Latency on http://arewefastyet.com/#machine=29&view=breakdown&suite=octane ?

Comment 11

[Terrence Cole [:terrence]]

This is extremely unlikely to have caused a regression. I'd be more inclined to suspect one of bug 1130860, bug 1130798, or bug 1120169, all of which made actual changes to code that runs, if not on splay, at least near it.

1- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c939ab0df211&tochange=87d812111396

Comment 12

[Terrence Cole [:terrence]]

I am wrong. I bisected this down to ac3e4d79297c.

Given that this is just a name change I am a bit baffled, but will backout to investigate.

Comment 13

[Terrence Cole [:terrence]]

I missed that we have to pass the context in manually now. Not passing the context was disabling the pre-tenuring optimization that we make for splay. So that's still working at least.

https://hg.mozilla.org/integration/mozilla-inbound/rev/079a6f0578b9

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/079a6f0578b9

Comment 15

[Terrence Cole [:terrence]]

And the regression is fixed \o/. Thanks for pointing out the regression Guilherme!

Comment 16

[Guilherme Lima]

(In reply to Terrence Cole [:terrence] from comment #15)
> And the regression is fixed \o/. Thanks for pointing out the regression
> Guilherme!

My pleasure to contribute in some way to Firefox!