Bug 194375 – [JSC] sizeof(JSString) should be 16

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

Discussed with Keith. Currently, JSString takes 24bytes. Because of GC size classes, it becomes 32bytes.
But some members are just copy of things in StringImpl. We should remove length and flags fields from JSString to make it fit in 16bytes.

[Yusuke Suzuki]

Created attachment 361566 [details]
Patch

[EWS Watchlist]

Attachment 361566 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1549:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1560:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

In non-rope string boolify case, we can just use string pointer comparison with empty string, let's do that to minimize the bloated instructions.

[Yusuke Suzuki]

Created attachment 361603 [details]
Patch

[Yusuke Suzuki]

Created attachment 361717 [details]
Patch

[Yusuke Suzuki]

Created attachment 361778 [details]
Starts working on all tiers

[EWS Watchlist]

Attachment 361778 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSString.h:305:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:305:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:411:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1554:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1565:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 8 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

I've quickly run this with cli JetStream2, and surprisingly perf is not so bad (neutral). I thought I start optimizing bunch of things from now..., lol.

Before:
Total Score:  161.205
After:
Total Score:  161.397

[EWS Watchlist]

Comment on attachment 361778 [details]
Starts working on all tiers

Attachment 361778 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11119540

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 361785 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Yusuke Suzuki]

Created attachment 361786 [details]
Fix more

[EWS Watchlist]

Attachment 361786 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSString.h:305:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:305:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:413:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1554:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1565:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 8 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 361854 [details]
Fix more

[EWS Watchlist]

Attachment 361854 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/runtime/JSString.h:305:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:305:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:413:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1554:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1565:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 8 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 361868 [details]
More sane implementation

[EWS Watchlist]

Attachment 361868 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:193:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSString.h:304:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:304:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:412:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1554:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1565:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 9 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 361943 [details]
More changes for fences

[Yusuke Suzuki]

Created attachment 361958 [details]
Patch

[EWS Watchlist]

Attachment 361958 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:193:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSString.h:342:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:342:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:450:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1554:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1565:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 9 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 361961 [details]
Patch

[Yusuke Suzuki]

Created attachment 361962 [details]
Patch

[EWS Watchlist]

Attachment 361962 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:193:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSString.h:342:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:342:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:450:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1554:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1565:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 9 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 362097 [details]
Patch

[EWS Watchlist]

Attachment 362097 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:193:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSString.h:334:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:334:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:442:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1553:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1564:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 8 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 362097 [details]
Patch

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

New failing tests:
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager-no-cjit-validate
stress/to-lower-case-intrinsic-on-empty-rope.js.bytecode-cache
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-inline-validate
stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-collect-continuously
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit-b3o1
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.default
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-maximal-flush-validate-no-cjit
stress/to-lower-case-intrinsic-on-empty-rope.js.no-llint
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-small-pool
stress/to-lower-case-intrinsic-on-empty-rope.js.no-ftl
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-validate-sampling-profiler
stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-validate-phases
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-b3o1
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-put-stack-validate
apiTests

[Yusuke Suzuki]

