Bug 179934 - GC constraint solving should be parallel
Summary: GC constraint solving should be parallel
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks: 166826 180906
  Show dependency treegraph
 
Reported: 2017-11-21 20:47 PST by Filip Pizlo
Modified: 2017-12-16 07:04 PST (History)
13 users (show)

See Also:


Attachments
it's a start (34.67 KB, patch)
2017-11-21 20:48 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
I think it's written (39.92 KB, patch)
2017-11-22 10:19 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (55.53 KB, patch)
2017-11-23 17:13 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
I'm starting to like the algorithm (66.46 KB, patch)
2017-11-23 19:50 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it can reliably run splay (71.01 KB, patch)
2017-11-24 14:23 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
taking a shot at a better load balancer (76.09 KB, patch)
2017-11-25 11:55 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (76.88 KB, patch)
2017-11-25 15:51 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit nicer (83.48 KB, patch)
2017-11-25 16:08 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
made it more testable (113.47 KB, patch)
2017-11-25 18:35 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
neutral on splay-latency (113.83 KB, patch)
2017-11-25 19:02 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
with parallel Wcoc constraint (128.35 KB, patch)
2017-11-27 20:35 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (144.21 KB, patch)
2017-11-28 11:40 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
a bit more (166.62 KB, patch)
2017-11-28 14:29 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
fix some compile errors (167.45 KB, patch)
2017-11-28 14:58 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
ConcurrentPtrHashSet unit tests pass (170.50 KB, patch)
2017-11-29 21:33 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
maybe 0.5% faster on Speedometer (170.78 KB, patch)
2017-11-30 10:00 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (180.92 KB, patch)
2017-11-30 11:15 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
reduced reliance on nested classes (210.66 KB, patch)
2017-12-01 09:30 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it compiles again (210.77 KB, patch)
2017-12-01 10:46 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (229.13 KB, patch)
2017-12-01 11:39 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
rebased (227.65 KB, patch)
2017-12-01 13:36 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (227.68 KB, patch)
2017-12-01 14:08 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (232.53 KB, patch)
2017-12-01 14:34 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
latest (244.94 KB, patch)
2017-12-01 22:25 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
back to progression on speedo (246.15 KB, patch)
2017-12-01 23:53 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
updated changelog (245.70 KB, patch)
2017-12-02 00:19 PST, Filip Pizlo
ews: commit-queue-
Details | Formatted Diff | Diff
more solid speedup (253.64 KB, patch)
2017-12-02 09:02 PST, Filip Pizlo
ews: commit-queue-
Details | Formatted Diff | Diff
I think I fixed the crash bug (266.53 KB, patch)
2017-12-02 15:14 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
trying to fix the regression from my bugfix (281.89 KB, patch)
2017-12-02 16:44 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
an even better attempt (298.63 KB, patch)
2017-12-02 18:35 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (298.63 KB, patch)
2017-12-02 19:31 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (298.63 KB, patch)
2017-12-02 20:10 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
more (304.93 KB, patch)
2017-12-03 11:55 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (303.36 KB, patch)
2017-12-03 18:49 PST, Filip Pizlo
jfbastien: review+
Details | Formatted Diff | Diff
patch for landing (304.34 KB, patch)
2017-12-04 21:49 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-11-21 20:47:15 PST
Patch forthcoming
Comment 1 Filip Pizlo 2017-11-21 20:48:35 PST
Created attachment 327441 [details]
it's a start

I think that I have a plan for how to make this work.  We will run constraints in parallel.  Additionally, constraints will be able to do their own parallel work.
Comment 2 Filip Pizlo 2017-11-22 10:19:26 PST
Created attachment 327460 [details]
I think it's written
Comment 3 Filip Pizlo 2017-11-23 17:13:22 PST
Created attachment 327517 [details]
more
Comment 4 Filip Pizlo 2017-11-23 19:50:52 PST
Created attachment 327521 [details]
I'm starting to like the algorithm
Comment 5 Build Bot 2017-11-24 05:53:58 PST
Attachment 327521 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:61:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:63:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:104:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.cpp:116:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.cpp:70:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.cpp:116:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.cpp:117:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.cpp:126:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.cpp:127:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.cpp:128:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 13 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Filip Pizlo 2017-11-24 14:23:25 PST
Created attachment 327556 [details]
it can reliably run splay

