Closed
Bug 1368086
Opened 7 years ago
Closed 7 years ago
GCRuntime::gcIfNeededPerAllocation is a misleading name
Categories
(Core :: JavaScript: GC, enhancement)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla55
| Tracking | Status | |
|---|---|---|
| firefox55 | --- | fixed |
People
(Reporter: pbone, Assigned: pbone)
Details
Attachments
(1 file, 1 obsolete file)
|
1.39 KB,
patch
|
pbone
:
review+
|
Details | Diff | Splinter Review |
No description provided.
|
Assignee | |
Updated•7 years ago
|
Assignee: nobody → pbone
|
|
Assignee | |
Comment 1•7 years ago
|
||
I didn't 'try' this patch since it's just a simple rename. It does build successfully.
Attachment #8871878 -
Flags: review?(sphink)
|
||
Comment 2•7 years ago
|
||
Comment on attachment 8871878 [details] [diff] [review] Rename gcIfNeededPerAllocation to gcIfNeededDuringAllocation Review of attachment 8871878 [details] [diff] [review]: ----------------------------------------------------------------- I was about to r- this because "during" confused me a bit; we don't GC during allocations, and the timespan of the need is not limited to the lifetime of the allocation or whatever. So clearly this should be gcIfNeededForAllocation. Except that's even worse, because we're just starting the GC. If we need to collect in order to allocate, this isn't going to do it. gcIfNeededByAllocation might work. But gcIfNeededDuringAllocation really isn't bad either. We *discover* the need during the allocation. Or then there's always maybeTriggerGCFromAllocation. r=me with any of these. (Ok, not gcIfNeededPerAllocation or gcIfNeededForAllocation.) If you keep it as is, just go ahead and mark checkin-needed. Otherwise, upload a new patch, set the r+ yourself, and mark checkin-needed.
Attachment #8871878 -
Flags: review?(sphink) → review+
|
|
Assignee | |
Comment 3•7 years ago
|
||
Attachment #8871878 -
Attachment is obsolete: true
Attachment #8871893 -
Flags: review+
|
|
Assignee | |
Updated•7 years ago
|
Status: NEW → ASSIGNED
Keywords: checkin-needed
Pushed by philringnalda@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d3bad87d7582 Rename gcIfNeededPerAllocation to gcIfNeededAtAllocation, r=sfink
Keywords: checkin-needed
|
||
Comment 5•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/d3bad87d7582
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•