Closed Bug 1130226 Opened 5 years ago Closed 5 years ago

Rename some GC trigger symbols for sanity

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Attached patch 1_rename_gcIfNeeded-v0.diff (obsolete) — Splinter Review
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.
Attachment #8560176 - Flags: review?(sphink)
Comment on attachment 8560176 [details] [diff] [review]
1_rename_gcIfNeeded-v0.diff

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

Failed to qref. A moment.
Attachment #8560176 - Flags: review?(sphink)
Attachment #8560176 - Attachment is obsolete: true
Attachment #8560194 - Flags: review?(sphink)
Attachment #8560195 - Flags: review?(jcoppeard)
Comment on attachment 8560195 [details] [diff] [review]
2_rename_MAYBEGC-v0.diff

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

Much better!
Attachment #8560195 - Flags: review?(jcoppeard) → review+
Attachment #8560521 - Flags: review?(sphink)
Fixup the triggers comment to follow the renaming we just did.
Attachment #8560686 - Flags: review?(sphink)
Attachment #8560194 - Flags: review?(sphink) → review+
Attachment #8560521 - Flags: review?(sphink) → review+
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.
Attachment #8560686 - Flags: review?(sphink) → review+
Can this be the responsible for the regression on Splay and Splay-Latency on http://arewefastyet.com/#machine=29&view=breakdown&suite=octane ?
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
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.
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
And the regression is fixed \o/. Thanks for pointing out the regression Guilherme!
(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!
You need to log in before you can comment on or make changes to this bug.