Closed Bug 1154441 Opened 5 years ago Closed 5 years ago

Record the slice budget in the GC statistics


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox40 --- fixed


(Reporter: sfink, Assigned: sfink)



(1 file, 1 obsolete file)

When I see a 35ms slice, I always wonder whether it way overran a 10ms budget, or underran a 40ms budget.
Attached patch imported patch budget (obsolete) — Splinter Review
This is a little wonky, in that the SliceBudget calls PRMJ_Now() internally a little before we record the start time of the slice, so instead of 10ms you'll see about 9.98ms. I could switch it to recording the originally requested time if you'd prefer. The difference hasn't bothered me yet.
Attachment #8592438 - Flags: review?(terrence)
Using snprintf with %f might use a localized decimal separator of ',' instead of '.', causing havoc if we try to add this to our json output. Perhaps we should do the plumbing to pass the budget through as an integer number of ms or us.
We could also simply add the original budget to SliceBudget. I did this once in attachment 8529238 [details] [diff] [review], but it wasn't needed for anything. Thankfully I also changed SliceBudget::reset() to SliceBudget::makeUnlimited() - otherwise having the original budget on hand would have made things doubly confusing :)

One question is what to do about work budgets. Time budgets are stored internally (currently implicitly) in microseconds, but dividing work budgets by PRMJ_USEC_PER_MSEC would make no sense. Work budgets can be easily identified because they have a deadline of 0, so we could just do something different for them (though they aren't widely used anyway).
Ok, is this kosher? It's still creating an intermediate string that is unaware of locales and things, but now it's always an integer.

This does bloat up SliceBudget quite a bit. The compiler wasn't happy with a union. I don't know if it was previously getting passed around through a pair of registers or anything.
Attachment #8594099 - Flags: review?(terrence)
Attachment #8592438 - Attachment is obsolete: true
Attachment #8592438 - Flags: review?(terrence)
Comment on attachment 8594099 [details] [diff] [review]
imported patch budget

Review of attachment 8594099 [details] [diff] [review]:

Attachment #8594099 - Flags: review?(terrence) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.