1020364 - Rename --ion-parallel-compile to --ion-offthread-compile to remove confusion with PJS

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Hannes Verschore [:h4writer]]

It is possible to enable / disable background compilation in the browser and also in the shell.

in the browser you go to: about:config => javascript.options.ion.parallel_compilation

in the shell you can provide: --ion-parallel-compile=

Now this has landed before we had support for PJS (Parallel JS). For new persons it is a bit hard to understand the difference when parallel means parallel js and when background compilation. To decrease the confusing we should rename the above switch to "ion-offthread-compile". Which describes it better.

This requires changes to:
mozilla/js/src/shell/js.cpp (for --ion-parallel-compile)
mozilla/js/xpconnect/src/XPCJSRuntime.cpp (for javascript.options.ion.parallel_compilation)

At the same time the variables used there should get adjusted over the engine. To be consistent.

Comment 1

[Sushant Dinesh]

I think I am done making these changes. Any way to check if I've done this right?

Comment 2

[Hannes Verschore [:h4writer]]

(In reply to Sushant Dinesh from comment #1)
> I think I am done making these changes. Any way to check if I've done this
> right?

If you think you are done and things compile, you can put the patch up for review or post the patch and flag me for feedback.

Comment 3

[Sushant Dinesh]

Attachment: Rename parallel_compile to offthread_compile to avoid confusion.

Comment 4

[Sushant Dinesh]

Attachment: Rename parallel_compile to offthread_compile to avoid confusion.

Comment 5

[Hannes Verschore [:h4writer]]

I only took a quick glance. Tomorrow I'll do a proper review.

There are few places where --ion-parallel-compile is used. So that needs to get adjustd too:
./js/src/parjs-benchmarks/run.sh:    echo "$S" --ion-parallel-compile=on -e "'"'var libdir="'$D'/"; var MODE="'$MODE'";'"'" "$T"
./js/src/parjs-benchmarks/run.sh:    "$S" --ion-parallel-compile=on -e 'var libdir="'$D'/"; var MODE="'$MODE'";' "$T"
./js/src/jit-test/jit_test.py:        flags = [['--baseline-eager'], ['--ion-eager', '--ion-parallel-compile=off']]
./js/src/tests/lib/tests.py:    ['--ion-eager', '--ion-parallel-compile=off'], # implies --baseline-eager
./js/src/tests/lib/tests.py:    ['--ion-eager', '--ion-parallel-compile=off', '--ion-check-range-analysis', '--no-sse3'],

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Thanks Sean for pointing this out to me in-person.

We'll need to update the fuzzing harness - please request feedback? from me so I can test out the patch first (after you get an r+).

Comment 7

[Hannes Verschore [:h4writer]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #6)
> Thanks Sean for pointing this out to me in-person.

Oh yeah, I should have cc you guys from the beginning. I was actually waiting till there was a patch. I'll ask feedback after reviewing it. Other people I should inform?

Comment 8

[Hannes Verschore [:h4writer]]

Comment on attachment 8436953 [details] [diff] [review]
Rename parallel_compile to offthread_compile to avoid confusion.

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

::: js/src/shell/js.cpp
@@ +5968,2 @@
>  #endif
>  

Can you add an option that still checks "ion-parallel-compile" and when it is used reports an error like:
"--ion-parallel-compile is deprecated. Please use --ion-offthread-compile."

Comment 9

[Hannes Verschore [:h4writer]]

s/option/check/

Comment 10

[Sushant Dinesh]

Attachment: Rename parallel_compile to offthread_compile to avoid confusion.

Added the deprecation warning and made the changes in the test files you mentioned.

Comment 11

[Sushant Dinesh]

Attachment: Rename parallel_compile to offthread_compile to avoid confusion.

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8437576 [details] [diff] [review]
Rename parallel_compile to offthread_compile to avoid confusion.

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

::: js/src/shell/js.cpp
@@ +6035,1 @@
>      if (const char *str = op.getStringOption("ion-parallel-compile")) {

Can you add a newline before. And remove the "const char *str = ". Since the result isn't used.

Comment 13

[Hannes Verschore [:h4writer]]

Comment on attachment 8437576 [details] [diff] [review]
Rename parallel_compile to offthread_compile to avoid confusion.

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

Oh and also adjust ./jit-test/lib/jitopts.js to use offthread-compilation.enabled?

Comment 14

[Sushant Dinesh]

Attachment: Rename parallel_compile to offthread_compile to avoid confusion.

Added the changes suggested above.

Comment 15

[Sushant Dinesh]

Attachment: Rename parallel_compile to offthread_compile to avoid confusion.

Comment 16

[Hannes Verschore [:h4writer]]

Comment on attachment 8437654 [details] [diff] [review]
Rename parallel_compile to offthread_compile to avoid confusion.

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

Asking gkw for feedback to update the fuzzer harnass, before putting this in the tree.

Comment 17

[Christian Holler (:decoder)]

Comment on attachment 8437654 [details] [diff] [review]
Rename parallel_compile to offthread_compile to avoid confusion.

Fine with me, we just need to adjust fuzzing configurations as soon as this change landed :)

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8437654 [details] [diff] [review]
Rename parallel_compile to offthread_compile to avoid confusion.

Thanks for requesting feedback! I have now prepared for this on my side, feel free to land this.

Note that this patch seems to require rebasing to land on m-c tip.

Comment 19

[Hannes Verschore [:h4writer]]

Didn't land over the weekend so there is more people online to explain the change if it causes problems. Also making sure the fuzzers don't break in the weekend.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ab3b097f304

Comment 20

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0d67b1ccff9
Fixes the bustage.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ab3b097f304
https://hg.mozilla.org/mozilla-central/rev/f0d67b1ccff9

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Hannes Verschore [:h4writer] from comment #19)
> Didn't land over the weekend so there is more people online to explain the
> change if it causes problems. Also making sure the fuzzers don't break in
> the weekend.

Thanks for holding off till Monday! :) (especially since a follow-up was needed)

I've landed corresponding changes in the fuzzing harness so all should be well going forward.

Comment 23

[Guilherme Lima]

Apparently these patches made all browsers machines on AWFY to regress a lot.

Comment 24

[Hannes Verschore [:h4writer]]

(In reply to Guilherme Lima from comment #23)
> Apparently these patches made all browsers machines on AWFY to regress a lot.

Interesting. I just got the same regressions on talos. I'm guessing it is taking the wrong default value for offthread ending up disabling offthread compilation. Gonna check.

Comment 25

[Hannes Verschore [:h4writer]]

(In reply to Guilherme Lima from comment #23)
> Apparently these patches made all browsers machines on AWFY to regress a lot.

https://hg.mozilla.org/mozilla-central/rev/bb35d1b73634