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

RESOLVED FIXED in mozilla33

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: h4writer, Assigned: Sushant Dinesh)

Tracking

unspecified
mozilla33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=h4writer][lang=c++])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Whiteboard: [good first bug][mentor=h4writer][lang=c++]
(Reporter)

Updated

3 years ago
Summary: Rename --ion-parallel-compile to --ion-offthread-compile to remove confusing with PJS → Rename --ion-parallel-compile to --ion-offthread-compile to remove confusion with PJS
(Reporter)

Updated

3 years ago
Assignee: nobody → eternalsushant
(Assignee)

Comment 1

3 years ago
I think I am done making these changes. Any way to check if I've done this right?
(Reporter)

Comment 2

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

Comment 3

3 years ago
Created attachment 8436949 [details]
Rename parallel_compile to offthread_compile to avoid confusion.
(Assignee)

Updated

3 years ago
Attachment #8436949 - Flags: review?(hv1989)
(Assignee)

Comment 4

3 years ago
Created attachment 8436953 [details] [diff] [review]
Rename parallel_compile to offthread_compile to avoid confusion.
Attachment #8436949 - Attachment is obsolete: true
Attachment #8436949 - Flags: review?(hv1989)
Attachment #8436953 - Flags: review?(hv1989)
(Reporter)

Comment 5

3 years ago
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'],
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+).
(Reporter)

Comment 7

3 years ago
(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?
(Reporter)

Comment 8

3 years ago
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."
(Reporter)

Comment 9

3 years ago
s/option/check/
(Assignee)

Comment 10

3 years ago
Created attachment 8437474 [details] [diff] [review]
Rename parallel_compile to offthread_compile to avoid confusion.

Added the deprecation warning and made the changes in the test files you mentioned.
Attachment #8436953 - Attachment is obsolete: true
Attachment #8436953 - Flags: review?(hv1989)
Attachment #8437474 - Flags: review?(hv1989)
(Assignee)

Comment 11

3 years ago
Created attachment 8437576 [details] [diff] [review]
Rename parallel_compile to offthread_compile to avoid confusion.
Attachment #8437474 - Attachment is obsolete: true
Attachment #8437474 - Flags: review?(hv1989)
Attachment #8437576 - Flags: review?(hv1989)
(Reporter)

Comment 12

3 years ago
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.
Attachment #8437576 - Flags: review?(hv1989) → review+
(Reporter)

Comment 13

3 years ago
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?
(Assignee)

Comment 14

3 years ago
Created attachment 8437641 [details] [diff] [review]
Rename parallel_compile to offthread_compile to avoid confusion.

Added the changes suggested above.
Attachment #8437576 - Attachment is obsolete: true
Attachment #8437641 - Flags: review?(hv1989)
(Assignee)

Comment 15

3 years ago
Created attachment 8437654 [details] [diff] [review]
Rename parallel_compile to offthread_compile to avoid confusion.
Attachment #8437641 - Attachment is obsolete: true
Attachment #8437641 - Flags: review?(hv1989)
Attachment #8437654 - Flags: review?(hv1989)
(Reporter)

Comment 16

3 years ago
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.
Attachment #8437654 - Flags: review?(hv1989)
Attachment #8437654 - Flags: review+
Attachment #8437654 - Flags: feedback?(gary)
(Reporter)

Updated

3 years ago
Attachment #8437654 - Flags: feedback?(choller)
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 :)
Attachment #8437654 - Flags: feedback?(choller) → feedback+
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.
Attachment #8437654 - Flags: feedback?(gary) → feedback+
Flags: needinfo?(hv1989)
(Reporter)

Comment 19

3 years ago
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
Flags: needinfo?(hv1989)
(Reporter)

Comment 20

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0d67b1ccff9
Fixes the bustage.
https://hg.mozilla.org/mozilla-central/rev/9ab3b097f304
https://hg.mozilla.org/mozilla-central/rev/f0d67b1ccff9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(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

3 years ago
Apparently these patches made all browsers machines on AWFY to regress a lot.
(Reporter)

Comment 24

3 years ago
(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.
Flags: needinfo?(hv1989)
(Reporter)

Comment 25

3 years ago
(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
Flags: needinfo?(hv1989)

Updated

3 years ago
Depends on: 1026355
Depends on: 1026794
You need to log in before you can comment on or make changes to this bug.