Closed Bug 1496297 Opened 7 years ago Closed 7 years ago

Allow skip-if in jit-tests

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: efaust, Assigned: efaust)

References

Details

Attachments

(23 files, 1 obsolete file)

5.95 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
40.03 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
18.49 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
23.27 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
32.59 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
63.33 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
46.43 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.89 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.16 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
14.99 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.49 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
853 bytes, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.82 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
838 bytes, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.02 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
8.34 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.28 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
2.12 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
5.87 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
13.93 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.39 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
4.41 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.63 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
No description provided.
Attached patch [Part 1] Allow skip-if in jit-tests (obsolete) — — Splinter Review
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #9014251 - Flags: feedback?(jorendorff)
Summary: allow skip-if in jit-tests → Allow skip-if in jit-tests
Comment on attachment 9014251 [details] [diff] [review] [Part 1] Allow skip-if in jit-tests Review of attachment 9014251 [details] [diff] [review]: ----------------------------------------------------------------- This is basically exactly what I had.
Attachment #9014251 - Flags: feedback?(jorendorff) → feedback+
Attachment #9014251 - Attachment is obsolete: true
Attachment #9014528 - Flags: review?(jorendorff)
Attachment #9014529 - Flags: review?(jorendorff)
Attachment #9014531 - Flags: review?(jorendorff)
Attachment #9014533 - Flags: review?(jorendorff)
Attachment #9014534 - Flags: review?(jorendorff)
Attachment #9014535 - Flags: review?(jorendorff)
Attachment #9014536 - Flags: review?(jorendorff)
Attachment #9014538 - Flags: review?(jorendorff)
Attachment #9014540 - Flags: review?(jorendorff)
Attachment #9014541 - Flags: review?(jorendorff)
Attachment #9014542 - Flags: review?(jorendorff)
Attachment #9014543 - Flags: review?(jorendorff)
Attachment #9014544 - Flags: review?(jorendorff)
Attachment #9014545 - Flags: review?(jorendorff)
Attachment #9014546 - Flags: review?(jorendorff)
Attachment #9014547 - Flags: review?(jorendorff)
Attachment #9014548 - Flags: review?(jorendorff)
Attachment #9014549 - Flags: review?(jorendorff)
Attachment #9014550 - Flags: review?(jorendorff)
Attachment #9014551 - Flags: review?(jorendorff)
Attachment #9014552 - Flags: review?(jorendorff)
Attachment #9014553 - Flags: review?(jorendorff)
Attachment #9014554 - Flags: review?(jorendorff)
Comment on attachment 9014528 [details] [diff] [review] Implement |jit-test| skip-if: cond Review of attachment 9014528 [details] [diff] [review]: ----------------------------------------------------------------- \o/ ::: js/src/tests/lib/jittests.py @@ +199,5 @@ > # For each list of jit flags, make a copy of the test. > return [self.copy_and_extend_jitflags(v) for v in variants] > > COOKIE = '|jit-test|' > + SKIPPED_RETURN = 59 SKIPPED_EXIT_STATUS please.
Attachment #9014528 - Flags: review?(jorendorff) → review+
Attachment #9014529 - Flags: review?(jorendorff) → review+
Attachment #9014531 - Flags: review?(jorendorff) → review+
Attachment #9014533 - Flags: review?(jorendorff) → review+
Comment on attachment 9014534 [details] [diff] [review] Use |jit-test| skip-if in jit-test/tests/wasm Review of attachment 9014534 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/wasm/timeout/1.js @@ +2,3 @@ > > // Don't include wasm.js in timeout tests: when wasm isn't supported, it will > // quit(0) which will cause the test to fail. Is this comment still correct? Same story in 2.js, debug-interrupt-1.js, etc.
Attachment #9014534 - Flags: review?(jorendorff) → review+
Attachment #9014535 - Flags: review?(jorendorff) → review+
Comment on attachment 9014536 [details] [diff] [review] Use |jit-test| skip-if in jit-test/tests/debug Review of attachment 9014536 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/debug/wasm-12.js @@ +5,1 @@ > Extreme style nit: remove the extra blank line here, please.
Attachment #9014536 - Flags: review?(jorendorff) → review+
Attachment #9014538 - Flags: review?(jorendorff) → review+
Attachment #9014540 - Flags: review?(jorendorff) → review+
Comment on attachment 9014541 [details] [diff] [review] Use |jit-test| skip-if in jit-test/tests/asm.js Review of attachment 9014541 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/asm.js/testZOOB.js @@ +1,1 @@ > +// |jit-test|; skip-if: !isAsmJSCompilationAvailable() Nit: Stray semicolon here.
Attachment #9014541 - Flags: review?(jorendorff) → review+
Attachment #9014542 - Flags: review?(jorendorff) → review+
Comment on attachment 9014543 [details] [diff] [review] Use |jit-test| skip-if in jit-test/tests/latin1 Review of attachment 9014543 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the confusion addressed one way or the other... ::: js/src/jit-test/tests/latin1/asm.js @@ +7,1 @@ > quit(); Little confused about this one. Is this right? Should it throw if caching wasn't enabled?
Attachment #9014543 - Flags: review?(jorendorff) → review+
Attachment #9014544 - Flags: review?(jorendorff) → review+
Attachment #9014545 - Flags: review?(jorendorff) → review+
Attachment #9014546 - Flags: review?(jorendorff) → review+
Attachment #9014547 - Flags: review?(jorendorff) → review+
Attachment #9014548 - Flags: review?(jorendorff) → review+
Attachment #9014549 - Flags: review?(jorendorff) → review+
Attachment #9014550 - Flags: review?(jorendorff) → review+
Attachment #9014551 - Flags: review?(jorendorff) → review+
Attachment #9014552 - Flags: review?(jorendorff) → review+
Attachment #9014553 - Flags: review?(jorendorff) → review+
Attachment #9014554 - Flags: review?(jorendorff) → review+
Priority: -- → P2
(In reply to Jason Orendorff [:jorendorff] from comment #31) > Comment on attachment 9014543 [details] [diff] [review] > Use |jit-test| skip-if in jit-test/tests/latin1 > > Review of attachment 9014543 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with the confusion addressed one way or the other... > > ::: js/src/jit-test/tests/latin1/asm.js > @@ +7,1 @@ > > quit(); > > Little confused about this one. Is this right? Should it throw if caching > wasn't enabled? Seems right. setCacheEnabled(true) sets the "I want to have this enabled" bool, but isCachingEnabled() checks whether there's a cachepath as well. Seems like we need both, and that skipping peacefully is fine.
Pushed by efaustbmo@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2a7a0b6b2bbf Implement skip-if in jit-tests. (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/012e94d3d96c Use |jit-test| skip-if as appropriate in jit-test/tests/TypedObject/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/fdf9b230e270 Use |jit-test| skip-if as appropriate in jit-test/tests/basic/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/b97fa179df85 Use |jit-test| skip-if as appropriate in jit-test/tests/ion/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/1b29d20f0309 Use |jit-test| skip-if as appropriate to jit-test/tests/wasm/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/4157315ccd54 Use |jit-test| skip-if as appropriate in jit-test/tests/gc/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/cb0c1944852b Use |jit-test| skip-if as appropriate in jit-test/tests/debug/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/bb27fccdb19c Use |jit-test| skip-if as appropriate in jit-test/tests/baseline/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/5140037f3a17 Use |jit-test| skip-if as appropriate in jit-test/tests/auto-regress/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/b95948e9a126 Use |jit-test| skip-if as appropriate in jite-test/tests/asm.js/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/0b8297687615 Use |jit-test| skip-if as appropriate in jit-test/tests/regexp/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/9da32b3fe705 Use |jit-test| skip-if as appropriate in jit-test/tests/latin1/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/6112cec4211f Use |jit-test| skip-if as appropriate in jit-test/tests/atomics/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/f6a61c3b3dbb Use |jit-test| skip-if as appropriate in jit-test/tests/realms/ (r=jornedorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/085483ce64fe Use |jit-test| skip-if as appropriate in jit-test/tests/saved-stacks/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/ca4e8e9425ae Use |jit-test| skip-if as appropriate in jit-test/tests/sharedbuf/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/0fb70553886a Use |jit-test| skip-if as appropriate in jit-test/tests/parser/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/178e8e65a1a3 Use |jit-test| skip-if as appropriate in jit-test/tests/profiler/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/4da5afa3f427 Use |jit-test| skip-if as appropriate in jit-test/tests/modules/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/222b9bd838ec Use |jit-test| skip-if as appropriate in jit-test/tests/regexp_parse/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/de0ab3ea1796 Use |jit-test| skip-if as appropriate in jit-test/tests/xdr/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/179b81691182 Use |jit-test| skip-if as appropriate in jit-test/tests/heap-analysis/ (r=jorendorff) https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b420c49253 Use |jit-test| skip-if as appropriate in jit-test/tests/ (r=jorendorff)
I'm getting a warning locally running jit-tests: /home/jon/clone/modules/js/src/jit-test/tests/gc/bug-1155455.js: warning: unrecognized |jit-test| attribute skip-if !('gczeal' in this) I don't know why this is happening for this test in particular.
That seems well worth looking into.
Flags: needinfo?(efaustbmo)
Depends on: 1497978
Flags: needinfo?(efaustbmo)
This turned all of the tier3 jit tests orange on the pixel2 aarch64 tests: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&tier=1,2,3&searchStr=android-hw&group_state=expanded&revision=c8b420c49253532689f462813d7c9673ba798165 Was that expected? We previously had perma orange for jit{5,6,10} which prevented the tests from moving up to tier 2. Now, it is not apparent these tests are worth running in this state. Should I file a follow up bug for these new errors or to just disable the jit tests on pixel2 aarch64 altogether?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: