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.
Created attachment 361566 [details] Patch
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.
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.
Created attachment 361603 [details] Patch
Created attachment 361717 [details] Patch
Created attachment 361778 [details] Starts working on all tiers
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.
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
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.
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
Created attachment 361786 [details] Fix more
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.
Created attachment 361854 [details] Fix more
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.
Created attachment 361868 [details] More sane implementation
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.
Created attachment 361943 [details] More changes for fences
Created attachment 361958 [details] Patch
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.
Created attachment 361961 [details] Patch
Created attachment 361962 [details] Patch
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.
Created attachment 362097 [details] Patch
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.
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
(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).
Created attachment 362362 [details] Patch
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.
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
Created attachment 362369 [details] Patch
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.
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
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
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
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
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
Created attachment 362480 [details] Patch
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.
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
Created attachment 362595 [details] Patch
Created attachment 362624 [details] Patch
Created attachment 362625 [details] Patch
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.
Created attachment 362627 [details] Patch
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.
Created attachment 362629 [details] Patch
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.
Created attachment 362632 [details] Patch
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.
Created attachment 362638 [details] Patch
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.
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
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
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
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
Created attachment 362669 [details] Patch
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.
Created attachment 362672 [details] Patch
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.
Created attachment 362676 [details] Patch
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.
Created attachment 362686 [details] Patch
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.
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.
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.
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?
Comment on attachment 362686 [details] Patch Thanks! I'm now reflecting your comments on my patch :D
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.
Created attachment 362816 [details] Patch
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.
Created attachment 362817 [details] Patch
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.
Created attachment 363064 [details] Patch
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.
Created attachment 363076 [details] Patch
Created attachment 363077 [details] Patch
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.
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
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
(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.
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.
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?
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.
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.
Created attachment 363288 [details] Patch for landing
Committed r242252: <https://trac.webkit.org/changeset/242252>
<rdar://problem/48498246>
(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?
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
(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.
(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.
Committed r242260: <https://trac.webkit.org/changeset/242260>
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
(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.
(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.