(In reply to Build Bot from comment #25)
> Comment on attachment 362097 [details]
> Patch
> 
> Attachment 362097 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/11156325
> 
> New failing tests:
> stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager-no-cjit-validate
> stress/to-lower-case-intrinsic-on-empty-rope.js.bytecode-cache
> stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-inline-
> validate
> stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-collect-continuously
> stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit-b3o1
> stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager
> stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager
> stress/to-lower-case-intrinsic-on-empty-rope.js.default
> stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-maximal-flush-validate-
> no-cjit
> stress/to-lower-case-intrinsic-on-empty-rope.js.no-llint
> stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit
> stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-small-pool
> stress/to-lower-case-intrinsic-on-empty-rope.js.no-ftl
> stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-validate-
> sampling-profiler
> stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-validate-phases
> stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-b3o1
> stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-put-stack-
> validate
> apiTests

This test becomes invalid now since this patch starts not allowing empty ropes now. Need to fix this test (maybe, just checking the produced string becomes non rope).

[Yusuke Suzuki]

Created attachment 362362 [details]
Patch

[EWS Watchlist]

Attachment 362362 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:193:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSString.h:331:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:331:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:439:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1557:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1568:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 8 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 362362 [details]
Patch

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

New failing tests:
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager-no-cjit-validate
stress/to-lower-case-intrinsic-on-empty-rope.js.bytecode-cache
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-inline-validate
stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-collect-continuously
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit-b3o1
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.default
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-maximal-flush-validate-no-cjit
stress/to-lower-case-intrinsic-on-empty-rope.js.no-llint
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-small-pool
stress/to-lower-case-intrinsic-on-empty-rope.js.no-ftl
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-validate-sampling-profiler
stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-validate-phases
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-b3o0
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-put-stack-validate
apiTests

[Yusuke Suzuki]

Created attachment 362369 [details]
Patch

[EWS Watchlist]

Attachment 362369 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.cpp:347:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSString.h:390:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:390:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:491:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1557:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1568:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 9 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 362369 [details]
Patch

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

New failing tests:
webgl/1.0.2/conformance/more/conformance/quickCheckAPI-B3.html

[EWS Watchlist]

Created attachment 362370 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362369 [details]
Patch

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

New failing tests:
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager-no-cjit-validate
stress/StringObject-define-length-getter-rope-string-oom.js.dfg-eager-no-cjit-validate
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-eager
stress/regress-189132.js.dfg-maximal-flush-validate-no-cjit
stress/regress-189132.js.default
stress/to-lower-case-intrinsic-on-empty-rope.js.bytecode-cache
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-maximal-flush-validate-no-cjit
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-no-cjit-validate-sampling-profiler
stress/to-lower-case-intrinsic-on-empty-rope.js.no-ftl
stress/regress-189132.js.ftl-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-validate-phases
stress/StringObject-define-length-getter-rope-string-oom.js.bytecode-cache
stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-collect-continuously
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-no-cjit-no-put-stack-validate
stress/regress-189132.js.ftl-no-cjit-no-inline-validate
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-eager-no-cjit
stress/regress-189132.js.bytecode-cache
stress/regress-189132.js.dfg-eager-no-cjit-validate
stress/regress-189132.js.ftl-no-cjit-small-pool
stress/regress-189132.js.ftl-no-cjit-validate-sampling-profiler
stress/StringObject-define-length-getter-rope-string-oom.js.no-ftl
stress/regress-189132.js.ftl-eager-no-cjit-b3o1
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-eager-no-cjit-b3o1
stress/regress-189132.js.ftl-no-cjit-no-put-stack-validate
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-no-cjit-small-pool
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.no-llint
stress/regress-189132.js.ftl-eager-no-cjit
stress/StringObject-define-length-getter-rope-string-oom.js.dfg-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-put-stack-validate
stress/StringObject-define-length-getter-rope-string-oom.js.no-cjit-collect-continuously
stress/regress-189132.js.no-ftl
stress/regress-189132.js.no-llint
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-b3o0
stress/StringObject-define-length-getter-rope-string-oom.js.default
stress/regress-189132.js.ftl-no-cjit-b3o0
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit
stress/to-lower-case-intrinsic-on-empty-rope.js.default
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit-b3o1
stress/StringObject-define-length-getter-rope-string-oom.js.dfg-maximal-flush-validate-no-cjit
stress/StringObject-define-length-getter-rope-string-oom.js.no-llint
stress/regress-189132.js.no-cjit-validate-phases
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-inline-validate
stress/regress-189132.js.no-cjit-collect-continuously
stress/regress-189132.js.dfg-eager
stress/StringObject-define-length-getter-rope-string-oom.js.no-cjit-validate-phases
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-small-pool
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-validate-sampling-profiler
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-no-cjit-no-inline-validate
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-no-cjit-b3o0
apiTests

[EWS Watchlist]

Comment on attachment 362369 [details]
Patch

Attachment 362369 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11202335

New failing tests:
webgl/2.0.0/conformance/more/conformance/quickCheckAPI-B3.html

[EWS Watchlist]

Created attachment 362379 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Yusuke Suzuki]

Created attachment 362480 [details]
Patch

[EWS Watchlist]

Attachment 362480 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "exec" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.h:206:  The parameter name "cell" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.cpp:347:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/runtime/JSString.h:390:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:390:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSString.h:491:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1557:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1568:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 9 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 362480 [details]
Patch

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

New failing tests:
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager-no-cjit-validate
stress/StringObject-define-length-getter-rope-string-oom.js.dfg-eager-no-cjit-validate
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-eager
stress/regress-189132.js.dfg-maximal-flush-validate-no-cjit
stress/regress-189132.js.default
stress/to-lower-case-intrinsic-on-empty-rope.js.bytecode-cache
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-maximal-flush-validate-no-cjit
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-no-cjit-validate-sampling-profiler
stress/to-lower-case-intrinsic-on-empty-rope.js.no-ftl
stress/regress-189132.js.ftl-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-validate-phases
stress/StringObject-define-length-getter-rope-string-oom.js.bytecode-cache
stress/to-lower-case-intrinsic-on-empty-rope.js.no-cjit-collect-continuously
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-no-cjit-no-put-stack-validate
stress/regress-189132.js.ftl-no-cjit-no-inline-validate
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-eager-no-cjit
stress/regress-189132.js.bytecode-cache
stress/regress-189132.js.dfg-eager-no-cjit-validate
stress/regress-189132.js.ftl-no-cjit-small-pool
stress/regress-189132.js.ftl-no-cjit-validate-sampling-profiler
stress/StringObject-define-length-getter-rope-string-oom.js.no-ftl
stress/regress-189132.js.ftl-eager-no-cjit-b3o1
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-eager-no-cjit-b3o1
stress/regress-189132.js.ftl-no-cjit-no-put-stack-validate
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-no-cjit-small-pool
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.no-llint
stress/regress-189132.js.ftl-eager-no-cjit
stress/StringObject-define-length-getter-rope-string-oom.js.dfg-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-put-stack-validate
stress/StringObject-define-length-getter-rope-string-oom.js.no-cjit-collect-continuously
stress/regress-189132.js.no-ftl
stress/regress-189132.js.no-llint
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-b3o0
stress/StringObject-define-length-getter-rope-string-oom.js.default
stress/regress-189132.js.ftl-no-cjit-b3o0
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit
stress/to-lower-case-intrinsic-on-empty-rope.js.default
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-eager-no-cjit-b3o1
stress/StringObject-define-length-getter-rope-string-oom.js.dfg-maximal-flush-validate-no-cjit
stress/StringObject-define-length-getter-rope-string-oom.js.no-llint
stress/regress-189132.js.no-cjit-validate-phases
stress/to-lower-case-intrinsic-on-empty-rope.js.dfg-eager
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-no-inline-validate
stress/regress-189132.js.no-cjit-collect-continuously
stress/regress-189132.js.dfg-eager
stress/StringObject-define-length-getter-rope-string-oom.js.no-cjit-validate-phases
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-small-pool
stress/to-lower-case-intrinsic-on-empty-rope.js.ftl-no-cjit-validate-sampling-profiler
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-no-cjit-no-inline-validate
stress/StringObject-define-length-getter-rope-string-oom.js.ftl-no-cjit-b3o0
apiTests

