867160 - Allow baseline to bypass scriptAnalysis on most scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Kannan Vijayan [:djvj]]

Implement just the simplified scriptAnalysis used by baseline instead of doing full |ensureRanAnalysis| before compiling with baseline.

Baseline only really uses |jumpTarget|, |stackDepth|, and presence-checks (to identify unreachable ops) from scriptAnalysis.  Those can be generated a lot more simply than running the heavyweight scriptAnalysis.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: Make baseline do its own script analysis

For scripts which are not |argumentsHasVarBinding|, this patch removes the usage of |ensureRanAnalysis| before baseline compilation.  Instead, a minimal custom analysis is done (cribbed from scriptAnalysis, but stripped of all the heavyweight stuff).

Analysis results are thrown away after compilation.  Also, scripts with stack depth exceeding 64K fail analysis (NO JIT FOR YOU!).

Passes jit-tests, doing try with linux and linux64 now.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 743629 [details] [diff] [review]
Make baseline do its own script analysis

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

Really glad to see this! Once bug 865059 lands this could be a nice memory/perf win, especially on huge files like Emscripten code...

::: js/src/ion/shared/BaselineCompiler-shared.cpp
@@ +10,5 @@
>  
>  using namespace js;
>  using namespace js::ion;
>  
> +static inline unsigned

I think this deserves its own file, like ion/BytecodeAnalysis.cpp, will also make it easier to use it in Ion later.

@@ +11,5 @@
>  using namespace js;
>  using namespace js::ion;
>  
> +static inline unsigned
> +GetDefCount(RawScript script, unsigned offset)

It would be nice if we could move these functions into jsopcodeinlines.h, so that they can be shared with jsanalyze.

@@ +19,5 @@
> +
> +    /*
> +     * Add an extra pushed value for OR/AND opcodes, so that they are included
> +     * in the pushed array of stack values for type inference.
> +     */

Nit: //-style comments in all these functions, update comment.

@@ +131,5 @@
> +            stackDepth -= nuses;
> +            stackDepth += ndefs;
> +
> +            /* If stack depth exceeds max allowed by analysis, fail fast. */
> +            if (stackDepth > BytecodeInfo::MAX_STACK_DEPTH)

Given that JSScript::nslots is uint16_t too, can this actually happen?

::: js/src/ion/shared/BaselineCompiler-shared.h
@@ +18,5 @@
>  namespace ion {
>  
> +/*
> + * Basic information about bytecodes in the script.  Used to help baseline compilation.
> + */

Nit: // comment

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: Updated patch with comments addressed

Comments addressed.

Also, I changed BytecodeNoFallThrough to BytecodeFallsThrough, and inverted the meaning, because it's always used in a negation context, and double negatives should be avoided.

if (BytecodeFallsThrough(...))

is a lot clearer than

if (!BytecodeNoFallThrough(...))

at first glance.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 744657 [details] [diff] [review]
Updated patch with comments addressed

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

Awesome. It probably doesn't matter much, but maybe good to discuss with Brian if this should land before/after bug 865059.

Comment 5

[Brian Hackett [Laid off!]]

I wouldn't consider bug 865059 as blocking this.  I have no idea when that bug will land fwiw, I'm still getting very intermittent talos crashes during Ion compilation on linux32 only (a browser I can't even build).

Comment 6

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4768761daaef

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4768761daaef