Bug 192816

Summary: Use idiomatic const references for equality methods in JSC::JSValue class
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: JavaScriptCoreAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED WONTFIX    
Severity: Normal CC: ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, saam, ysuzuki
Priority: P2    
Version: WebKit Local Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=192813
Attachments:
Description Flags
Patch v1
ews-watchlist: commit-queue-
Patch v1 (retry EWS) none

Description David Kilzer (:ddkilzer) 2018-12-18 11:18:30 PST 
Use idiomatic const references for equality helper methods for JSC::JSValue class.
Comment 1 David Kilzer (:ddkilzer) 2018-12-18 11:26:51 PST 
Created attachment 357588 [details]
Patch v1
Comment 2 EWS Watchlist 2018-12-18 13:08:08 PST 
Comment on attachment 357588 [details]
Patch v1

Attachment 357588 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/10461961

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-cjit
stress/const-semantics.js.ftl-eager-no-cjit
apiTests
Comment 3 David Kilzer (:ddkilzer) 2018-12-18 14:10:56 PST 
(In reply to Build Bot from comment #2)
> Comment on attachment 357588 [details]
> Patch v1
> 
> Attachment 357588 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/10461961
> 
> New failing tests:
> stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-
> cjit
> stress/const-semantics.js.ftl-eager-no-cjit
> apiTests

That's surprising!  Will run the tests locally next.
Comment 4 David Kilzer (:ddkilzer) 2018-12-19 10:52:18 PST 
(In reply to David Kilzer (:ddkilzer) from comment #3)
> (In reply to Build Bot from comment #2)
> > Comment on attachment 357588 [details]
> > Patch v1
> > 
> > Attachment 357588 [details] did not pass jsc-ews (mac):
> > Output: https://webkit-queues.webkit.org/results/10461961
> > 
> > New failing tests:
> > stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-
> > cjit
> > stress/const-semantics.js.ftl-eager-no-cjit
> > apiTests
> 
> That's surprising!  Will run the tests locally next.

Running locally I don't see the stress/* test failures:

$ ./Tools/Scripts/run-javascriptcore-tests --no-build --no-fail-fast --release --verbose

For apiTests, I see:

2018-12-19 10:50:22.980 testapi[83267:1353141] Testing Objective-C API
2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Negative number maintained its original value": PASSED
2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "runJITThreadLimitTests() must run at the very beginning to test the case where the global JIT worklist was not initialized yet": PASSED
2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of DFG threads should be the value provided through Options": PASSED
2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of DFG threads should have been updated": PASSED
2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of DFG threads should be the value provided through Options": PASSED
2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of DFG threads should have been updated": PASSED
2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of FTL threads should be the value provided through Options": PASSED
2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of FTL threads should have been updated": FAILED
2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of FTL threads should be the value provided through Options": PASSED
2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of FTL threads should have been updated": FAILED
[...]
FAIL: Some tests failed.
testapi completed with rc=256 (1)

Next I need to rerun the tests locally without the change.
Comment 5 David Kilzer (:ddkilzer) 2018-12-19 12:46:06 PST 
(In reply to David Kilzer (:ddkilzer) from comment #4)
> (In reply to David Kilzer (:ddkilzer) from comment #3)
> > (In reply to Build Bot from comment #2)
> > > Comment on attachment 357588 [details]
> > > Patch v1
> > > 
> > > Attachment 357588 [details] did not pass jsc-ews (mac):
> > > Output: https://webkit-queues.webkit.org/results/10461961
> > > 
> > > New failing tests:
> > > stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-
> > > cjit
> > > stress/const-semantics.js.ftl-eager-no-cjit
> > > apiTests
> > 
> > That's surprising!  Will run the tests locally next.
> 
> Running locally I don't see the stress/* test failures:
> 
> $ ./Tools/Scripts/run-javascriptcore-tests --no-build --no-fail-fast
> --release --verbose
> 
> For apiTests, I see:
> 
> 2018-12-19 10:50:22.980 testapi[83267:1353141] Testing Objective-C API
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Negative number
> maintained its original value": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST:
> "runJITThreadLimitTests() must run at the very beginning to test the case
> where the global JIT worklist was not initialized yet": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of DFG
> threads should be the value provided through Options": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of DFG threads
> should have been updated": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of DFG
> threads should be the value provided through Options": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of DFG threads
> should have been updated": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of FTL
> threads should be the value provided through Options": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of FTL threads
> should have been updated": FAILED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of FTL
> threads should be the value provided through Options": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of FTL threads
> should have been updated": FAILED
> [...]
> FAIL: Some tests failed.
> testapi completed with rc=256 (1)
> 
> Next I need to rerun the tests locally without the change.

This was using a Release build of WebKit trunk r239276.
Comment 6 David Kilzer (:ddkilzer) 2018-12-20 04:50:27 PST 
(In reply to David Kilzer (:ddkilzer) from comment #4)
> (In reply to David Kilzer (:ddkilzer) from comment #3)
> > (In reply to Build Bot from comment #2)
> > > Comment on attachment 357588 [details]
> > > Patch v1
> > > 
> > > Attachment 357588 [details] did not pass jsc-ews (mac):
> > > Output: https://webkit-queues.webkit.org/results/10461961
> > > 
> > > New failing tests:
> > > stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-
> > > cjit
> > > stress/const-semantics.js.ftl-eager-no-cjit
> > > apiTests
> > 
> > That's surprising!  Will run the tests locally next.
> 
> Running locally I don't see the stress/* test failures:
> 
> $ ./Tools/Scripts/run-javascriptcore-tests --no-build --no-fail-fast
> --release --verbose
> 
> For apiTests, I see:
> 
> 2018-12-19 10:50:22.980 testapi[83267:1353141] Testing Objective-C API
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Negative number
> maintained its original value": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST:
> "runJITThreadLimitTests() must run at the very beginning to test the case
> where the global JIT worklist was not initialized yet": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of DFG
> threads should be the value provided through Options": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of DFG threads
> should have been updated": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of DFG
> threads should be the value provided through Options": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of DFG threads
> should have been updated": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of FTL
> threads should be the value provided through Options": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of FTL threads
> should have been updated": FAILED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Initial number of FTL
> threads should be the value provided through Options": PASSED
> 2018-12-19 10:50:22.988 testapi[83267:1353141] TEST: "Number of FTL threads
> should have been updated": FAILED
> [...]
> FAIL: Some tests failed.
> testapi completed with rc=256 (1)
> 
> Next I need to rerun the tests locally without the change.

The "Number of FTL threads should have been updated" test fails without this patch, so seems unrelated.
Comment 7 David Kilzer (:ddkilzer) 2018-12-20 04:51:26 PST 
Created attachment 357806 [details]
Patch v1 (retry EWS)

Uploading Patch v1 (Attachment 357588 [details]) again to force a second EWS run.
Comment 8 David Kilzer (:ddkilzer) 2018-12-20 05:00:52 PST 
Probably don't need to do this based on Darin's comment about WTF::StringView in Bug 192813 Comment #2.