[Yusuke Suzuki]

Created attachment 362595 [details]
Patch

[Yusuke Suzuki]

Created attachment 362624 [details]
Patch

[Yusuke Suzuki]

Created attachment 362625 [details]
Patch

[EWS Watchlist]

Attachment 362625 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 33 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 362627 [details]
Patch

[EWS Watchlist]

Attachment 362627 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 362629 [details]
Patch

[EWS Watchlist]

Attachment 362629 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 362632 [details]
Patch

[EWS Watchlist]

Attachment 362632 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 362638 [details]
Patch

[EWS Watchlist]

Attachment 362638 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 362638 [details]
Patch

Attachment 362638 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11237156

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/idbfactory-open-request-error.html

[EWS Watchlist]

Created attachment 362658 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362638 [details]
Patch

Attachment 362638 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11237101

New failing tests:
webgl/2.0.0/conformance/more/conformance/quickCheckAPI-B3.html

[EWS Watchlist]

Created attachment 362661 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Yusuke Suzuki]

Created attachment 362669 [details]
Patch

[EWS Watchlist]

Attachment 362669 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 362672 [details]
Patch

[EWS Watchlist]

Attachment 362672 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 362676 [details]
Patch

[EWS Watchlist]

Attachment 362676 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 362686 [details]
Patch

[EWS Watchlist]

Attachment 362686 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 362686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362686&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        Both classes cut 16 bytes per instance in GC allocation. This new layout is used in 64bit architectures which allows unaligned accesses and

Ignore this "unaligned access" part, this feature is no longer used in the latest patch. I'll drop this part.

[Yusuke Suzuki]

Comment on attachment 362686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362686&action=review

> Source/JavaScriptCore/runtime/JSString.h:494
>      {
>          Base::finishCreation(vm);
>          ASSERT(!sumOverflows<int32_t>(s1->length(), s2->length()));
> +        setIsSubstring(false);
>          setLength(s1->length() + s2->length());
>          setIs8Bit(s1->is8Bit() && s2->is8Bit());
> -        setIsSubstring(false);
> -        fiber(0).set(vm, this, s1);
> -        fiber(1).set(vm, this, s2);
> -        fiber(2).clear();
> +        initializeFiber0(vm, s1);
> +        initializeFiber1(vm, s2);
> +        initializeFiber2(vm, nullptr);
> +        ASSERT((s1->length() + s2->length()) == length());
>      }
>  
>      void finishCreation(VM& vm, JSString* s1, JSString* s2, JSString* s3)
>      {
>          Base::finishCreation(vm);
>          ASSERT(!sumOverflows<int32_t>(s1->length(), s2->length(), s3->length()));
> +        setIsSubstring(false);
>          setLength(s1->length() + s2->length() + s3->length());
>          setIs8Bit(s1->is8Bit() && s2->is8Bit() &&  s3->is8Bit());
> -        setIsSubstring(false);
> -        fiber(0).set(vm, this, s1);
> -        fiber(1).set(vm, this, s2);
> -        fiber(2).set(vm, this, s3);
> +        initializeFiber0(vm, s1);
> +        initializeFiber1(vm, s2);
> +        initializeFiber2(vm, s3);
> +        ASSERT((s1->length() + s2->length() + s3->length()) == length());
>      }
>  
>      void finishCreation(VM& vm, ExecState* exec, JSString* base, unsigned offset, unsigned length)

NOTE: Moving JSRopeString::finishCreation's operations into JSRopeString constructors is better since we would like to ensure that all the JSRopeString's fields valid before JSCell::finishCreation(vm) performs mutator fence. I'll move them in the updated patch.

[Saam Barati]

Comment on attachment 362686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362686&action=review

> Source/JavaScriptCore/ChangeLog:14
> +        We emit store-store fence when we put String pointer. This should exist even before this patch, so this patch also fixes one concurrency issue.

"store-store fence when we put String pointer" => "store-store fence before we put String pointer"

> Source/JavaScriptCore/ChangeLog:18
> +        fibers to compress three fibers + length + flags into three pointer width storage.

Maybe worth a memory diagram of the layout here for future reference?

> Source/JavaScriptCore/ChangeLog:24
> +

You need to talk about how you made the empty JSString* a singleton.

