Get rid of the compileAndGo flag in ContextOptions

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ejpbruel, Assigned: ejpbruel)

Tracking

(Blocks 1 bug)

unspecified
mozilla28
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

Assignee

Description

6 years ago
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.
Assignee

Comment 1

6 years ago
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.
Attachment #833060 - Flags: review?(bobbyholley+bmo)
Assignee

Updated

6 years ago
Attachment #833060 - Attachment description: patch → Pass CompileOptions as an argument to the JS_Compile* family of functions
Assignee

Comment 2

6 years ago
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.
Attachment #833061 - Flags: review?(bobbyholley+bmo)
Assignee

Comment 4

6 years ago
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.
Assignee

Comment 5

6 years ago
(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.
Assignee

Updated

6 years ago
Blocks: 934418
No longer blocks: 934419
(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)?
Assignee

Comment 7

6 years ago
(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.
Attachment #833060 - Flags: review?(bobbyholley+bmo) → review+
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
Attachment #833061 - Flags: review?(bobbyholley+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/1a83f8d13bdb
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.