Rename some GC trigger symbols for sanity

RESOLVED FIXED in Firefox 38

Status

()

Core
JavaScript: GC
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8560176 [details] [diff] [review]
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.
Attachment #8560176 - Flags: review?(sphink)
(Assignee)

Comment 1

3 years ago
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)
(Assignee)

Comment 2

3 years ago
Created attachment 8560194 [details] [diff] [review]
1_rename_gcIfNeeded-v1.diff
Attachment #8560176 - Attachment is obsolete: true
Attachment #8560194 - Flags: review?(sphink)
(Assignee)

Comment 3

3 years ago
Created attachment 8560195 [details] [diff] [review]
2_rename_MAYBEGC-v0.diff
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+
(Assignee)

Comment 5

3 years ago
Created attachment 8560521 [details] [diff] [review]
3_rename_mallocBytes-v0.diff
Attachment #8560521 - Flags: review?(sphink)
(Assignee)

Comment 6

3 years ago
Created attachment 8560686 [details] [diff] [review]
4_fixup_comments-v0.diff

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+
(Assignee)

Comment 8

3 years ago
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Comment 10

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

Comment 11

3 years ago
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
(Assignee)

Comment 12

3 years ago
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.
(Assignee)

Comment 13

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/079a6f0578b9
(Assignee)

Comment 15

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

Comment 16

3 years ago
(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.