> Source/JavaScriptCore/bytecode/InlineAccess.cpp:322
> +    jit.loadPtr(CCallHelpers::Address(base, JSString::offsetOfValue()), scratch);
> +    auto isRope = jit.branchIfRopeStringImpl(scratch);
> +    jit.load32(CCallHelpers::Address(scratch, StringImpl::lengthMemoryOffset()), value.payloadGPR());
> +    auto done = jit.jump();
> +
> +    isRope.link(&jit);
> +    jit.load32(CCallHelpers::Address(base, JSRopeString::offsetOfLength()), value.payloadGPR());
> +
> +    done.link(&jit);

Let's verify this fits in the inline nop-sled still. I think it should.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:2180
> +    if (to > from && to > 0 && from < length) {
> +        if (from < 0)
> +            from = 0;
> +        if (to > length)
> +            to = length;
> +        scope.release();
> +        return jsSubstring(exec, string, static_cast<unsigned>(from), static_cast<unsigned>(to) - static_cast<unsigned>(from));
> +    }

Do we not have a helper for this already? Perhaps we can abstract parts out of the normal runtime functionality?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1554
> +    auto emitPopulateSliceIndex = [&] (Edge& target, GPRReg indexGPR, GPRReg lengthGPR, GPRReg resultGPR) {

Let's try to make the previous version of this have a variant that allows for your use case since the code is almost identical.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2189
> +    // unsigned comparison so we can filter out negative indices and indices that are too large

you should put this above comment above speculation check.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2233
> +    m_jit.loadPtr(MacroAssembler::Address(baseReg, JSString::offsetOfValue()), scratchReg);

aren't you missing an isRope check here? If so, please add a test.

> Source/JavaScriptCore/runtime/JSString.h:178
>          unsigned length = value->length();

Can we assert the length is non-zero?

[Yusuke Suzuki]

Comment on attachment 362686 [details]
Patch

Thanks! I'm now reflecting your comments on my patch :D

[Yusuke Suzuki]

Comment on attachment 362686 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362686&action=review

>> Source/JavaScriptCore/ChangeLog:9
>> +        Both classes cut 16 bytes per instance in GC allocation. This new layout is used in 64bit architectures which allows unaligned accesses and
> 
> Ignore this "unaligned access" part, this feature is no longer used in the latest patch. I'll drop this part.

Fixed.

>> Source/JavaScriptCore/ChangeLog:14
>> +        We emit store-store fence when we put String pointer. This should exist even before this patch, so this patch also fixes one concurrency issue.
> 
> "store-store fence when we put String pointer" => "store-store fence before we put String pointer"

Fixed.

>> Source/JavaScriptCore/ChangeLog:18
>> +        fibers to compress three fibers + length + flags into three pointer width storage.
> 
> Maybe worth a memory diagram of the layout here for future reference?

OK, I've pasted the diagram shown in JSString.h here :)

>> Source/JavaScriptCore/ChangeLog:24
>> +
> 
> You need to talk about how you made the empty JSString* a singleton.

Nice, yeah, I should do this. Fixed.

>> Source/JavaScriptCore/bytecode/InlineAccess.cpp:322
>> +    done.link(&jit);
> 
> Let's verify this fits in the inline nop-sled still. I think it should.

I've checked and found that basically various inline access sizes are changed. I've updated the numbers.

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:2180
>> +    }
> 
> Do we not have a helper for this already? Perhaps we can abstract parts out of the normal runtime functionality?

Make sense, I'll extract the similar part in runtime function and this DFG operation and put it in some Inlines header to be shared.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:1554
>> +    auto emitPopulateSliceIndex = [&] (Edge& target, GPRReg indexGPR, GPRReg lengthGPR, GPRReg resultGPR) {
> 
> Let's try to make the previous version of this have a variant that allows for your use case since the code is almost identical.

OK, fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2189
>> +    // unsigned comparison so we can filter out negative indices and indices that are too large
> 
> you should put this above comment above speculation check.

Oops, right. Fixed.

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:2233
>> +    m_jit.loadPtr(MacroAssembler::Address(baseReg, JSString::offsetOfValue()), scratchReg);
> 
> aren't you missing an isRope check here? If so, please add a test.

This is OK. As you can see, the old code does not have rope check too (the old code also accesses StringImpl::flagsOffset in L2187).
This is because this code is for GetByVal. We already emit Array check thing for Array::String (this is also asserted in L2230), and it forces rope to be resolved. So here, the input is always non rope string.

>> Source/JavaScriptCore/runtime/JSString.h:178
>>          unsigned length = value->length();
> 
> Can we assert the length is non-zero?

Sure! Fixed.

[Yusuke Suzuki]

Created attachment 362816 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 362816 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362816&action=review

> Source/JavaScriptCore/bytecode/InlineAccess.h:50
>  #if CPU(X86_64)
> -        return 23;
> +        return 26;
>  #elif CPU(X86)
>          return 27;
>  #elif CPU(ARM64)

I'll update ARM64 part in the subsequent revision of this patch.

[Yusuke Suzuki]

Created attachment 362817 [details]
Patch

[EWS Watchlist]

Attachment 362817 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 363064 [details]
Patch

[EWS Watchlist]

Attachment 363064 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 363076 [details]
Patch

[Yusuke Suzuki]

Created attachment 363077 [details]
Patch

[EWS Watchlist]

Attachment 363077 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ftl/FTLAbstractHeapRepository.h:196:  JSString_value is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 363077 [details]
Patch

Attachment 363077 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11300827

New failing tests:
http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html

[EWS Watchlist]

Created attachment 363080 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Yusuke Suzuki]

