Bug 192816 - Use idiomatic const references for equality methods in JSC::JSValue class
Summary: Use idiomatic const references for equality methods in JSC::JSValue class
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-12-18 11:18 PST by David Kilzer (:ddkilzer)
Modified: 2018-12-20 15:14 PST (History)
7 users (show)

See Also:


Attachments
Patch v1 (5.88 KB, patch)
2018-12-18 11:26 PST, David Kilzer (:ddkilzer)
ews: commit-queue-
Details | Formatted Diff | Diff
Patch v1 (retry EWS) (5.88 KB, patch)
2018-12-20 04:51 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 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.