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

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: h4writer, Assigned: eternalsushant)

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)

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.
Whiteboard: [good first bug][mentor=h4writer][lang=c++]
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
Assignee: nobody → eternalsushant
I think I am done making these changes. Any way to check if I've done this right?
(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.
Attachment #8436949 - Flags: review?(hv1989)
Attachment #8436949 - Attachment is obsolete: true
Attachment #8436949 - Flags: review?(hv1989)
Attachment #8436953 - Flags: review?(hv1989)
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+).
(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 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."
s/option/check/
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)
Attachment #8437474 - Attachment is obsolete: true
Attachment #8437474 - Flags: review?(hv1989)
Attachment #8437576 - Flags: review?(hv1989)
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+
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?
Added the changes suggested above.
Attachment #8437576 - Attachment is obsolete: true
Attachment #8437641 - Flags: review?(hv1989)
Attachment #8437641 - Attachment is obsolete: true
Attachment #8437641 - Flags: review?(hv1989)
Attachment #8437654 - Flags: review?(hv1989)
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)
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)
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)
https://hg.mozilla.org/mozilla-central/rev/9ab3b097f304
https://hg.mozilla.org/mozilla-central/rev/f0d67b1ccff9
Status: NEW → RESOLVED
Closed: 5 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.
Apparently these patches made all browsers machines on AWFY to regress a lot.
(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)
(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)
Depends on: 1026355
Depends on: 1026794
You need to log in before you can comment on or make changes to this bug.