GCRuntime::gcIfNeededPerAllocation is a misleading name

RESOLVED FIXED in Firefox 55

Status

()

--
trivial
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: pbone, Assigned: pbone)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → pbone
(Assignee)

Comment 1

2 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 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

2 years ago
Attachment #8871878 - Attachment is obsolete: true
Attachment #8871893 - Flags: review+
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
Keywords: checkin-needed

Comment 4

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d3bad87d7582
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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.