Closed Bug 411267 Opened 15 years ago Closed 15 years ago

Less frequent operation callback calls

Categories

(Core :: DOM: Core & HTML, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

[This is a spin-off of bug 409109 comment 21]

The bug 409109 changes nsJSContext implementation to use operation counting API instead of branch callback ones to interrupt long-running scripts and periodically call GC. But the code in the last proposed patch in that bug does not change the frequency of the callback calls, the callback is still called at least once per backward jump in scrips.

It would be nice to make the calls less frequent and save about 20000 invocations of the callback when the browser starts while avoiding regressions that made Talos red (see 409109 comment 21).
Attached patch v1 (obsolete) — Splinter Review
The attached patch should be applied on top of the patch from bug 409109 comment 22. I will generate a CVS version after resolving that bug.
Attached patch v1 (cvs diff)Splinter Review
Here is a version of v1 as CVS diff. The patch changes the operation counting weight to call the callback from JS_OPERATION_WEIGHT_BASE to 5000 * JS_OPERATION_WEIGHT_BASE and modifies callback's code to do something usefull each time it is ccalled, not just once per 4096 calls. I have used 5000, not 4096, to account for the count rounding effect from bug 410969 comment 1.

The patch also checks if the global object is system each time it is called, not the first time after script's invocation. The idea behind this is that if there is a mixture of chrome/non-chrome cases contributing to the operation weight, then the longer time would be allowed for chrome if on average the callback is more frequently called for that case.
Attachment #295932 - Attachment is obsolete: true
Attachment #296406 - Flags: review?(jst)
Priority: -- → P2
This should provide the noticeable performance boost with looping JS code, see bug 364776 comment 8.
Status: NEW → ASSIGNED
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: P2 → P1
Comment on attachment 296406 [details] [diff] [review]
v1 (cvs diff)

Looks good, let's get it in and see how things look. I'd be interested in tuning the JS_MaybeGC characteristics of this code further, but that's material for another ubg...

r+sr=jst
Attachment #296406 - Flags: superreview+
Attachment #296406 - Flags: review?(jst)
Attachment #296406 - Flags: review+
I checked in the patch from comment 2 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%252Fcvsroot&date=explicit&mindate=1200691680&maxdate=1200691681&who=igor%25mir2.org

Lets see what happens with Talos boxes this time.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
The Talos box reported in bug 410969 comment 0, qm-mini-ubuntu02, and qm-mini-vista02 completed at least one green cycle with the patch applied. So it seems increasing JS_OPERATION_WEIGHT_BASE from 4096 (the original version) to 5000 helped.
The attachment 293938 [details] from bug 364776 comment 17 provides a way to test the speedup provided by the checked-in patch. 
Keywords: perf
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.