Bug 194375 - [JSC] sizeof(JSString) should be 16
Summary: [JSC] sizeof(JSString) should be 16
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 194471
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-06 21:04 PST by Yusuke Suzuki
Modified: 2019-03-05 11:10 PST (History)
16 users (show)

See Also:


Attachments
Patch (90.75 KB, patch)
2019-02-08 17:08 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (111.30 KB, patch)
2019-02-09 00:00 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (102.56 KB, patch)
2019-02-11 15:34 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Starts working on all tiers (119.44 KB, patch)
2019-02-12 00:27 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-highsierra (411.81 KB, application/zip)
2019-02-12 01:40 PST, Build Bot
no flags Details
Fix more (119.69 KB, patch)
2019-02-12 02:21 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Fix more (119.57 KB, patch)
2019-02-12 15:34 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
More sane implementation (123.23 KB, patch)
2019-02-12 17:01 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
More changes for fences (125.00 KB, patch)
2019-02-13 15:09 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (120.76 KB, patch)
2019-02-13 16:32 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (120.83 KB, patch)
2019-02-13 16:36 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (120.45 KB, patch)
2019-02-13 16:39 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (128.81 KB, patch)
2019-02-14 22:42 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (140.91 KB, patch)
2019-02-18 18:41 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (146.17 KB, patch)
2019-02-18 23:09 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.54 MB, application/zip)
2019-02-19 00:27 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.80 MB, application/zip)
2019-02-19 03:32 PST, Build Bot
no flags Details
Patch (148.46 KB, patch)
2019-02-19 21:02 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (155.66 KB, patch)
2019-02-20 22:39 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (156.45 KB, patch)
2019-02-21 11:46 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (156.77 KB, patch)
2019-02-21 11:55 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (154.37 KB, patch)
2019-02-21 12:20 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (154.37 KB, patch)
2019-02-21 12:46 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (155.40 KB, patch)
2019-02-21 12:54 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (154.55 KB, patch)
2019-02-21 13:22 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-highsierra (2.21 MB, application/zip)
2019-02-21 15:59 PST, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.49 MB, application/zip)
2019-02-21 16:11 PST, Build Bot
no flags Details
Patch (156.10 KB, patch)
2019-02-21 16:34 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (156.32 KB, patch)
2019-02-21 17:12 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (156.19 KB, patch)
2019-02-21 17:45 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (158.95 KB, patch)
2019-02-21 19:31 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (181.44 KB, patch)
2019-02-22 22:53 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (181.49 KB, patch)
2019-02-22 23:01 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (181.06 KB, patch)
2019-02-26 21:17 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (182.56 KB, patch)
2019-02-26 23:30 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (182.42 KB, patch)
2019-02-26 23:57 PST, Yusuke Suzuki
sbarati: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.58 MB, application/zip)
2019-02-27 01:29 PST, Build Bot
no flags Details
Patch for landing (183.37 KB, patch)
2019-02-28 19:12 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-02-06 21:04:20 PST
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.
Comment 1 Yusuke Suzuki 2019-02-08 17:08:32 PST
Created attachment 361566 [details]
Patch
Comment 2 Build Bot 2019-02-08 17:11:28 PST
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.
Comment 3 Yusuke Suzuki 2019-02-08 22:48:47 PST
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.
Comment 4 Yusuke Suzuki 2019-02-09 00:00:52 PST
Created attachment 361603 [details]
Patch
Comment 5 Yusuke Suzuki 2019-02-11 15:34:47 PST
Created attachment 361717 [details]
Patch
Comment 6 Yusuke Suzuki 2019-02-12 00:27:38 PST
Created attachment 361778 [details]
Starts working on all tiers
Comment 7 Build Bot 2019-02-12 00:31:46 PST
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.
Comment 8 Yusuke Suzuki 2019-02-12 00:35:34 PST
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 9 Build Bot 2019-02-12 01:40:54 PST
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.
Comment 10 Build Bot 2019-02-12 01:40:56 PST
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
Comment 11 Yusuke Suzuki 2019-02-12 02:21:23 PST
Created attachment 361786 [details]
Fix more
Comment 12 Build Bot 2019-02-12 02:39:26 PST
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.
Comment 13 Yusuke Suzuki 2019-02-12 15:34:11 PST
Created attachment 361854 [details]
Fix more
Comment 14 Build Bot 2019-02-12 15:37:39 PST
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.
Comment 15 Yusuke Suzuki 2019-02-12 17:01:28 PST
Created attachment 361868 [details]
More sane implementation
Comment 16 Build Bot 2019-02-12 17:29:38 PST
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.
Comment 17 Yusuke Suzuki 2019-02-13 15:09:58 PST
Created attachment 361943 [details]
More changes for fences
Comment 18 Yusuke Suzuki 2019-02-13 16:32:16 PST
Created attachment 361958 [details]
Patch
Comment 19 Build Bot 2019-02-13 16:36:01 PST
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.
Comment 20 Yusuke Suzuki 2019-02-13 16:36:12 PST
Created attachment 361961 [details]
Patch
Comment 21 Yusuke Suzuki 2019-02-13 16:39:05 PST
Created attachment 361962 [details]
Patch
Comment 22 Build Bot 2019-02-13 16:40:44 PST
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.
Comment 23 Yusuke Suzuki 2019-02-14 22:42:30 PST
Created attachment 362097 [details]
Patch
Comment 24 Build Bot 2019-02-14 22:44:47 PST
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 25 Build Bot 2019-02-15 01:22:45 PST
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
Comment 26 Yusuke Suzuki 2019-02-15 02:17:21 PST
(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).
Comment 27 Yusuke Suzuki 2019-02-18 18:41:49 PST
Created attachment 362362 [details]
Patch
Comment 28 Build Bot 2019-02-18 18:44:42 PST
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 29 Build Bot 2019-02-18 21:23:56 PST
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
Comment 30 Yusuke Suzuki 2019-02-18 23:09:00 PST
Created attachment 362369 [details]
Patch
Comment 31 Build Bot 2019-02-18 23:11:32 PST
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 32 Build Bot 2019-02-19 00:27:06 PST
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
Comment 33 Build Bot 2019-02-19 00:27:08 PST
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 34 Build Bot 2019-02-19 01:47:44 PST
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 35 Build Bot 2019-02-19 03:32:30 PST
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
Comment 36 Build Bot 2019-02-19 03:32:32 PST
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
Comment 37 Yusuke Suzuki 2019-02-19 21:02:18 PST
Created attachment 362480 [details]
Patch
Comment 38 Build Bot 2019-02-19 21:05:11 PST
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 39 Build Bot 2019-02-19 23:42:47 PST
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
Comment 40 Yusuke Suzuki 2019-02-20 22:39:54 PST
Created attachment 362595 [details]
Patch
Comment 41 Yusuke Suzuki 2019-02-21 11:46:17 PST
Created attachment 362624 [details]
Patch
Comment 42 Yusuke Suzuki 2019-02-21 11:55:14 PST
Created attachment 362625 [details]
Patch
Comment 43 Build Bot 2019-02-21 11:57:05 PST
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.
Comment 44 Yusuke Suzuki 2019-02-21 12:20:00 PST
Created attachment 362627 [details]
Patch
Comment 45 Build Bot 2019-02-21 12:23:26 PST
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.
Comment 46 Yusuke Suzuki 2019-02-21 12:46:58 PST
Created attachment 362629 [details]
Patch
Comment 47 Build Bot 2019-02-21 12:49:44 PST
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.
Comment 48 Yusuke Suzuki 2019-02-21 12:54:01 PST
Created attachment 362632 [details]
Patch
Comment 49 Build Bot 2019-02-21 12:57:33 PST
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.
Comment 50 Yusuke Suzuki 2019-02-21 13:22:03 PST
Created attachment 362638 [details]
Patch
Comment 51 Build Bot 2019-02-21 13:25:42 PST
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 52 Build Bot 2019-02-21 15:59:57 PST
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
Comment 53 Build Bot 2019-02-21 15:59:59 PST
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 54 Build Bot 2019-02-21 16:11:10 PST
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
Comment 55 Build Bot 2019-02-21 16:11:13 PST
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
Comment 56 Yusuke Suzuki 2019-02-21 16:34:17 PST
Created attachment 362669 [details]
Patch
Comment 57 Build Bot 2019-02-21 16:37:05 PST
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.
Comment 58 Yusuke Suzuki 2019-02-21 17:12:23 PST
Created attachment 362672 [details]
Patch
Comment 59 Build Bot 2019-02-21 17:15:29 PST
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.
Comment 60 Yusuke Suzuki 2019-02-21 17:45:32 PST
Created attachment 362676 [details]
Patch
Comment 61 Build Bot 2019-02-21 17:47:48 PST
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.
Comment 62 Yusuke Suzuki 2019-02-21 19:31:50 PST
Created attachment 362686 [details]
Patch
Comment 63 Build Bot 2019-02-21 19:35:27 PST
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 64 Yusuke Suzuki 2019-02-21 20:17:36 PST
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 65 Yusuke Suzuki 2019-02-22 14:44:14 PST
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 66 Saam Barati 2019-02-22 19:05:15 PST
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 67 Yusuke Suzuki 2019-02-22 21:14:37 PST
Comment on attachment 362686 [details]
Patch

