Closed
Bug 1193032
Opened 9 years ago
Closed 9 years ago
Fix some names in and about SliceBudget for sanity
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: terrence, Assigned: terrence)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
6.80 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
11.51 KB,
patch
|
ehoogeveen
:
review+
|
Details | Diff | Splinter Review |
3.46 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
1) We frequently call SliceBudgets on the stack |sliceBudget|. The name of the default time budget, however, is (GCRuntime::)sliceBudget, which is highly confusing to read. I've renamed the default time budget to defaultTimeBudget_, which makes everything much nicer. 2) SliceBudget::Unlimited is the same for both work and time budgets. This is not that big of a head-scratcher, but I still thought it would be worth giving these unique names.
Attachment #8645977 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 1•9 years ago
|
||
The default SliceBudget constructor gives an unlimited budget. It is easy to forget that the "default" constructed budget is not the same as the default budget that we actually use for GC (10ms or 40ms). To make this clearer, I've hidden the default constructor to force all users to instead construct with SliceBudget::unlimited(), so it's clear what you're getting in all cases.
Attachment #8645981 -
Flags: review?(emanuel.hoogeveen)
Assignee | ||
Comment 2•9 years ago
|
||
And the gecko changes required to use SliceBudget::unlimited().
Attachment #8645984 -
Flags: review?(continuation)
Comment 3•9 years ago
|
||
Comment on attachment 8645984 [details] [diff] [review] 2.6-gk-do_not_default_construct_SliceBudget-v0.diff Review of attachment 8645984 [details] [diff] [review]: ----------------------------------------------------------------- It is a little uglier, but on the other hand it is less magical.
Attachment #8645984 -
Flags: review?(continuation) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8645981 [details] [diff] [review] 2.6-sm-do_not_default_construct_SliceBudget-v0.diff Review of attachment 8645981 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, nicer to be explicit about this. I have mixed feelings about using auto here: using SliceBudget twice would be redundant, but in theory SliceBudget::unlimited() could return something other than a SliceBudget, e.g. an unlimited TimeBudget to initialize a budget somewhere down the line. I don't feel strongly about it though.
Attachment #8645981 -
Flags: review?(emanuel.hoogeveen) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8645977 [details] [diff] [review] 2.5_rename_slice_budget-v0.diff Review of attachment 8645977 [details] [diff] [review]: ----------------------------------------------------------------- Nice. ::: js/src/jsgc.cpp @@ +1502,5 @@ > availableChunks(lock).count() + > emptyChunks(lock).count()); > case JSGC_SLICE_TIME_BUDGET: > + MOZ_RELEASE_ASSERT(defaultTimeBudget_ < UINT32_MAX); > + return Max(0U, uint32_t(defaultTimeBudget_)); It's not obvious what's going on here. Maybe something more like the inverse of the set code above, e.g.: if (defaultTimeBudget_ == SliceBudget::UnlimitedTimeBudget) { return 0; } else { MOZ_ASSERT(...); return uint32_t(defaultTimeBudget_); }
Attachment #8645977 -
Flags: review?(jcoppeard) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b89a7ac12de https://hg.mozilla.org/integration/mozilla-inbound/rev/27d3fb0a4d0a
https://hg.mozilla.org/mozilla-central/rev/6b89a7ac12de https://hg.mozilla.org/mozilla-central/rev/27d3fb0a4d0a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in
before you can comment on or make changes to this bug.
Description
•