Bug 179934

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.

Created attachment 327460 [details] I think it's written

Created attachment 327517 [details] more

Created attachment 327521 [details] I'm starting to like the algorithm

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.

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.

(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.

Created attachment 327576 [details] taking a shot at a better load balancer

Created attachment 327577 [details] more

Created attachment 327578 [details] a bit nicer Put each new class into its own file

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.

Created attachment 327581 [details] neutral on splay-latency

(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.

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.

Created attachment 327766 [details] more

Created attachment 327788 [details] a bit more To make this make any sense, I needed to improve how the opaque root set works.

Created attachment 327794 [details] fix some compile errors

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.

Created attachment 327951 [details] ConcurrentPtrHashSet unit tests pass

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.

Created attachment 327996 [details] more Removed the need to lock the event target data map lock.

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.

Created attachment 328121 [details] it compiles again

Created attachment 328129 [details] the patch

EWS doesn't like the patch. Can you rebase?

(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 on attachment 328129 [details] the patch This is not yet ready for review.

Created attachment 328149 [details] rebased

Created attachment 328156 [details] the patch

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.

Created attachment 328158 [details] the patch Made a few stylistic tweaks.

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.

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.

Created attachment 328218 [details] back to progression on speedo I think

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.

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 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

Created attachment 328233 [details] more solid speedup

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.

(In reply to Filip Pizlo from comment #38) > Created attachment 328233 [details] > more solid speedup This totally has crashes when collecting continuously. Investigating...

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

Created attachment 328258 [details] I think I fixed the crash bug

(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.

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.

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...

Created attachment 328274 [details] an even better attempt StampedLock combined with dependency chaining FTW.

Created attachment 328276 [details] the patch Still a speed-up on Speedometer. No longer crashing. Need to test ARM...

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.

Created attachment 328280 [details] the patch

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.

Created attachment 328298 [details] more Cleaned up some things.

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.

Created attachment 328317 [details] the patch

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.

(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 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

(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.

Created attachment 328438 [details] patch for landing

> > > 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!

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.

Landed in https://trac.webkit.org/changeset/225524/webkit

<rdar://problem/35857160>

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?

(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!

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?

(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?

(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?

(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?

(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!

(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