(In reply to Build Bot from comment #79)
> Created attachment 363080 [details]
> Archive of layout-test-results from ews107 for mac-highsierra-wk2
> 
> The attached test failures were seen while running run-webkit-tests on the
> mac-wk2-ews.
> Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

I don't think this is related since this crash happens in initAVEncoderBitRateKey, initialization part of softlinking.

[Yusuke Suzuki]

Comment on attachment 363077 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363077&action=review

> Source/JavaScriptCore/bytecode/InlineAccess.cpp:169
> +    constexpr bool failIfCantInline = true;

Oops, ignore this. I used this to explore the value I want.

[Saam Barati]

Comment on attachment 363077 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363077&action=review

r=me with comments.

> Source/JavaScriptCore/bytecode/InlineAccess.cpp:169
> +    constexpr bool failIfCantInline = true;

this should go back to being false.

> Source/JavaScriptCore/bytecode/InlineAccess.h:47
> +        return 26;

Do we need to bump sizes on arm64?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5269
> +    Allocator allocatorValue = allocatorForNonVirtualConcurrently<JSRopeString>(*m_jit.vm(), sizeof(JSRopeString), AllocatorForMode::AllocatorIfExists);

Should we assert this isn't null since we're calling AllocatorIfExists but also assuming it's a constant? E.g, the code below breaks if it's not a constant.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5324
> +            m_jit.and32(TrustedImm32(string->is8Bit() ? StringImpl::flagIs8Bit() : 0), scratchGPR);

Why not and16 to be consistent w/ below?

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6506
> +    struct FlagsAndLength {
> +        LValue flags;
> +        LValue length;
> +    };

style nit: This should go inside compileMakeRope

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6587
> +                    LValue length = lengthCheck;

this is unneeded.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6590
> +                        length

just use lengthCheck here

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6621
> +

Instead of having duplicate code in mergeFlagsAndLength and initializeFlagsAndLength, I think you could just repeatedly call initializeFlagAndLengths (and also rename the lambda) and just do:

LValue flags = m_out.bitAnd(flagsAndLength.flags, <flags>);
CheckValue* lengthCheck = m_out.speculateAdd(flagsAndLength.length, <length>));
return FlagsAndLength ...

> Source/JavaScriptCore/runtime/JSString.cpp:245
> +    WTF::storeStoreFence();
> +    new (&uninitializedValueInternal()) String(WTFMove(string));

This is valid today, since this constructor is just a single refptr move. However, this would be incorrect in the future if sizeof(String) ever changes. Maybe we should have a static_assert here?

It might be cleaner if we just treated this field like RefPtr<StringImpl> ourselves.

> Source/JavaScriptCore/runtime/JSString.cpp:246
> +    ASSERT(!JSString::isRope());

Might also be worth a comment saying we don't clear the other bits since we could be reading the length concurrently.

> Source/JavaScriptCore/runtime/JSString.h:41
> +IGNORE_RETURN_TYPE_WARNINGS_BEGIN

What's this needed for?

> Source/JavaScriptCore/runtime/JSString.h:280
> +    static constexpr uintptr_t flagMask = 0xffff000000000000ULL;
> +    static constexpr uintptr_t stringMask = ~(flagMask | IsRopeInPointer);
> +    static constexpr uintptr_t Is8BitInPointer = static_cast<uintptr_t>(Is8Bit) << 48;

style nit: capitalization of first letter seems inconsistent here.

> Source/JavaScriptCore/runtime/JSString.h:496
> +        initializeLength(s1->length() + s2->length() + s3->length());

Seems like this is prior code, but where do we check for overflow?

> Source/JavaScriptCore/runtime/JSString.h:541
> +        vm.heap.writeBarrier(this, s1);
> +        vm.heap.writeBarrier(this, s2);

Why? |this| is guaranteed to be in new space here.

> Source/JavaScriptCore/runtime/JSString.h:549
> +        vm.heap.writeBarrier(this, s1);
> +        vm.heap.writeBarrier(this, s2);
> +        vm.heap.writeBarrier(this, s3);

ditto

> Source/JavaScriptCore/runtime/JSString.h:556
> +        vm.heap.writeBarrier(this, updatedBase);

ditto

> Source/JavaScriptCore/runtime/JSString.h:667
> +        uintptr_t pointer = bitwise_cast<uintptr_t>(fiber);

let's also assert that !(pointer & ~stringMask)

> Source/JavaScriptCore/runtime/JSString.h:734
> +ALWAYS_INLINE bool JSString::is8Bit() const

It's worth a comment saying this can be run concurrently.

> Source/JavaScriptCore/runtime/JSString.h:750
> +ALWAYS_INLINE unsigned JSString::length() const

ditto

> Source/JavaScriptCore/runtime/JSString.h:754
> +        return static_cast<const JSRopeString*>(this)->length();

