951679 - Move useCount, ion, and parallelIon from JSScript to BaselineScript

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[Kannan Vijayan [:djvj]]

This is partly prep-work for segregated loop compilation, but it has an immediate minor memory win so I thought it would be appropriate to land separately.

JSScript::useCount only goes up to 10 before a script is baseline compiled.  In general, we don't care about higher useCounts on all JSScripts - but only JSScripts which have a BaselineScript associated with them (since having a BaselineScript is a prerequisite for both sequential and parallel Ion compilation).

We can change JSScript::useCount to be a uint8_t field, and store the full uint32_t counter on the BaselineScript itself.  This removes a 32-bit field from JSScript and adds a 8-bit field, which should be a net win (at the least, no net loss).

Likewise, IonScripts can only exist for JSScripts which have a BaselineScript.  Instead of storing IonScripts (JSScript::ion and JSScript::parallelIon) on the Script itself, we can store them in the BaselineScript.  This removes two pointer fields from JSScript.  The only compensating fields required on JSScript are two flags for when the script is marked not-ion-compileable (we don't want to lose that info if the BaselineScript is gc-ed).

This change also helps implementation of segregated loop compilation because I eventually want to make it so that loop useCounts are tracked on a per-loop basis and for there to be potentially a separate IonScript for every loop in a given script.  Allocating these directly on JSScript is wasteful.  It's more efficient to allocate the memory for those fields on BaselineScripts only - we'll only use the extra space for scripts that actually get executed often enough to be baseline-compiled.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: move-usecount-to-baseline-script.patch

Passes jit-tests, green on try: https://tbpl.mozilla.org/?tree=Try&rev=31575bd9d1eb

Comment 2

[Kannan Vijayan [:djvj]]

Attachment: move-ion-and-parallel-ion-to-baseline-script.patch

Moves ion and parallelIon to BaselineScript.  To make sure that |canIonCompile| and |canParallelIonCompile| info is saved across GCs, two bit-flags are added to JSScript to track this info.

Seems green on try: https://tbpl.mozilla.org/?tree=Try&rev=8c1c8a51b81b

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8349447 [details] [diff] [review]
move-usecount-to-baseline-script.patch

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

First some high level question:
1) Why reset the usecount of JSScript to 0, when starting to use BaselineScript usecount. I don't understand the reasoning behind this.
2) I find it very confusing to have JSScript->usecount and BaselineScript->usecount, can we rename JSScript->usecount to warmupCount?
3) Would you mind doing the codegen of MRecompileCheck. (Optimization levels has landed and sticks)

::: js/src/jit/shared/BaselineCompiler-shared.h
@@ +65,5 @@
>      js::Vector<ICLoadLabel, 16, SystemAllocPolicy> icLoadLabels_;
>  
> +    // Labels for the 'moveWithPatch' for loading BaselineScript pointers
> +    // when incrementing the useCount.
> +    js::Vector<CodeOffsetLabel, 16, SystemAllocPolicy> baselineScriptPatchLabels_;

Do we have 16 entries that need to get patches by default? I would think only one per loop? So 1-2 should be good enough.

Comment 4

[Hannes Verschore [:h4writer]]

Comment on attachment 8349483 [details] [diff] [review]
move-ion-and-parallel-ion-to-baseline-script.patch

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

1) Just for myself. When do BaselineScripts removed? I thought GC only removes the stubs?

- Oh I really dislike to have the same functions on JSScript * and BaselineScript *. How bad would it be to move them all to BaselineScript * and remove the JSScript * ones?
- Another thing I have problems with is keeping the same information on 2 places. I.e. JSScript->forbidIonCompilation_ and BaselineScript->ion_ == ION_DISABLED_SCRIPT. This will get out of sync for sure.

I don't really how to solve this. I was thinking BaselineScripts getting removed don't happen often (Therefor my first question). So technically if we would keep the forbidIonCompilation only on BaselineScript wouldn't be that bad? It wouldn't take a lot of time to recompute it is "disabled"...

::: js/src/jit/BaselineJIT.h
@@ +300,5 @@
>      void resetUseCount() {
>          useCount_ = 0;
>      }
> +
> +    // Ion related methods.    

Nit: whitespace

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: move-usecount-ion-and-parallelion-to-baseline-script.patch

Trying to take incremental steps towards segregated loop compilation again.  These patches got sidelined due to complications related to off-thread IonBuilder stuff that was going on a few months back.

This patch seems to pass jit-tests.  Running on try now:

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

Comment 6

[Mayank Bansal]

Is this bug still relevant?

Comment 7

[Tooru Fujisawa [:arai]]

Thanks!
They're moved to dedicate struct in single field (js::BaseScript::warmUpData_), and no longer necessary to move to BaselineScript.