973570 - Use compile-and-go for scripts in xpcshell

Status: RESOLVED INVALID

(Core :: XPConnect, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Noticed this while working on bug 939562. The JS shell and browser (for content) also use CNG, and it's fine to do the same for xpcshell, as we pass the same global object to Compile and Execute. This allows the JS engine to optimize name accesses better and to Ion-compile more.

The patch also removes an unnecessary root. Green on try with some other patches.

Comment 1

[Bobby Holley (:bholley)]

Comment on attachment 8377095 [details] [diff] [review]
Patch

Review of attachment 8377095 [details] [diff] [review]:
-----------------------------------------------------------------

I assume CNG scripts can still be XDR-serialized? Presumably they must, otherwise the component loader's use of the startup cache would be totally broken.

r=me assuming that's the case

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (:bholley) from comment #1)
> I assume CNG scripts can still be XDR-serialized? Presumably they must,
> otherwise the component loader's use of the startup cache would be totally
> broken.

Hrm, do we really XDR scripts that are loaded via |xpcshell foo.js| or load("foo.js")? This patch does not affect scripts loaded by mozJSComponentLoader etc.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Hrm, do we really XDR scripts that are loaded via |xpcshell foo.js| or
> load("foo.js")? This patch does not affect scripts loaded by
> mozJSComponentLoader etc.

Hm, I guess not actually. We certainly use xpcshell to precompile the startup cache, but the stuff we care about doesn't happen in the scope of the xpcshell global.

Does CNG prevent XDR encoding, in general?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (:bholley) from comment #3)
> Hm, I guess not actually. We certainly use xpcshell to precompile the
> startup cache, but the stuff we care about doesn't happen in the scope of
> the xpcshell global.

OK in that case this patch should be fine probably..

> Does CNG prevent XDR encoding, in general?

It used to, but bug 920322 ("Add support for compileAndGo optimizations to XDRScript") was fixed a few months ago, so presumably it should work now.

Comment 5

[Bobby Holley (:bholley)]

Ok. In that case we should probably turn on CNG for JSMs and whatnot, right?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (:bholley) from comment #5)
> Ok. In that case we should probably turn on CNG for JSMs and whatnot, right?

Yes, actually I was looking into that a few days ago (Ion only compiles CNG scripts atm, we could fix that pretty easily but CNG code is also easier to optimize in the parser/emitter etc).

Unfortunately I'm not familiar with the JSM code etc to know if there are other issues..

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> (In reply to Bobby Holley (:bholley) from comment #3)
> > Does CNG prevent XDR encoding, in general?
> 
> It used to, but bug 920322 ("Add support for compileAndGo optimizations to
> XDRScript") was fixed a few months ago, so presumably it should work now.

It works, but I still haven't landed the tests yet. The harness/tests are blocked by others patches which are preventing the tests to be green. (see Bug 900789)

The only details behind Bug 920322, is that we keep one version of the object as a template as long as we expect to XDR the object.  Do we XDR before, or after the execution?  In any case, you will have to turn cloneSingletons (CompartmentOptions) to "true" before doing any XDR or Execution, and XDR assert that this is maintained before cloning any object literals[1].  As soon as we do not intend to XDR anything, we can flip back the cloneSingleton flag to reuse objects instead of cloning them.

There is still another issue to correctly restore Singletons, but this would be fixed as part of Bug 946849.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jsobj.cpp?from=XDRObjectLiteral#1900

Comment 8

[Jan de Mooij [:jandem]]

Bug 679939 removed CNG.