You should say why this is safe to call concurrently. It's because we never override the length bits when we resolve a rope.

Also nit: use jsCast

> JSTests/stress/to-lower-case-intrinsic-on-empty-rope.js:-24
> -    assert(isRope(rope)); // Keep this test future proofed. If this stops returning a rope, we should try to find another way to return an empty rope.

Instead of deleting this test, can you just change this to !isRope(rope) and rename variables appropriately?

[Yusuke Suzuki]

Comment on attachment 363077 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363077&action=review

Thanks!

>>> Source/JavaScriptCore/bytecode/InlineAccess.cpp:169
>>> +    constexpr bool failIfCantInline = true;
>> 
>> Oops, ignore this. I used this to explore the value I want.
> 
> this should go back to being false.

Fixed.

>> Source/JavaScriptCore/bytecode/InlineAccess.h:47
>> +        return 26;
> 
> Do we need to bump sizes on arm64?

Yes, I'm now updating it on ARM64 machine :)

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5269
>> +    Allocator allocatorValue = allocatorForNonVirtualConcurrently<JSRopeString>(*m_jit.vm(), sizeof(JSRopeString), AllocatorForMode::AllocatorIfExists);
> 
> Should we assert this isn't null since we're calling AllocatorIfExists but also assuming it's a constant? E.g, the code below breaks if it's not a constant.

If allocatorValue is nullptr constant, then emitAllocateJSCell emits the jump to the slow path. So this works :)

>> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:5324
>> +            m_jit.and32(TrustedImm32(string->is8Bit() ? StringImpl::flagIs8Bit() : 0), scratchGPR);
> 
> Why not and16 to be consistent w/ below?

Right now, and32 only has `TrustedImm32(), GPRReg` formed version in ARM64. I can add it easily, but given that we would change this code in a follow-up patch, I think we should do that in that patch if necessary.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6506
>> +    };
> 
> style nit: This should go inside compileMakeRope

Fixed

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6587
>> +                    LValue length = lengthCheck;
> 
> this is unneeded.

Right. Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6590
>> +                        length
> 
> just use lengthCheck here

Fixed.

>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:6621
>> +
> 
> Instead of having duplicate code in mergeFlagsAndLength and initializeFlagsAndLength, I think you could just repeatedly call initializeFlagAndLengths (and also rename the lambda) and just do:
> 
> LValue flags = m_out.bitAnd(flagsAndLength.flags, <flags>);
> CheckValue* lengthCheck = m_out.speculateAdd(flagsAndLength.length, <length>));
> return FlagsAndLength ...

Sounds reasonable. Fixed.

>> Source/JavaScriptCore/runtime/JSString.cpp:245
>> +    new (&uninitializedValueInternal()) String(WTFMove(string));
> 
> This is valid today, since this constructor is just a single refptr move. However, this would be incorrect in the future if sizeof(String) ever changes. Maybe we should have a static_assert here?
> 
> It might be cleaner if we just treated this field like RefPtr<StringImpl> ourselves.

Yes, and you're right. I like the idea, using RefPtr<StringImpl> directly in JSString. But it involves bunch of code changes and I would like to do that in a subsequent patch.
For now, I inserted static_assert(sizeof(String) == sizeof(RefPtr<StringImpl>)) here.

>> Source/JavaScriptCore/runtime/JSString.cpp:246
>> +    ASSERT(!JSString::isRope());
> 
> Might also be worth a comment saying we don't clear the other bits since we could be reading the length concurrently.

Right! Added a comment.

>> Source/JavaScriptCore/runtime/JSString.h:41
>> +IGNORE_RETURN_TYPE_WARNINGS_BEGIN
> 
> What's this needed for?

Previously I had a code requiring this but we no longer have that. We can remove this, fixed.

>> Source/JavaScriptCore/runtime/JSString.h:280
>> +    static constexpr uintptr_t Is8BitInPointer = static_cast<uintptr_t>(Is8Bit) << 48;
> 
> style nit: capitalization of first letter seems inconsistent here.

I'm still not sure the right convention because I can see JSString::MaxLength and JSBigInt::maxLength.
Here, I use `is8BitInPointer`. Fixed.

>> Source/JavaScriptCore/runtime/JSString.h:496
>> +        initializeLength(s1->length() + s2->length() + s3->length());
> 
> Seems like this is prior code, but where do we check for overflow?

Yes. Our jsString factory functions do that before calling actual JSRopeString::create / JSString::create functions because we already have another MaxLength limit.

>> Source/JavaScriptCore/runtime/JSString.h:541
>> +        vm.heap.writeBarrier(this, s2);
> 
> Why? |this| is guaranteed to be in new space here.

Right! This is unnecessary. Removed.

>> Source/JavaScriptCore/runtime/JSString.h:549
>> +        vm.heap.writeBarrier(this, s3);
> 
> ditto

Fixed.

>> Source/JavaScriptCore/runtime/JSString.h:556
>> +        vm.heap.writeBarrier(this, updatedBase);
> 
> ditto

Fixed.

> Source/JavaScriptCore/runtime/JSString.h:567
> +        vm.heap.writeBarrier(this, substringBase());

I removed this too.

