409109 - Replacing JS_SetBranchCallback by JS_SetOperationCallback

Status: RESOLVED FIXED

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

Description

[Igor Bukanov]

JS_SetOperationCallback API introduced by bug 364776 allows to avoid the overhead of calling the branch callback on each backward jump in a script. It would be nice to switch the browser to the new API.

Comment 1

[Igor Bukanov]

Attachment: v1

This is a version that I will test before asking for a review. Who should I ask for a review?

Comment 2

[Brendan Eich [:brendan]]

jst@moz, probably. Is it safe to use JS_GetOperationCallbackClosure instead of JS_GetContextPrivate in all the XPConnect cases? Are we no longer setting context data to point at the nsISupports when we set JSOPTION_PRIVATE_IS_ISUPPORTS? I did not see the patch for bug 364776 change that context option flag's meaning.

Separately, forgot to nit-pick the OperationCallbackClosure vs. operationClosure (member in JSContext) name disparity. Could go back to the longer but consistent name for the latter, if you care hit me for fast r/a+.

/be

Comment 3

[Igor Bukanov]

Attachment: v2

The patch switches nsJSContext implementation from dom/src/base/nsJSEnvironment.cpp and ContextHolder from js/src/xpconnect/src/xpccomponents.cpp to use the operation limit API from bug 364776. 

With this patch and the patch from bug 364776 comment 27 the best time from the test from bug 364776 comment 17 drops from 333 ms down to 278 ms, a 17% improvment.

Comment 4

[Mike Schroepfer]

Marking blocking P2 you are clear to land once reviews are good

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 294077 [details] [diff] [review]
v2

- In nsJSContext::ExecuteScript() etc:

+  // TODO: use JS_Begin/EndRequest to keep the GC from racing with JS
+  // execution on any thread.

This comment got reformatted, but it doesn't seem to apply any more. The code below here uses JSAutoRequest, so the TODO should be removed.

   jsval val;
   JSBool ok;
 
   nsJSContext::TerminationFuncHolder holder(this);
   JSAutoRequest ar(mContext);
   ok = ::JS_ExecuteScript(mContext,
                           (JSObject *)aScopeObject,
                           (JSScript*) ::JS_GetPrivate(mContext,

r+sr=jst

Comment 6

[Igor Bukanov]

Attachment: v1b

The new version removes TODO: comments.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 294454 [details] [diff] [review]
v1b

a=beltzner (though, blocking bugs don't need approvals as per the current tree rules)

Comment 8

[Igor Bukanov]

I checked in the patch from comment 6 to the CVS trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1198579560&maxdate=1198579587&who=igor%mir2.org

Comment 9

[Igor Bukanov]

To tinderbox watchers: I will be out of the internet soon as I forgot the power supply for my laptop at the office. So if tinderbox would be red, just take out the above patch.

Comment 10

[Jesse Ruderman]

Backed out in an attempt to fix the Talos red (tgfx failures) that started around the time this was checked in.

Comment 11

[Igor Bukanov]

(In reply to comment #2 from bug 409109)
> When Jesse backed out bug 364776 and bug 409109, boxset dropped right back to
> 900ms.

I suspect that this is caused by an extra GC cycle that the patch triggers as it may take 20ms or so. 

Comment 12

[Igor Bukanov]

I rechecked in the patch from comment 6 to CVS trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1199374020&maxdate=1199374381&who=igor%mir2.org

Comment 13

[:Gavin Sharp [email: [e-mail]]]

I've backed this patch out again because it was checked in just after fxdbug-win32-tb went orange, and the orange has been persistent since. We're kind of running out of options - rhelmer says the box looks normal, and backing out bug 409383 didn't help. I will check it back in if it doesn't fix the orange.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Backing this patch out seems to have fixed the orange. I will keep an eye on the box to make sure. The logs from the failing runs seem to either end with:

  Assertion failure: !rt->gcRunning, at e:/builds/tinderbox/Fx-Trunk-Memtest/WINNT_5.2_Depend/mozilla/js/src/jsgc.c:1336
----------- End Output from MozillaAliveTest --------- 
Error: firefox.exe: exited with status 3
MozillaAliveTest: test failed

which seems like it could be related to bug 402966, bug 407034, or bug 410323, or:

----------- End Output from trace-malloc bloat test --------- 
Error: bloat test timed out after 1800 seconds.

Comment 15

[Igor Bukanov]

Attachment: v2

The regression from comment 13 happens because after fixing the regression for the bug 364776 I have not updated the number in this patch before committing the patch again. Effectively this moved the regression to this patch via greater frequency of JS GC calls.

The new version adds a symbolic constant for operation counting base to jsapi.h and makes sure that the rest of code refers to it and not the hard-coded number. I wish I would use the constant in the first place and avoid the second regression. Sorry about that.

Comment 16

[Igor Bukanov]

Attachment: v1b-v2 delta

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 295370 [details] [diff] [review]
v2

>+ * When operationLimit is JS_OPERATION_WEIGHT_BASE, the callback will be
>+ * called at least after each backward jump in the interpreter. To minimize
>+ * the overhead of the callback invocation we suggest at least
>+ *   100 * JS_OPERATION_WEIGHT_BASE
>+ * as a value for operationLimit.

Nit: a blank comment-line around the excerpted expression would be nice.

r/a=me, this time for sure!

/be

Comment 18

[Igor Bukanov]

Attachment: v2b

The patch with the last nit addressed.

Comment 19

[Igor Bukanov]

I checked in the patch from comment 18 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1199532180&maxdate=1199532331&who=igor%mir2.org

Comment 20

[Samuel Sidler (old account; do not CC)]

This caused the same Talos red that Jesse mentioned in comment 10. I filed bug 410969 on it.

Comment 21

[Igor Bukanov]

i backed out the patch again. I will split the patch into 2, the first one would be about just changing the code to switch to the operation counting, and the second is about changing the frequency of callback calls.

Comment 22

[Igor Bukanov]

Attachment: v3

Compared with v2, the new patch in dom/src/base/nsJSEnvironment.(cpp|h) mostly does branch->operation renames and adjusts the API calls. The callback is still called after each backward script jump.

Comment 23

[Igor Bukanov]

I checked in the patch from comment 22 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1199979525&maxdate=1199979595&who=igor%mir2.org