Thanks! I'm now reflecting your comments on my patch :D
Comment 68 Yusuke Suzuki 2019-02-22 21:31:48 PST
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.
Comment 69 Yusuke Suzuki 2019-02-22 22:53:53 PST
Created attachment 362816 [details]
Patch
Comment 70 Yusuke Suzuki 2019-02-22 22:54:40 PST
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.
Comment 71 Yusuke Suzuki 2019-02-22 23:01:54 PST
Created attachment 362817 [details]
Patch
Comment 72 Build Bot 2019-02-22 23:04:30 PST
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.
Comment 73 Yusuke Suzuki 2019-02-26 21:17:00 PST
Created attachment 363064 [details]
Patch
Comment 74 Build Bot 2019-02-26 21:19:59 PST
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.
Comment 75 Yusuke Suzuki 2019-02-26 23:30:23 PST
Created attachment 363076 [details]
Patch
Comment 76 Yusuke Suzuki 2019-02-26 23:57:53 PST
Created attachment 363077 [details]
Patch
Comment 77 Build Bot 2019-02-27 00:00:48 PST
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 78 Build Bot 2019-02-27 01:29:06 PST
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
Comment 79 Build Bot 2019-02-27 01:29:09 PST
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
Comment 80 Yusuke Suzuki 2019-02-27 10:58:52 PST
(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 81 Yusuke Suzuki 2019-02-28 12:25:33 PST
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 82 Saam Barati 2019-02-28 12:37:39 PST
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 83 Yusuke Suzuki 2019-02-28 15:16:08 PST
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.
Comment 84 Yusuke Suzuki 2019-02-28 17:56:11 PST
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.
Comment 85 Yusuke Suzuki 2019-02-28 19:12:08 PST
Created attachment 363288 [details]
Patch for landing
Comment 86 Yusuke Suzuki 2019-02-28 19:13:40 PST
Committed r242252: <https://trac.webkit.org/changeset/242252>
Comment 87 Radar WebKit Bug Importer 2019-02-28 19:14:28 PST
<rdar://problem/48498246>
Comment 88 Saam Barati 2019-02-28 19:54:38 PST
(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?
Comment 89 Ryan Haddad 2019-02-28 21:15:11 PST
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
Comment 90 Yusuke Suzuki 2019-02-28 22:33:27 PST
(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.
Comment 91 Yusuke Suzuki 2019-02-28 23:06:08 PST
(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.
Comment 92 Yusuke Suzuki 2019-03-01 00:41:33 PST
Committed r242260: <https://trac.webkit.org/changeset/242260>
Comment 93 Shawn Roberts 2019-03-05 10:52:07 PST
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
Comment 94 Yusuke Suzuki 2019-03-05 10:54:19 PST
(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.
Comment 95 Yusuke Suzuki 2019-03-05 11:10:45 PST
(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.