>> Source/JavaScriptCore/runtime/JSString.h:667
>> +        uintptr_t pointer = bitwise_cast<uintptr_t>(fiber);
> 
> let's also assert that !(pointer & ~stringMask)

Sounds nice, fixed.

>> Source/JavaScriptCore/runtime/JSString.h:734
>> +ALWAYS_INLINE bool JSString::is8Bit() const
> 
> It's worth a comment saying this can be run concurrently.

Super nice. Fixed.

>> Source/JavaScriptCore/runtime/JSString.h:750
>> +ALWAYS_INLINE unsigned JSString::length() const
> 
> ditto

Fixed.

>> Source/JavaScriptCore/runtime/JSString.h:754
>> +        return static_cast<const JSRopeString*>(this)->length();
> 
> You should say why this is safe to call concurrently. It's because we never override the length bits when we resolve a rope.
> 
> Also nit: use jsCast

Fixed.

>> JSTests/stress/to-lower-case-intrinsic-on-empty-rope.js:-24
>> -    assert(isRope(rope)); // Keep this test future proofed. If this stops returning a rope, we should try to find another way to return an empty rope.
> 
> Instead of deleting this test, can you just change this to !isRope(rope) and rename variables appropriately?

Yeah, fixed.

[Yusuke Suzuki]

I changed InlineAccess's linkCodeInline to use `bool needsBranchCompaction = true`.
This is because now StringLength inline access code may use jumps, which are not correctly handled if ENABLE(BRANCH_COMPACTION) is on and needsBranchCompaction = false.

[Yusuke Suzuki]

Created attachment 363288 [details]
Patch for landing

[Yusuke Suzuki]

Committed r242252: <https://trac.webkit.org/changeset/242252>

[Radar WebKit Bug Importer]

<rdar://problem/48498246>

[Saam Barati]