Looks like it may be a regression though.  There is a lot about this algorithm that is imperfect.
Comment 7 Filip Pizlo 2017-11-24 14:50:01 PST
(In reply to Filip Pizlo from comment #6)
> Created attachment 327556 [details]
> it can reliably run splay
> 
> Looks like it may be a regression though.  There is a lot about this
> algorithm that is imperfect.

I think that the regression is because of those times when the old solver would just run a single constraint to produce forward progress.  It's even good at picking which constraint to run.  Now we will run ~8 constraints on an 8 core machine.  The first one will be the only one we need to run.

The parallelism helps the most when we know we want to run multiple constraints.  I need to figure out a way of turning on the parallelism in exactly those cases where it is profitable.
Comment 8 Filip Pizlo 2017-11-25 11:55:43 PST
Created attachment 327576 [details]
taking a shot at a better load balancer
Comment 9 Filip Pizlo 2017-11-25 15:51:45 PST
Created attachment 327577 [details]
more
Comment 10 Filip Pizlo 2017-11-25 16:08:49 PST
Created attachment 327578 [details]
a bit nicer

Put each new class into its own file
Comment 11 Filip Pizlo 2017-11-25 18:35:32 PST
Created attachment 327579 [details]
made it more testable

This finally adds "splay-latency" to the "Octane" in run-jsc-benchmarks.  What that harness calls Octane was never Octane, so I went ahead and added the JetStream-style splay-latency, in the sense that it's the average above the 99.5% percentile.  However, it runs significantly longer and so should detect tinier changes in GC responsiveness.

The previous version of the patch is a 10% regression on this test.

The hope with this patch is that it gets to neutral on splay-latency and provides a speed-up on DOM-heavy code.  It's not going to do either of those things yet, but being within shooting distance of neutral on splay-latency is pretty good.
Comment 12 Filip Pizlo 2017-11-25 19:02:58 PST
Created attachment 327581 [details]
neutral on splay-latency
Comment 13 Filip Pizlo 2017-11-27 20:34:24 PST
(In reply to Filip Pizlo from comment #12)
> Created attachment 327581 [details]
> neutral on splay-latency

It was also neutral on Speedometer.  But I also confirmed that Speedometer spends a decent amount of time in constraint solving per GC cycle.  I don't know if that means a significant amount of time overall.  I think that the parallelism in this patch did not help because Speedometer GC cycles spend most of their time in one of the DOM constraints.  I think there are just two of them.

So, right now I'm working on making those constraints parallel.
Comment 14 Filip Pizlo 2017-11-27 20:35:08 PST
Created attachment 327723 [details]
with parallel Wcoc constraint

I haven't even compiled this yet.  I'm just starting to understand how to introduce parallelism into the innards of constraints.
Comment 15 Filip Pizlo 2017-11-28 11:40:26 PST
Created attachment 327766 [details]
more
Comment 16 Filip Pizlo 2017-11-28 14:29:18 PST
Created attachment 327788 [details]
a bit more

To make this make any sense, I needed to improve how the opaque root set works.
Comment 17 Filip Pizlo 2017-11-28 14:58:18 PST
Created attachment 327794 [details]
fix some compile errors
Comment 18 Build Bot 2017-11-29 03:52:26 PST
Attachment 327794 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:665:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/SharedTask.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:46:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:47:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/ForwardingHeaders/heap/SimpleMarkingConstraint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WTF/wtf/ConcurrentPtrHashSet.h:154:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.h:50:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:60:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:62:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 16 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Filip Pizlo 2017-11-29 21:33:10 PST
Created attachment 327951 [details]
ConcurrentPtrHashSet unit tests pass
Comment 20 Filip Pizlo 2017-11-30 10:00:36 PST
Created attachment 327984 [details]
maybe 0.5% faster on Speedometer

I haven't even picked the EventTargetDataMap lock.  I haven't even made WeakSet processing parallel.

I think that I need to do those things for this patch to make sense.  I don't want to run the risk that the EventTargetDataMap lock contention is a regression on some systems.

The WeakSet part is just so trivial that it would be weird not to do it.
Comment 21 Filip Pizlo 2017-11-30 11:15:53 PST
Created attachment 327996 [details]
more

Removed the need to lock the event target data map lock.
Comment 22 Filip Pizlo 2017-12-01 09:30:01 PST
Created attachment 328103 [details]
reduced reliance on nested classes

This patch still introduces a lot of new uses of nested classes, but earlier versions used them a bit too much.  I think that once a class is so big that it doesn't fit on the screen, it should probably be in a separate file.
Comment 23 Filip Pizlo 2017-12-01 10:46:29 PST
Created attachment 328121 [details]
it compiles again
Comment 24 Filip Pizlo 2017-12-01 11:39:32 PST
Created attachment 328129 [details]
the patch
Comment 25 Mark Lam 2017-12-01 11:47:47 PST
EWS doesn't like the patch.  Can you rebase?
Comment 26 Filip Pizlo 2017-12-01 12:58:25 PST
(In reply to Mark Lam from comment #25)
> EWS doesn't like the patch.  Can you rebase?

Yeah!  Also, I didn't mean to mark it r? yet.
Comment 27 Filip Pizlo 2017-12-01 12:58:40 PST
Comment on attachment 328129 [details]
the patch

This is not yet ready for review.
Comment 28 Filip Pizlo 2017-12-01 13:36:36 PST
Created attachment 328149 [details]
rebased
Comment 29 Filip Pizlo 2017-12-01 14:08:21 PST
Created attachment 328156 [details]
the patch
Comment 30 Build Bot 2017-12-01 14:11:17 PST
Attachment 328156 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:662:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/SharedTask.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:60:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:92:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:95:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:125:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:128:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:153:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:156:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:158:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:163:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:167:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:169:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:170:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:172:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:183:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:191:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:192:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:194:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:199:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:202:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:204:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:206:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:211:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:214:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:218:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:220:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:230:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:46:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:47:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/ForwardingHeaders/heap/SimpleMarkingConstraint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WTF/wtf/ConcurrentPtrHashSet.h:157:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:55:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:56:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:68:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:74:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/StackShotProfiler.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StackShotProfiler.h:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.h:50:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:61:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:63:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 49 in 68 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Filip Pizlo 2017-12-01 14:34:49 PST
Created attachment 328158 [details]
the patch

Made a few stylistic tweaks.
Comment 32 Build Bot 2017-12-01 14:36:53 PST
Attachment 328158 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:662:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/SharedTask.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:45:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:46:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:47:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/ForwardingHeaders/heap/SimpleMarkingConstraint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WTF/wtf/ConcurrentPtrHashSet.h:157:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:55:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:56:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:68:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:74:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/StackShotProfiler.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StackShotProfiler.h:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:60:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:92:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:95:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:125:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:128:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:153:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:156:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:158:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:163:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:167:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:169:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:170:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:172:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:183:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:191:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:192:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:194:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:199:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:202:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:204:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:206:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:211:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:214:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:218:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:220:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:230:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.h:53:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:62:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:64:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 49 in 69 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 33 Filip Pizlo 2017-12-01 22:25:26 PST
Created attachment 328213 [details]
latest

The previous patch was a slight regression on JetStream because of how we synchronized the worker threads.  This fixes that, but it regresses Speedometer.  It seems that the solver sometimes gets stuck in Speedometer now; I'm investigating why.
Comment 34 Filip Pizlo 2017-12-01 23:53:34 PST
Created attachment 328218 [details]
back to progression on speedo

I think
Comment 35 Filip Pizlo 2017-12-02 00:19:40 PST
Created attachment 328221 [details]
updated changelog

I wasn't able to measure the progression on Speedometer, but I think that might be just measurement noise.

I confirmed that Speedometer sees a nice progression if I set concurrency to Sequential for the conservative root constraint. This version of the patch does not do that, and instead relies on some other defenses against weird behavior in that constraint. I'm pretty sure I still have more work to do. For example, I think that ConservativeRoots will scan the main thread twice if it's invoked from a marking thread.
Comment 36 Build Bot 2017-12-02 04:31:09 PST
Attachment 328221 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:282:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/SharedTask.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/ForwardingHeaders/heap/SimpleMarkingConstraint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WTF/wtf/ConcurrentPtrHashSet.h:157:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:74:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/StackShotProfiler.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StackShotProfiler.h:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:66:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:109:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:163:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:173:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:185:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:187:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:221:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:223:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:225:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:230:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:233:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:237:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:239:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:251:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:64:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:66:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 30 in 72 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 37 Build Bot 2017-12-02 07:22:55 PST
Comment on attachment 328221 [details]
updated changelog

Attachment 328221 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/5466603

New failing tests:
microbenchmarks/v8-regexp-search.js.no-cjit-collect-continuously
stress/v8-regexp-strict.js.ftl-eager
microbenchmarks/direct-construct.js.ftl-eager
stress/array-concat-spread-proxy-exception-check.js.dfg-eager-no-cjit-validate
stress/value-add-on-double-array-with-holes.js.no-cjit-collect-continuously
v8-v6/v8-regexp.js.ftl-eager
stress/proxy-all-the-parameters.js.dfg-eager-no-cjit-validate
stress/spread-calling.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.dfg-maximal-flush-validate-no-cjit
stress/proxy-all-the-parameters.js.no-ftl
microbenchmarks/v8-regexp-search.js.dfg-eager-no-cjit-validate
stress/proxy-all-the-parameters.js.ftl-no-cjit-no-put-stack-validate
v8-v6/v8-regexp.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.no-llint
stress/value-add-on-double-array-with-holes.js.ftl-eager-no-cjit
microbenchmarks/string-add-constant-folding.js.ftl-eager
stress/proxy-all-the-parameters.js.ftl-eager-no-cjit-b3o1
stress/array-slice-jettison-on-constructor-change.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.ftl-eager
v8-v6/v8-regexp.js.no-cjit-collect-continuously
stress/spread-capture-rest.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.ftl-no-cjit-b3o1
stress/proxy-all-the-parameters.js.ftl-no-cjit-small-pool
v8-v6/v8-regexp.js.dfg-eager
microbenchmarks/dont-confuse-structures-from-different-executable-as-poly-proto.js.dfg-eager-no-cjit-validate
microbenchmarks/rest-parameter-allocation-elimination.js.ftl-eager-no-cjit
stress/v8-regexp-strict.js.dfg-eager
stress/proxy-all-the-parameters.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/v8-regexp-search.js.dfg-eager
stress/v8-regexp-strict.js.no-cjit-collect-continuously
stress/proxy-all-the-parameters.js.default
microbenchmarks/v8-regexp-search.js.ftl-eager
stress/proxy-all-the-parameters.js.ftl-no-cjit-no-inline-validate
stress/array-slice-osr-exit.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.ftl-eager-no-cjit
stress/put-direct-index-broken-2.js.dfg-eager
stress/value-add-on-double-array-with-holes.js.dfg-eager
stress/v8-regexp-strict.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.dfg-eager
stress/array-slice-osr-exit-2.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.no-cjit-collect-continuously
v8-v6/v8-regexp.js.dfg-eager-no-cjit-validate
stress/proxy-all-the-parameters.js.no-cjit-validate-phases
Comment 38 Filip Pizlo 2017-12-02 09:02:13 PST
Created attachment 328233 [details]
more solid speedup
Comment 39 Build Bot 2017-12-02 09:04:35 PST
Attachment 328233 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/heap/SlotVisitor.cpp:282:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/SharedTask.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/ForwardingHeaders/heap/SimpleMarkingConstraint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WTF/wtf/ConcurrentPtrHashSet.h:157:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:74:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/StackShotProfiler.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StackShotProfiler.h:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:112:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:166:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:176:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:190:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:224:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:226:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:228:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:233:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:236:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:240:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:242:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:254:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:64:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:66:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 30 in 75 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Filip Pizlo 2017-12-02 09:43:32 PST
(In reply to Filip Pizlo from comment #38)
> Created attachment 328233 [details]
> more solid speedup

This totally has crashes when collecting continuously.  Investigating...
Comment 41 Build Bot 2017-12-02 09:56:16 PST
Comment on attachment 328233 [details]
more solid speedup

Attachment 328233 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/5467576

New failing tests:
stress/call-apply-exponential-bytecode-size.js.dfg-eager-no-cjit-validate
stress/v8-regexp-strict.js.ftl-eager
stress/spread-multi-layers.js.ftl-eager
stress/exit-after-int52-to-value.js.ftl-eager-no-cjit
stress/v8-regexp-strict.js.no-cjit-collect-continuously
stress/proxy-all-the-parameters.js.dfg-eager-no-cjit-validate
stress/spread-calling.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.default
microbenchmarks/call-spread-call.js.ftl-eager
stress/proxy-all-the-parameters.js.no-ftl
v8-v6/v8-regexp.js.no-cjit-collect-continuously
v8-v6/v8-regexp.js.ftl-eager-no-cjit
basic-tests.yaml/stress-test.js.ftl-eager-no-cjit
stress/value-add-on-double-array-with-holes.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.ftl-eager-no-cjit-b3o1
stress/array-slice-jettison-on-constructor-change.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.ftl-eager
v8-v6/v8-regexp.js.ftl-eager
airjs-tests.yaml/stress-test.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.ftl-no-cjit-b3o1
stress/proxy-all-the-parameters.js.ftl-no-cjit-small-pool
microbenchmarks/rest-parameter-allocation-elimination.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.ftl-no-cjit-no-inline-validate
stress/proxy-all-the-parameters.js.ftl-no-cjit-validate-sampling-profiler
stress/v8-regexp-strict.js.dfg-eager-no-cjit-validate
cdjs-tests.yaml/main.js.ftl-no-cjit
stress/proxy-all-the-parameters.js.dfg-eager
stress/proxy-all-the-parameters.js.dfg-maximal-flush-validate-no-cjit
stress/value-add-on-double-array-with-holes.js.ftl-eager
stress/proxy-all-the-parameters.js.no-llint
stress/array-slice-osr-exit.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.ftl-eager-no-cjit
stress/generator-fib-ftl-and-string.js.ftl-eager-no-cjit
stress/value-add-on-double-array-with-holes.js.dfg-eager
stress/proxy-all-the-parameters.js.ftl-no-cjit-no-put-stack-validate
stress/array-slice-osr-exit-2.js.ftl-eager-no-cjit
stress/proxy-all-the-parameters.js.no-cjit-collect-continuously
microbenchmarks/direct-construct-arity-mismatch.js.ftl-eager
microbenchmarks/rest-parameter-allocation-elimination.js.ftl-eager
v8-v6/v8-regexp.js.dfg-eager-no-cjit-validate
stress/proxy-all-the-parameters.js.no-cjit-validate-phases
Comment 42 Filip Pizlo 2017-12-02 15:14:22 PST
Created attachment 328258 [details]
I think I fixed the crash bug
Comment 43 Filip Pizlo 2017-12-02 15:14:49 PST
(In reply to Filip Pizlo from comment #42)
> Created attachment 328258 [details]
> I think I fixed the crash bug

Now I have to check if the perf is still there.
Comment 44 Build Bot 2017-12-02 15:17:43 PST
Attachment 328258 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:380:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/ForwardingHeaders/heap/SimpleMarkingConstraint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
ERROR: Source/WTF/wtf/ConcurrentPtrHashSet.h:157:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:74:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/StackShotProfiler.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StackShotProfiler.h:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/SharedTask.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:112:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:166:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:176:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:190:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:224:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:226:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:228:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:233:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:236:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:240:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:242:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:254:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:64:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:66:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 29 in 81 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 45 Filip Pizlo 2017-12-02 16:44:43 PST
Created attachment 328264 [details]
trying to fix the regression from my bugfix

It looks as though acquiring a lock in isLive is not great.  I can fix that by using a StampedLock to protect the MarkedBlock data structures.  This is an attempt...
Comment 46 Filip Pizlo 2017-12-02 18:35:37 PST
Created attachment 328274 [details]
an even better attempt

StampedLock combined with dependency chaining FTW.
Comment 47 Filip Pizlo 2017-12-02 19:31:01 PST
Created attachment 328276 [details]
the patch

Still a speed-up on Speedometer.  No longer crashing.

Need to test ARM...
Comment 48 Build Bot 2017-12-02 19:34:42 PST
Attachment 328276 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:408:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:410:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:415:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:416:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:408:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:410:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:415:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:416:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:74:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/StackShotProfiler.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StackShotProfiler.h:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/SharedTask.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:64:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:66:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ConcurrentPtrHashSet.h:157:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:112:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:166:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:176:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:190:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:224:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:226:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:228:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:233:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:236:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:240:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:242:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:254:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/ForwardingHeaders/heap/SimpleMarkingConstraint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 35 in 86 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 49 Filip Pizlo 2017-12-02 20:10:18 PST
Created attachment 328280 [details]
the patch
Comment 50 Build Bot 2017-12-02 20:13:16 PST
Attachment 328280 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:408:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:410:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:415:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:416:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:408:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:410:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:415:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:416:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:74:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/StackShotProfiler.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StackShotProfiler.h:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/SharedTask.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:64:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:66:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ConcurrentPtrHashSet.h:157:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:112:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:166:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:176:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:190:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:224:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:226:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:228:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:233:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:236:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:240:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:242:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:254:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/ForwardingHeaders/heap/SimpleMarkingConstraint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 35 in 86 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 51 Filip Pizlo 2017-12-03 11:55:19 PST
Created attachment 328298 [details]
more

Cleaned up some things.
Comment 52 Build Bot 2017-12-03 11:57:52 PST
Attachment 328298 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:396:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:398:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:408:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:396:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:398:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:408:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:74:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/StackShotProfiler.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StackShotProfiler.h:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/SharedTask.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:64:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:66:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ConcurrentPtrHashSet.h:157:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkedBlock.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:112:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:166:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:176:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:190:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:224:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:226:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:228:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:233:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:236:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:240:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:242:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:254:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/ForwardingHeaders/heap/SimpleMarkingConstraint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 34 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 53 Filip Pizlo 2017-12-03 18:49:07 PST
Created attachment 328317 [details]
the patch
Comment 54 Build Bot 2017-12-03 18:51:11 PST
Attachment 328317 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:396:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:398:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:408:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:396:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:398:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:408:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:74:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/StackShotProfiler.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StackShotProfiler.h:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/SharedTask.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:64:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:66:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ConcurrentPtrHashSet.h:157:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkedBlock.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:112:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:166:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:176:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:190:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:224:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:226:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:228:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:233:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:236:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:240:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:242:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:254:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/ForwardingHeaders/heap/SimpleMarkingConstraint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 34 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 55 Filip Pizlo 2017-12-04 09:48:44 PST
(In reply to Filip Pizlo from comment #47)
> Created attachment 328276 [details]
> the patch
> 
> Still a speed-up on Speedometer.  No longer crashing.
> 
> Need to test ARM...

Seems to work fine on ARM.
Comment 56 JF Bastien 2017-12-04 11:36:40 PST
Comment on attachment 328317 [details]
the patch

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

I know the code usually doesn't, but it would be nice to use `final` where you can.

I haven't really wrapped my mind around how the GC races are correct, so modulo that the code looks good module the union UB comments. r=me with that fixed.

> Source/JavaScriptCore/heap/ConstraintConcurrency.h:41
> +inline void printInternal(PrintStream& out, JSC::ConstraintConcurrency concurrency)

I think this can be in the JSC namespace above, and be found through ADL?

> Source/JavaScriptCore/heap/ConstraintParallelism.h:41
> +inline void printInternal(PrintStream& out, JSC::ConstraintParallelism parallelism)

Ditto.

> Source/JavaScriptCore/heap/Heap.cpp:618
> +            dataLog("FATAL: Visitor ", RawPointer(&visitor), " is not empty!\n");

I don't know what to do if this fires. Could you make it more helpful? Also doesn't seem fatal as the message implies, since it doesn't actually kill anything.

> Source/JavaScriptCore/heap/Heap.cpp:2599
> +        [this, lastVersion = static_cast<uint64_t>(0)] (SlotVisitor& slotVisitor) mutable {

Neat. I never use mutable lambdas, I totally should.

> Source/JavaScriptCore/heap/Heap.cpp:2613
> +            lastVersion = m_phaseVersion;

m_phaseVersion can be modified concurrently to this executing, right? Is it problematic if this read of m_phaseVersion is the same / different from the one at the top of the function? Seems like not but I want to make sure you don't need something like READ_ONCE.

> Source/JavaScriptCore/heap/Heap.cpp:2865
> +    }

I don't understand why you need to lock after setting and running the bonus task, and why you're waiting for the refcount this way. Could you add a comment?

> Source/JavaScriptCore/heap/HeapInlines.h:269
> +        func(*slotVisitor);

Is there some Science or Heuristic behind the order of the func() calls?

> Source/JavaScriptCore/heap/MarkStackMergingConstraint.h:35
> +    ~MarkStackMergingConstraint();

override

> Source/JavaScriptCore/heap/MarkingConstraint.cpp:34
> +static constexpr bool verbose = false;

With unified builds this needs to be localized, no?

> Source/JavaScriptCore/heap/MarkingConstraint.cpp:71
> +    return 0;

Why is that correct?

> Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:126
> +    if (order[index]->quickWorkEstimate(m_mainVisitor)) {

I find testing floating-point like this weird. Seems like you want quickWorkEstimate > 0. here.

> Source/JavaScriptCore/heap/SimpleMarkingConstraint.h:44
> +    JS_EXPORT_PRIVATE ~SimpleMarkingConstraint();

override

> Source/JavaScriptCore/heap/SlotVisitor.h:97
> +    bool addOpaqueRoot(void*); // Returns true if the root was new.

Want to use a named enum class instead of a comment?

> Source/WTF/wtf/Atomics.h:358
> +    return opaqueMixture(arguments...) + u.copy;

Your usage of union is UB. You want to operate through bitwise_cast or memcpy / memset.

> Source/WTF/wtf/Atomics.h:371
> +    // phantom depenendency.

Typo "dependency".

> Source/WTF/wtf/ConcurrentPtrHashSet.cpp:82
> +                return add(ptr);

You can exhaust the stack here if you're having a real bad time?

> Source/WTF/wtf/ConcurrentPtrHashSet.h:99
> +        Atomic<void*> array[1];

We can't use the C extension `Atomic<void*> array[];` ?

> Source/WTF/wtf/ConcurrentPtrHashSet.h:119
> +        return u.ptr;

As above, you want memset here, then u.value = value.

> Source/WTF/wtf/CountingLock.h:55
> +// The latter is important for us because some GC paths are known to be sensitive to fences on ARM.

This is neat. As we discussed on IRC in some cases seqlock might be more efficient. I have a  version here:
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0690r0.html#seqlock
Comment 57 Filip Pizlo 2017-12-04 21:35:28 PST
(In reply to JF Bastien from comment #56)
> Comment on attachment 328317 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328317&action=review
> 
> I know the code usually doesn't, but it would be nice to use `final` where
> you can.
> 
> I haven't really wrapped my mind around how the GC races are correct, so
> modulo that the code looks good module the union UB comments. r=me with that
> fixed.
> 
> > Source/JavaScriptCore/heap/ConstraintConcurrency.h:41
> > +inline void printInternal(PrintStream& out, JSC::ConstraintConcurrency concurrency)
> 
> I think this can be in the JSC namespace above, and be found through ADL?

What is ADL?  Is that the thing that works for operators in different namespaces?

> 
> > Source/JavaScriptCore/heap/ConstraintParallelism.h:41
> > +inline void printInternal(PrintStream& out, JSC::ConstraintParallelism parallelism)
> 
> Ditto.
> 
> > Source/JavaScriptCore/heap/Heap.cpp:618
> > +            dataLog("FATAL: Visitor ", RawPointer(&visitor), " is not empty!\n");
> 
> I don't know what to do if this fires. Could you make it more helpful? Also
> doesn't seem fatal as the message implies, since it doesn't actually kill
> anything.

It's sets ok to false, and we RELEASE_ASSERT(ok) below.  This error only comes up when you break the constraint solver, and if you touched that code, you'd know about visitors.

I suppose it would be different if we were seeing this in the wild.  An earlier version of this patch did, before I added some of the changes to how SlotVisitor::isEmpty and didReachTermination work.

> 
> > Source/JavaScriptCore/heap/Heap.cpp:2599
> > +        [this, lastVersion = static_cast<uint64_t>(0)] (SlotVisitor& slotVisitor) mutable {
> 
> Neat. I never use mutable lambdas, I totally should.

Yeah.  I find that they are useful up to exactly 1 of these variables.  That's why there are so many nested classes in this patch.  I wrote a couple of those as lambdas initially, and it did not look very clear.

> 
> > Source/JavaScriptCore/heap/Heap.cpp:2613
> > +            lastVersion = m_phaseVersion;
> 
> m_phaseVersion can be modified concurrently to this executing, right? Is it
> problematic if this read of m_phaseVersion is the same / different from the
> one at the top of the function? Seems like not but I want to make sure you
> don't need something like READ_ONCE.

m_phaseVersion is modified by the GC before/after any of the parallel constrain solving starts.  Therefore, the constraints are allowed to read it.

There is a ton of stuff in the GC that works this way... locklessness because reasons.

> 
> > Source/JavaScriptCore/heap/Heap.cpp:2865
> > +    }
> 
> I don't understand why you need to lock after setting and running the bonus
> task, and why you're waiting for the refcount this way. Could you add a
> comment?

Ah!  Because the constraint solver expects termination of this function to imply termination of the task in all threads.  I will add a comment.

> 
> > Source/JavaScriptCore/heap/HeapInlines.h:269
> > +        func(*slotVisitor);
> 
> Is there some Science or Heuristic behind the order of the func() calls?

Nope.

> 
> > Source/JavaScriptCore/heap/MarkStackMergingConstraint.h:35
> > +    ~MarkStackMergingConstraint();
> 
> override

Why?  I didn't know we used override on destructors and there are many places where we don't.

> 
> > Source/JavaScriptCore/heap/MarkingConstraint.cpp:34
> > +static constexpr bool verbose = false;
> 
> With unified builds this needs to be localized, no?

Good point.  I renamed it to verboseMarkingConstraint.

> 
> > Source/JavaScriptCore/heap/MarkingConstraint.cpp:71
> > +    return 0;
> 
> Why is that correct?

Because quickWorkEstimate and workEstimate are regarded by callers as guesses.  They tell the constraint solver how much work you think you might create.  But the very nature of most constraints is that they cannot possibly know until they try.

workEstimate estimates by just remembering how much work the constraint generated last time, and adding that to quickWorkEstimate.

quickWorkEstimate is meant to be overridden by those constraints that have some way of predicting how much work they have.  There is one constraint right know that can do this - the MarkStackMergingConstraint.

By the way, this isn't new to the patch - it's just that once I turned MarkingConstraint into a base class with subclasses that override virtual methods, I no longer needed it to be a dumping ground for WTF::Functions.  So SimpleMarkingConstraint is for the case where you can sum yourself up as one lambda (the execute lambda), and everyone else - like those who have a quick work estimate - must subclass instead.  Hence, the quickWorkEstimate function stops being a WTF::Function and becomes a virtual function.

> 
> > Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:126
> > +    if (order[index]->quickWorkEstimate(m_mainVisitor)) {
> 
> I find testing floating-point like this weird. Seems like you want
> quickWorkEstimate > 0. here.

Good point.  I will change it.

> 
> > Source/JavaScriptCore/heap/SimpleMarkingConstraint.h:44
> > +    JS_EXPORT_PRIVATE ~SimpleMarkingConstraint();
> 
> override
> 
> > Source/JavaScriptCore/heap/SlotVisitor.h:97
> > +    bool addOpaqueRoot(void*); // Returns true if the root was new.
> 
> Want to use a named enum class instead of a comment?

True-if-new is so common for add methods in WebKit.  It's the default.  I just like to add a comment anyway since this is a rarely-seen add method.  I want callers that just be reassured that the bool means what it usually means.

> 
> > Source/WTF/wtf/Atomics.h:358
> > +    return opaqueMixture(arguments...) + u.copy;
> 
> Your usage of union is UB. You want to operate through bitwise_cast or
> memcpy / memset.

I think this is an area of union semantics that is so widely relied-upon that we are OK.

I checked a bunch of compilers.  They all generate what I want here.  Specifically, if the argument type is less than 4 bytes, they zero-extend it.

This kind of use of unions is a staple of binary format transformation code.  For as long as I've written that kind of code I've heard scary stories about what the compiler might do.  I think it's mostly because it's very difficult to specify what the compiler must really do, so compiler writers and compiler users have a number of implicit understandings in areas of the semantics that are hard to write down.  It's important for compiler writers to avoid turning this into an opportunity to "steal" semantics from users and turn them into performance opportunities.

> 
> > Source/WTF/wtf/Atomics.h:371
> > +    // phantom depenendency.
> 
> Typo "dependency".

Fixed.  I think.  I lose track of how many e's and n's there are in that word.

> 
> > Source/WTF/wtf/ConcurrentPtrHashSet.cpp:82
> > +                return add(ptr);
> 
> You can exhaust the stack here if you're having a real bad time?

This is pretty sure to get TCO'd, but even if it isn't, this loop-around happens on resize and resize is exponential, so the number of recursions here is log_2(set size).  That won't blow the stack.

> 
> > Source/WTF/wtf/ConcurrentPtrHashSet.h:99
> > +        Atomic<void*> array[1];
> 
> We can't use the C extension `Atomic<void*> array[];` ?

This way is habit. :-)

> 
> > Source/WTF/wtf/ConcurrentPtrHashSet.h:119
> > +        return u.ptr;
> 
> As above, you want memset here, then u.value = value.
> 
> > Source/WTF/wtf/CountingLock.h:55
> > +// The latter is important for us because some GC paths are known to be sensitive to fences on ARM.
> 
> This is neat. As we discussed on IRC in some cases seqlock might be more
> efficient. I have a  version here:
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0690r0.html#seqlock

The way to make mine more efficient is to get rid of the branch on tryOptimisticRead.  We can do that by ^ing the loaded lock state with the mask in the tryOptimisticRead, rather than doing getStamp.

Or we could count on unlock.  I didn't do that because I wasn't sure if that would work with our fair unlock path.

I'll write a FIXME about what we could do here.
Comment 58 Filip Pizlo 2017-12-04 21:49:56 PST
Created attachment 328438 [details]
patch for landing
Comment 59 JF Bastien 2017-12-04 22:25:38 PST
> > > Source/JavaScriptCore/heap/ConstraintConcurrency.h:41
> > > +inline void printInternal(PrintStream& out, JSC::ConstraintConcurrency concurrency)
> > 
> > I think this can be in the JSC namespace above, and be found through ADL?
> 
> What is ADL?  Is that the thing that works for operators in different
> namespaces?

All functions, not just operators, will be looked up in the parameter's namespace when called unqualified. So boo(bar::baz, foo::doo) will lookup boo locally as well as in bar and in foo.


> > > Source/WTF/wtf/ConcurrentPtrHashSet.h:99
> > > +        Atomic<void*> array[1];
> > 
> > We can't use the C extension `Atomic<void*> array[];` ?
> 
> This way is habit. :-)

Yeah but C. You can't cling to C union semantics and not fully embrace C!
Comment 60 Build Bot 2017-12-05 01:30:50 PST
Attachment 328438 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Atomics.h:396:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:398:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:408:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:396:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:398:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Atomics.h:408:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:74:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/bindings/js/DOMGCOutputConstraint.cpp:80:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WTF/wtf/StackShotProfiler.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/StackShotProfiler.h:86:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/SharedTask.h:63:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:64:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkingConstraint.h:66:  The parameter name "visitor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/API/JSMarkingConstraintPrivate.cpp:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ConcurrentPtrHashSet.h:157:  The parameter name "table" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/heap/MarkedBlock.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:67:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:112:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:166:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:176:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:188:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:190:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:224:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:226:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:228:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:233:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:236:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:240:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:242:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/heap/MarkingConstraintSolver.cpp:254:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/ForwardingHeaders/heap/SimpleMarkingConstraint.h:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Total errors found: 34 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 61 Filip Pizlo 2017-12-05 09:54:53 PST
Landed in https://trac.webkit.org/changeset/225524/webkit
Comment 62 Radar WebKit Bug Importer 2017-12-05 09:55:32 PST
<rdar://problem/35857160>
Comment 63 Yusuke Suzuki 2017-12-15 10:13:28 PST
I see some crashes in assertMarkStacksEmpty.

stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: FATAL: Visitor 0x7f4ecc076000 is not empty!
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 1   0x7f4f13548657 WTFCrash
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 2   0x7f4f12fafb70 JSC::Heap::gatherStackRoots(JSC::ConservativeRoots&)
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 3   0x7f4f12fb91b6 JSC::Heap::runFixpointPhase(JSC::GCConductor)
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 4   0x7f4f12fb9409 JSC::Heap::runCurrentPhase(JSC::GCConductor, JSC::CurrentThreadState*)
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 5   0x7f4f12fbaf7a
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 6   0x7f4f12fc7b65 JSC::callWithCurrentThreadState(WTF::ScopedLambda<void (JSC::CurrentThreadState&)> const&)
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 7   0x7f4f12fb94d7 JSC::Heap::collectInMutatorThread()
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 8   0x7f4f12fb953c JSC::Heap::stopIfNecessarySlow(unsigned int)
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 9   0x7f4f12fb957b JSC::Heap::stopIfNecessarySlow()
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 10  0x7f4f12fb9b93 JSC::Heap::collectIfNecessaryOrDefer(JSC::GCDeferralContext*)
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 11  0x7f4f12fcd09f JSC::MarkedAllocator::allocateSlowCase(JSC::GCDeferralContext*, JSC::AllocationFailureMode)
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 12  0x41a476 JSC::jsString(JSC::VM*, WTF::String const&)
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 13  0x7f4f133fb365 JSC::operationStringProtoFuncReplaceRegExpString(JSC::ExecState*, JSC::JSString*, JSC::RegExpObject*, JSC::JSString*)
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 14  0x7f4ecea46104
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: Segmentation fault (core dumped)
stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: ERROR: Unexpected exit code: 139
stress/tail-call-profiler.js.profiler: > quit
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: FATAL: Visitor 0x7f83f9d53000 is not empty!
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 1   0x7f83fe784657 WTFCrash
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 2   0x7f83fe1ebb70 JSC::Heap::gatherStackRoots(JSC::ConservativeRoots&)
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 3   0x7f83fe1f51b6 JSC::Heap::runFixpointPhase(JSC::GCConductor)
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 4   0x7f83fe1f5409 JSC::Heap::runCurrentPhase(JSC::GCConductor, JSC::CurrentThreadState*)
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 5   0x7f83fe1f6f7a
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 6   0x7f83fe203b65 JSC::callWithCurrentThreadState(WTF::ScopedLambda<void (JSC::CurrentThreadState&)> const&)
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 7   0x7f83fe1f54d7 JSC::Heap::collectInMutatorThread()
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 8   0x7f83fe1f553c JSC::Heap::stopIfNecessarySlow(unsigned int)
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 9   0x7f83fe1f557b JSC::Heap::stopIfNecessarySlow()
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 10  0x7f83fe1f5b93 JSC::Heap::collectIfNecessaryOrDefer(JSC::GCDeferralContext*)
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 11  0x7f83fe20909f JSC::MarkedAllocator::allocateSlowCase(JSC::GCDeferralContext*, JSC::AllocationFailureMode)
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 12  0x7f83fe511913 JSC::JSValue::toStringSlowCase(JSC::ExecState*, bool) const
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 13  0x7f83fe478bb1
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: 14  0x7f83b9cffaae
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: Segmentation fault (core dumped)
stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-validate: ERROR: Unexpected exit code: 139

After investigating, I found that this assertMarkStacksEmpty is this one.

  1331         if (converged && slotVisitor.isEmpty()) {
  1332             assertMarkStacksEmpty();
  1333             return changePhase(conn, CollectorPhase::End);
  1334         }

I'm still exploring... Do you have any ideas?
Comment 64 Filip Pizlo 2017-12-15 10:20:28 PST
(In reply to Yusuke Suzuki from comment #63)
> I see some crashes in assertMarkStacksEmpty.
> 
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: FATAL: Visitor 0x7f4ecc076000
> is not empty!
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 1   0x7f4f13548657 WTFCrash
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 2   0x7f4f12fafb70
> JSC::Heap::gatherStackRoots(JSC::ConservativeRoots&)
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 3   0x7f4f12fb91b6
> JSC::Heap::runFixpointPhase(JSC::GCConductor)
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 4   0x7f4f12fb9409
> JSC::Heap::runCurrentPhase(JSC::GCConductor, JSC::CurrentThreadState*)
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 5   0x7f4f12fbaf7a
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 6   0x7f4f12fc7b65
> JSC::callWithCurrentThreadState(WTF::ScopedLambda<void
> (JSC::CurrentThreadState&)> const&)
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 7   0x7f4f12fb94d7
> JSC::Heap::collectInMutatorThread()
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 8   0x7f4f12fb953c
> JSC::Heap::stopIfNecessarySlow(unsigned int)
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 9   0x7f4f12fb957b
> JSC::Heap::stopIfNecessarySlow()
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 10  0x7f4f12fb9b93
> JSC::Heap::collectIfNecessaryOrDefer(JSC::GCDeferralContext*)
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 11  0x7f4f12fcd09f
> JSC::MarkedAllocator::allocateSlowCase(JSC::GCDeferralContext*,
> JSC::AllocationFailureMode)
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 12  0x41a476
> JSC::jsString(JSC::VM*, WTF::String const&)
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 13  0x7f4f133fb365
> JSC::operationStringProtoFuncReplaceRegExpString(JSC::ExecState*,
> JSC::JSString*, JSC::RegExpObject*, JSC::JSString*)
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: 14  0x7f4ecea46104
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: Segmentation fault (core dumped)
> stress/v8-regexp-strict.js.ftl-no-cjit-b3o1: ERROR: Unexpected exit code: 139
> stress/tail-call-profiler.js.profiler: > quit
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: FATAL: Visitor 0x7f83f9d53000 is not empty!
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 1   0x7f83fe784657 WTFCrash
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 2   0x7f83fe1ebb70
> JSC::Heap::gatherStackRoots(JSC::ConservativeRoots&)
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 3   0x7f83fe1f51b6 JSC::Heap::runFixpointPhase(JSC::GCConductor)
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 4   0x7f83fe1f5409 JSC::Heap::runCurrentPhase(JSC::GCConductor,
> JSC::CurrentThreadState*)
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 5   0x7f83fe1f6f7a
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 6   0x7f83fe203b65
> JSC::callWithCurrentThreadState(WTF::ScopedLambda<void
> (JSC::CurrentThreadState&)> const&)
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 7   0x7f83fe1f54d7 JSC::Heap::collectInMutatorThread()
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 8   0x7f83fe1f553c JSC::Heap::stopIfNecessarySlow(unsigned int)
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 9   0x7f83fe1f557b JSC::Heap::stopIfNecessarySlow()
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 10  0x7f83fe1f5b93
> JSC::Heap::collectIfNecessaryOrDefer(JSC::GCDeferralContext*)
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 11  0x7f83fe20909f
> JSC::MarkedAllocator::allocateSlowCase(JSC::GCDeferralContext*,
> JSC::AllocationFailureMode)
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 12  0x7f83fe511913 JSC::JSValue::toStringSlowCase(JSC::ExecState*,
> bool) const
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 13  0x7f83fe478bb1
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: 14  0x7f83b9cffaae
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: Segmentation fault (core dumped)
> stress/operation-in-may-have-negative-int32-int32-array.js.dfg-eager-no-cjit-
> validate: ERROR: Unexpected exit code: 139
> 
> After investigating, I found that this assertMarkStacksEmpty is this one.
> 
>   1331         if (converged && slotVisitor.isEmpty()) {
>   1332             assertMarkStacksEmpty();
>   1333             return changePhase(conn, CollectorPhase::End);
>   1334         }
> 
> I'm still exploring... Do you have any ideas?

Presumably that's a visitor for some parallel marker thread.  Figure out why we got `converged == true` with that visitor still active.  There are some other conditions (number of active visitors, etc) that are used by the GC to detect termination; those conditions should also prove that the mark stacks are empty.  But maybe I got it wrong!
Comment 65 Yusuke Suzuki 2017-12-15 12:37:37 PST
Thanks! Now investigating why it happens...
Basically, 1-6 tests fail randomly in run-javascriptcore-tests. If we disable parallel constraint solver by setting export JSC_useParallelMarkingConstraintSolver=false, we do not observe this failure.

I've a bit instrumented the code to dump more information.

FATAL: Visitor 0x7f6800a67000 is not empty!1
Visitor 0x7f6844cfe090 is not empty!0 0 1
Visitor 0x7f6844cfe120 is not empty!0 0 1
Visitor 0x7f6800ada000 is not empty!0 0 1
Visitor 0x7f6800ad1000 is not empty!0 0 1
Visitor 0x7f6800afc000 is not empty!0 0 1
Visitor 0x7f6800a79000 is not empty!0 0 1
Visitor 0x7f6800a73000 is not empty!0 0 1
Visitor 0x7f6800a6d000 is not empty!0 0 1
Visitor 0x7f6800a67000 is not empty!0 16 1
1   0x7f68496d8967 WTFCrash
2   0x7f684913fe67 JSC::Heap::assertMarkStacksEmpty(int)
3   0x7f684914948d JSC::Heap::runFixpointPhase(JSC::GCConductor)
4   0x7f68491496d9 JSC::Heap::runCurrentPhase(JSC::GCConductor, JSC::CurrentThreadState*)
5   0x7f684914b24a
6   0x7f6849157e35 JSC::callWithCurrentThreadState(WTF::ScopedLambda<void (JSC::CurrentThreadState&)> const&)
7   0x7f68491497a7 JSC::Heap::collectInMutatorThread()
8   0x7f684914980c JSC::Heap::stopIfNecessarySlow(unsigned int)
9   0x7f684914984b JSC::Heap::stopIfNecessarySlow()
10  0x7f6849149e63 JSC::Heap::collectIfNecessaryOrDefer(JSC::GCDeferralContext*)
11  0x7f684915d36f JSC::MarkedAllocator::allocateSlowCase(JSC::GCDeferralContext*, JSC::AllocationFailureMode)
12  0x7f68492259e7
13 0x7f6804c4f7a0

This `1` is

>   1331         if (converged && slotVisitor.isEmpty()) {
>   1332             assertMarkStacksEmpty();
>   1333             return changePhase(conn, CollectorPhase::End);
>   1334         }

's code location. It seems that Visitor 0x7f6800a67000 has 16 cells in MutatorStack. I collected several crashes. It seems that non empty SlotVisitor is always in m_parallelSlotVisitors.

This is my rough thought: can # of m_parallelSlotVisitors increase between MarkingConstraintSolver's constructor and parallel run tasks?
Comment 66 Yusuke Suzuki 2017-12-15 12:44:21 PST
(In reply to Yusuke Suzuki from comment #65)
> This is my rough thought: can # of m_parallelSlotVisitors increase between
> MarkingConstraintSolver's constructor and parallel run tasks?

We set up m_visitCounters in MarkingConstraintSolver. And we release m_parallelSlotVisitorLock.
And we decide converged state in MarkingConstaintSet::executeConvergenceImpl() based on this m_visitCounters.
After that, we iterate slot visitors in assertMarkStacksEmpty.
When executing executeConvergenceImpl's MarkingConstraintSolver::converge, m_parallelSlotVisitorLock is not held.
If SlotVisitor can increase during this, is the subsequent assertMarkStacksEmpty reasonable?
Comment 67 Filip Pizlo 2017-12-15 12:44:59 PST
(In reply to Yusuke Suzuki from comment #65)
> Thanks! Now investigating why it happens...
> Basically, 1-6 tests fail randomly in run-javascriptcore-tests. If we
> disable parallel constraint solver by setting export
> JSC_useParallelMarkingConstraintSolver=false, we do not observe this failure.
> 
> I've a bit instrumented the code to dump more information.
> 
> FATAL: Visitor 0x7f6800a67000 is not empty!1
> Visitor 0x7f6844cfe090 is not empty!0 0 1
> Visitor 0x7f6844cfe120 is not empty!0 0 1
> Visitor 0x7f6800ada000 is not empty!0 0 1
> Visitor 0x7f6800ad1000 is not empty!0 0 1
> Visitor 0x7f6800afc000 is not empty!0 0 1
> Visitor 0x7f6800a79000 is not empty!0 0 1
> Visitor 0x7f6800a73000 is not empty!0 0 1
> Visitor 0x7f6800a6d000 is not empty!0 0 1
> Visitor 0x7f6800a67000 is not empty!0 16 1
> 1   0x7f68496d8967 WTFCrash
> 2   0x7f684913fe67 JSC::Heap::assertMarkStacksEmpty(int)
> 3   0x7f684914948d JSC::Heap::runFixpointPhase(JSC::GCConductor)
> 4   0x7f68491496d9 JSC::Heap::runCurrentPhase(JSC::GCConductor,
> JSC::CurrentThreadState*)
> 5   0x7f684914b24a
> 6   0x7f6849157e35 JSC::callWithCurrentThreadState(WTF::ScopedLambda<void
> (JSC::CurrentThreadState&)> const&)
> 7   0x7f68491497a7 JSC::Heap::collectInMutatorThread()
> 8   0x7f684914980c JSC::Heap::stopIfNecessarySlow(unsigned int)
> 9   0x7f684914984b JSC::Heap::stopIfNecessarySlow()
> 10  0x7f6849149e63
> JSC::Heap::collectIfNecessaryOrDefer(JSC::GCDeferralContext*)
> 11  0x7f684915d36f
> JSC::MarkedAllocator::allocateSlowCase(JSC::GCDeferralContext*,
> JSC::AllocationFailureMode)
> 12  0x7f68492259e7
> 13 0x7f6804c4f7a0
> 
> This `1` is
> 
> >   1331         if (converged && slotVisitor.isEmpty()) {
> >   1332             assertMarkStacksEmpty();
> >   1333             return changePhase(conn, CollectorPhase::End);
> >   1334         }
> 
> 's code location. It seems that Visitor 0x7f6800a67000 has 16 cells in
> MutatorStack. I collected several crashes. It seems that non empty
> SlotVisitor is always in m_parallelSlotVisitors.
> 
> This is my rough thought: can # of m_parallelSlotVisitors increase between
> MarkingConstraintSolver's constructor and parallel run tasks?

Yes!  Because the parallel helper pool may realize as late as it likes how many threads to start. Sometimes a tread waits a long time before starting just because of weird OS jitter. Maybe this happens more on Linux. 

Won’t this cause an increase in number of active workers?
Comment 68 Yusuke Suzuki 2017-12-15 12:59:54 PST
(In reply to Filip Pizlo from comment #67)
> Yes!  Because the parallel helper pool may realize as late as it likes how
> many threads to start. Sometimes a tread waits a long time before starting
> just because of weird OS jitter. Maybe this happens more on Linux. 
> 
> Won’t this cause an increase in number of active workers?

Yeah, it seems that # of SlotVisitors is increasing after setting up m_visitCounters.
I added the following code before `unsigned iteration = m_iterations++;` and after `solver.converge(m_ordered);` in MarkingConstraintSet::executeConvergenceImpl.

+    unsigned beforeCount = 0;
+    m_heap.forEachSlotVisitor([&] (SlotVisitor& visitor) {
+        ++beforeCount;
+    });

+    unsigned AfterCount = 0;
+    m_heap.forEachSlotVisitor([&] (SlotVisitor& visitor) {
+        ++afterCount;
+    });

And I inserted the following check just before `return !solver.didVisitSomething();`.

+    if (afterCount != beforeCount) {
+        dataLog(afterCount, " ", beforeCount, "\n");
+        RELEASE_ASSERT_NOT_REACHED();
+    }

So, assertion hits so often. It seems that # of SlotVisitors can increase after m_visitCounters are created.
As a result, I think that the current `converged` result does not mean all the SlotVisitors are empty (this is checked in assertMarkStacksEmpty). Is my understanding correct?
Comment 69 Filip Pizlo 2017-12-15 13:23:37 PST
(In reply to Yusuke Suzuki from comment #68)
> (In reply to Filip Pizlo from comment #67)
> > Yes!  Because the parallel helper pool may realize as late as it likes how
> > many threads to start. Sometimes a tread waits a long time before starting
> > just because of weird OS jitter. Maybe this happens more on Linux. 
> > 
> > Won’t this cause an increase in number of active workers?
> 
> Yeah, it seems that # of SlotVisitors is increasing after setting up
> m_visitCounters.
> I added the following code before `unsigned iteration = m_iterations++;` and
> after `solver.converge(m_ordered);` in
> MarkingConstraintSet::executeConvergenceImpl.
> 
> +    unsigned beforeCount = 0;
> +    m_heap.forEachSlotVisitor([&] (SlotVisitor& visitor) {
> +        ++beforeCount;
> +    });
> 
> +    unsigned AfterCount = 0;
> +    m_heap.forEachSlotVisitor([&] (SlotVisitor& visitor) {
> +        ++afterCount;
> +    });
> 
> And I inserted the following check just before `return
> !solver.didVisitSomething();`.
> 
> +    if (afterCount != beforeCount) {
> +        dataLog(afterCount, " ", beforeCount, "\n");
> +        RELEASE_ASSERT_NOT_REACHED();
> +    }
> 
> So, assertion hits so often. It seems that # of SlotVisitors can increase
> after m_visitCounters are created.
> As a result, I think that the current `converged` result does not mean all
> the SlotVisitors are empty (this is checked in assertMarkStacksEmpty). Is my
> understanding correct?

Yeah, it looks like if number of visitors changes, then MarkingConstraintSolver::didVisitSomething needs to return true.

That seems like it would solve the bug!
Comment 70 Yusuke Suzuki 2017-12-16 06:16:38 PST
(In reply to Filip Pizlo from comment #69)
> (In reply to Yusuke Suzuki from comment #68)
> > (In reply to Filip Pizlo from comment #67)
> > > Yes!  Because the parallel helper pool may realize as late as it likes how
> > > many threads to start. Sometimes a tread waits a long time before starting
> > > just because of weird OS jitter. Maybe this happens more on Linux. 
> > > 
> > > Won’t this cause an increase in number of active workers?
> > 
> > Yeah, it seems that # of SlotVisitors is increasing after setting up
> > m_visitCounters.
> > I added the following code before `unsigned iteration = m_iterations++;` and
> > after `solver.converge(m_ordered);` in
> > MarkingConstraintSet::executeConvergenceImpl.
> > 
> > +    unsigned beforeCount = 0;
> > +    m_heap.forEachSlotVisitor([&] (SlotVisitor& visitor) {
> > +        ++beforeCount;
> > +    });
> > 
> > +    unsigned AfterCount = 0;
> > +    m_heap.forEachSlotVisitor([&] (SlotVisitor& visitor) {
> > +        ++afterCount;
> > +    });
> > 
> > And I inserted the following check just before `return
> > !solver.didVisitSomething();`.
> > 
> > +    if (afterCount != beforeCount) {
> > +        dataLog(afterCount, " ", beforeCount, "\n");
> > +        RELEASE_ASSERT_NOT_REACHED();
> > +    }
> > 
> > So, assertion hits so often. It seems that # of SlotVisitors can increase
> > after m_visitCounters are created.
> > As a result, I think that the current `converged` result does not mean all
> > the SlotVisitors are empty (this is checked in assertMarkStacksEmpty). Is my
> > understanding correct?
> 
> Yeah, it looks like if number of visitors changes, then
> MarkingConstraintSolver::didVisitSomething needs to return true.
> 
> That seems like it would solve the bug!

OK, opened a bug for that and uploaded the patch.
https://bugs.webkit.org/show_bug.cgi?id=180906