938907 - Get rid of the compileAndGo flag in ContextOptions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eddy Bruel [:ejpbruel]]

As its name already implies, this flag belongs more properly on CompileOptions. Since we already store it there, we should be able to remove it from ContextOptions.

This is a dependency for bug 934419, as it will give us one less flag to care about.

Comment 1

[Eddy Bruel [:ejpbruel]]

Attachment: Pass CompileOptions as an argument to the JS_Compile* family of functions

Once we remove the compile and go flag from the context options, there will be a couple of places, particularly in the JS shell, where we will have to pass the compile and go flag to the JS_Compile* family of functions. These functions currently do not take a CompileOptions as an argument.

Rather than adding the compile and go flag as an argument to these functions, it makes more sense to remove the filename and line number arguments, and pass all these three options via a single CompileOptions argument.

Comment 2

[Eddy Bruel [:ejpbruel]]

Attachment: Get rid of the compile and go flag on ContextOptions

Still pending a green try run, but it looks like this is all there is to removing the compile and go flag from the context options.

Comment 3

[Eddy Bruel [:ejpbruel]]

Try run looks good so far:

https://tbpl.mozilla.org/?tree=Try&rev=f38440cc7f8a

Comment 4

[Eddy Bruel [:ejpbruel]]

Caveat to that last patch: I still need to remove the compileAndGo field from the ContextOptions (it's currently always set to false). I'll add that to the patch pending review.

Comment 5

[Eddy Bruel [:ejpbruel]]

(In reply to Eddy Bruel [:ejpbruel] from comment #4)
> Caveat to that last patch: I still need to remove the compileAndGo field
> from the ContextOptions (it's currently always set to false). I'll add that
> to the patch pending review.

Nevermind that, I had already done that.

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Eddy Bruel [:ejpbruel] from comment #4)
> Caveat to that last patch: I still need to remove the compileAndGo field
> from the ContextOptions (it's currently always set to false). I'll add that
> to the patch pending review.

Always set to false ?!
Can you verify that Ion is compiling as expected, as Ion is only supposed to compile when compileAndGo is set to true.

Also, we have are doing optimizations of some JS included in XUL which have not known scope-chain and compileAndGo was used to optimize JS scripts with a known scope-chain, as well as backing in constants, is that still the case?
Do we still generate JSOP_OBJECT for array litterals (instead of JSOP_NEWARRAY + JSOP_INIT)?

Comment 7

[Eddy Bruel [:ejpbruel]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> (In reply to Eddy Bruel [:ejpbruel] from comment #4)
> > Caveat to that last patch: I still need to remove the compileAndGo field
> > from the ContextOptions (it's currently always set to false). I'll add that
> > to the patch pending review.
> 
> Always set to false ?!
> Can you verify that Ion is compiling as expected, as Ion is only supposed to
> compile when compileAndGo is set to true.
> 
> Also, we have are doing optimizations of some JS included in XUL which have
> not known scope-chain and compileAndGo was used to optimize JS scripts with
> a known scope-chain, as well as backing in constants, is that still the case?
> Do we still generate JSOP_OBJECT for array litterals (instead of
> JSOP_NEWARRAY + JSOP_INIT)?

Nicholas,

The compileAndGo field on CompileOptions it *initialized* to false in the constructor, but then set to true explicitly in the shell where appropriate. In other words: its still there, its just not initialized from the compile and go flag on the JSContext anymore.

The green try run in comment 3 seems to confirm that everything is okay, since it runs the jit-tests with the --tbpl flag enabled.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 833061 [details] [diff] [review]
Get rid of the compile and go flag on ContextOptions

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

Nice. r=bholley

Comment 9

[Eddy Bruel [:ejpbruel]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a83f8d13bdb

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a83f8d13bdb