(In reply to Yusuke Suzuki from comment #84)
> I changed InlineAccess's linkCodeInline to use `bool needsBranchCompaction =
> true`.
> This is because now StringLength inline access code may use jumps, which are
> not correctly handled if ENABLE(BRANCH_COMPACTION) is on and
> needsBranchCompaction = false.

Can you explain?

[Ryan Haddad]

Two webkitpy tests are failing after this change:

[1871/1875] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_8bit_string failed:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mojave-release-tests-wk1/build/Tools/lldb/lldb_webkit_unittest.py", line 118, in serial_test_WTFStringImpl_SummaryProvider_8bit_string
      self.assertEqual(summary, "{ length = 8, is8bit = 1, contents = 'r\\xe9sum\\xe9' }")
  AssertionError: "{ length = 8, is8bit = 0, contents = '\\uc372\\u73a9\\u6d75\\ua9c3\\x00\\x00\\x02\\x00' }" != "{ length = 8, is8bit = 1, contents = 'r\\xe9sum\\xe9' }"
  
[1873/1875] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_8bit_string failed:
  Traceback (most recent call last):
    File "/Volumes/Data/slave/mojave-release-tests-wk1/build/Tools/lldb/lldb_webkit_unittest.py", line 136, in serial_test_WTFString_SummaryProvider_8bit_string
      self.assertEqual(summary, "{ length = 8, contents = 'r\\xe9sum\\xe9' }")
  AssertionError: "{ length = 8, contents = '\\uc372\\u73a9\\u6d75\\ua9c3\\x00\\x00\\x02\\x00' }" != "{ length = 8, contents = 'r\\xe9sum\\xe9' }"
  
https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK1%20%28Tests%29/builds/3313/steps/webkitpy-test/logs/stdio

[Yusuke Suzuki]

(In reply to Ryan Haddad from comment #89)
> Two webkitpy tests are failing after this change:
> 
> [1871/1875]
> lldb_webkit_unittest.TestSummaryProviders.
> serial_test_WTFStringImpl_SummaryProvider_8bit_string failed:
>   Traceback (most recent call last):
>     File
> "/Volumes/Data/slave/mojave-release-tests-wk1/build/Tools/lldb/
> lldb_webkit_unittest.py", line 118, in
> serial_test_WTFStringImpl_SummaryProvider_8bit_string
>       self.assertEqual(summary, "{ length = 8, is8bit = 1, contents =
> 'r\\xe9sum\\xe9' }")
>   AssertionError: "{ length = 8, is8bit = 0, contents =
> '\\uc372\\u73a9\\u6d75\\ua9c3\\x00\\x00\\x02\\x00' }" != "{ length = 8,
> is8bit = 1, contents = 'r\\xe9sum\\xe9' }"
>   
> [1873/1875]
> lldb_webkit_unittest.TestSummaryProviders.
> serial_test_WTFString_SummaryProvider_8bit_string failed:
>   Traceback (most recent call last):
>     File
> "/Volumes/Data/slave/mojave-release-tests-wk1/build/Tools/lldb/
> lldb_webkit_unittest.py", line 136, in
> serial_test_WTFString_SummaryProvider_8bit_string
>       self.assertEqual(summary, "{ length = 8, contents = 'r\\xe9sum\\xe9'
> }")
>   AssertionError: "{ length = 8, contents =
> '\\uc372\\u73a9\\u6d75\\ua9c3\\x00\\x00\\x02\\x00' }" != "{ length = 8,
> contents = 'r\\xe9sum\\xe9' }"
>   
> https://build.webkit.org/builders/
> Apple%20Mojave%20Release%20WK1%20%28Tests%29/builds/3313/steps/webkitpy-test/
> logs/stdio

Oh! Yeah, this failure is caused by this patch because I changed StringImpl's flags. I'll land the fix for that.

[Yusuke Suzuki]

(In reply to Saam Barati from comment #88)
> (In reply to Yusuke Suzuki from comment #84)
> > I changed InlineAccess's linkCodeInline to use `bool needsBranchCompaction =
> > true`.
> > This is because now StringLength inline access code may use jumps, which are
> > not correctly handled if ENABLE(BRANCH_COMPACTION) is on and
> > needsBranchCompaction = false.
> 
> Can you explain?

Sure. Previously, InlineAccess uses `needsBranchCompaction = false` configuration when linking the code in LinkBuffer. This configuration is no-op in x64, and it affects on ARM64 and ARMv7. When we emit `jump()` we pessimistically emits instructions on ARM64. And LinkBuffer's copyCompactAndLinkCode attempts to shrink the code by compacting branches.

We have a special path in copyCompactAndLinkCode, which is used when we pass `needsBranchCompaction = false`. This mode is like, "If you know your generated code does not include any branches which can be compacted, then you can specify this option and copyCompactAndLinkCode skips compaction attempt". We used this option in InlineAccess's code generation because we did not emit any jumps except for patchable jumps in InlineAccess. Since patchable jumps are not compact-able, this option works fine. If this option is passed, we skip all the jump relocations and just copy the instruction data.

But now StringLength has jumps. So this assumption becomes broken. If we use needsBranchCompaction option, we emit the broken code since we skip all the jump relocations.
There are two ways to fix this issue I think. One is adding a jump relocation code to the path used for `needsBranchCompaction = false` too. Another is using `needsBranchCompaction = true`.
I took the second one here since this is simple. If skipping compaction has significant impact, we can implement it in a subsequent patch.

[Yusuke Suzuki]

Committed r242260: <https://trac.webkit.org/changeset/242260>

[Shawn Roberts]

It appears since this revision that this is causing 79 Assertion failures on Debug JSC

ChakraCore.yaml/ChakraCore/test/Basics/Length.js.default
ChakraCore.yaml/ChakraCore/test/TaggedIntegers/comparison.js.defaultChakraCore.yaml/ChakraCore/test/UnifiedRegex/propertyString.js.default
airjs-tests.yaml/stress-test.js
jsc-layout-tests.yaml/js/script-tests/equality.js
microbenchmarks/emscripten-cube2hash.js.no-ftl
stress/get-by-val-with-string-generated.js
stress/rest-parameter-many-arguments.js.
stress/tailCallForwardArguments.js.ftl-no-cjit-small-pool
wasm.yaml/wasm/js-api/globals-import.js

Build Log :

https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/2227/steps/jscore-test/logs/stdio

[Yusuke Suzuki]

(In reply to Shawn Roberts from comment #93)
> It appears since this revision that this is causing 79 Assertion failures on
> Debug JSC
> 
> ChakraCore.yaml/ChakraCore/test/Basics/Length.js.default
> ChakraCore.yaml/ChakraCore/test/TaggedIntegers/comparison.js.
> defaultChakraCore.yaml/ChakraCore/test/UnifiedRegex/propertyString.js.default
> airjs-tests.yaml/stress-test.js
> jsc-layout-tests.yaml/js/script-tests/equality.js
> microbenchmarks/emscripten-cube2hash.js.no-ftl
> stress/get-by-val-with-string-generated.js
> stress/rest-parameter-many-arguments.js.
> stress/tailCallForwardArguments.js.ftl-no-cjit-small-pool
> wasm.yaml/wasm/js-api/globals-import.js
> 
> Build Log :
> 
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/2227/steps/jscore-
> test/logs/stdio

I'll look into it now.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #94)
> (In reply to Shawn Roberts from comment #93)
> > It appears since this revision that this is causing 79 Assertion failures on
> > Debug JSC
> > 
> > ChakraCore.yaml/ChakraCore/test/Basics/Length.js.default
> > ChakraCore.yaml/ChakraCore/test/TaggedIntegers/comparison.js.
> > defaultChakraCore.yaml/ChakraCore/test/UnifiedRegex/propertyString.js.default
> > airjs-tests.yaml/stress-test.js
> > jsc-layout-tests.yaml/js/script-tests/equality.js
> > microbenchmarks/emscripten-cube2hash.js.no-ftl
> > stress/get-by-val-with-string-generated.js
> > stress/rest-parameter-many-arguments.js.
> > stress/tailCallForwardArguments.js.ftl-no-cjit-small-pool
> > wasm.yaml/wasm/js-api/globals-import.js
> > 
> > Build Log :
> > 
> > https://build.webkit.org/builders/
> > Apple%20High%20Sierra%20Debug%20JSC%20%28Tests%29/builds/2227/steps/jscore-
> > test/logs/stdio
> 
> I'll look into it now.

OK, this is missing exception check, I'll soon fix this.