Closed Bug 1193032 Opened 4 years ago Closed 4 years ago

Fix some names in and about SliceBudget for sanity

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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)
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)
And the gecko changes required to use SliceBudget::unlimited().
Attachment #8645984 - Flags: review?(continuation)
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 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 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+
Depends on: 1237153
You need to log in before you can comment on or make changes to this bug.