932039 - jit::AnalyzeNewScriptProperties eagerly baseline-compile any "new"-script.

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

When Jits are enabled, any scripted-constructor would be eagerly compiled.  This is this the source of a start-up issue on B2G, as compiling eagerly implies creating an IonRuntime which takes around 25ms to get initialized.

Also, we should not eagerly compile any function if they are used only once.

I think the right way to work around this would be to remove the dependence on the BaselineInspector when we are using the DefinitePropertiesAnalysis fake-execution (compile) mode.

Comment 1

[Brian Hackett [Laid off!]]

(In reply to Nicolas B. Pierron [:nbp] from comment #0)
> When Jits are enabled, any scripted-constructor would be eagerly compiled. 
> This is this the source of a start-up issue on B2G, as compiling eagerly
> implies creating an IonRuntime which takes around 25ms to get initialized.

Is type inference enabled in any code which is running on startup?  It seems like the presence of any loop running 10 or more iterations would cause the script to get baseline compiled and incur the same overhead.  Bug 928562 added this baseline-compile-before-ionbuild step because the bytecode type map is now in the baseline script, and that bytecode type map needs to exist independently of ionbuilder.  IonBuilder could create its own bytecode type map if no baseline script was available, but that would incur some complexity and it would be nice to explore other avenues first.

> Also, we should not eagerly compile any function if they are used only once.

This really isn't a very good hard rule.  It's not ideal of course but we've already performed plenty of actions eagerly at the point the script first runs --- syntax-parsing, full-parsing and emitted to bytecode.

Comment 2

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

(In reply to Brian Hackett (:bhackett) from comment #1)
>  Bug 928562
> added this baseline-compile-before-ionbuild step because the bytecode type
> map is now in the baseline script, and that bytecode type map needs to exist
> independently of ionbuilder.  IonBuilder could create its own bytecode type
> map if no baseline script was available, but that would incur some
> complexity and it would be nice to explore other avenues first.

Ok, so if the type map is not available through baseline, then we should allocate one in IonBuilder and reuse it when we compile in baseline?

> > Also, we should not eagerly compile any function if they are used only once.
> 
> This really isn't a very good hard rule.  It's not ideal of course but we've
> already performed plenty of actions eagerly at the point the script first
> runs --- syntax-parsing, full-parsing and emitted to bytecode.

Because it is needed for running in the interpreter.  Compiling with Baseline is not needed for running in the interpreter, and we want to keep running in the interpreter until we reached the use-count or any heuristic based value, not by accident.

Comment 3

[Brian Hackett [Laid off!]]

(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> Ok, so if the type map is not available through baseline, then we should
> allocate one in IonBuilder and reuse it when we compile in baseline?

The one used by baseline is part of the baseline script, and the one used by IonBuilder would presumably be discarded immediately afterwards, so there's not much opportunity for reuse.  It would be fine if baseline just redid the analysis, it's quick, but I'm still wondering about why TI or the JITs are enabled at all in this compartment if keeping the IonRuntime from being initialized is so critical.

Comment 4

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

This bug does not seems to be responsible for the 50ms slow down noticed during the start-up of the settings app.  Still, it seems to be cost a lot of time in other applications.

I/JSPerf  ( 1102): AnalyzeNewScriptProperties:app://sms.gaiamobile.org/js/recipients.js:371: 12ms
I/JSPerf  ( 1212): AnalyzeNewScriptProperties:app://clock.gaiamobile.org/js/startup.js:2355: 12ms
I/JSPerf  ( 1195): AnalyzeNewScriptProperties:app://calendar.gaiamobile.org/js/views/time_header.js:5: 13ms
I/JSPerf  ( 1176): AnalyzeNewScriptProperties:app://fm.gaiamobile.org/shared/js/l10n.js:416: 15ms
I/JSPerf  ( 1245): AnalyzeNewScriptProperties:app://email.gaiamobile.org/js/ext/mailapi/worker-bootstrap.js:13261: 15ms
I/JSPerf  ( 1195): AnalyzeNewScriptProperties:app://calendar.gaiamobile.org/js/provider/local.js:5: 17ms
I/JSPerf  ( 1153): AnalyzeNewScriptProperties:app://gallery.gaiamobile.org/shared/js/mediadb.js:364: 18ms
I/JSPerf  ( 1245): AnalyzeNewScriptProperties:app://email.gaiamobile.org/js/cards/setup_account_info.js:17: 18ms
I/JSPerf  ( 1245): AnalyzeNewScriptProperties:app://email.gaiamobile.org/js/ext/mailapi/worker-bootstrap.js:14694: 22ms
I/JSPerf  ( 1245): AnalyzeNewScriptProperties:app://email.gaiamobile.org/js/ext/mailapi/worker-bootstrap.js:11978: 26ms

Comment 5

[Nicolas B. Pierron [:nbp | PTO until 11/11]]

Attachment: time-AnalyzeNewScriptProperties.diff

This patch was used to collect the values reported in the previous comment.
Note that I am not including the time of ensureJitCompartmentExists as it might create it the first time it is executed.