Patch forthcoming.
Created attachment 329519 [details] work in progress
Created attachment 329617 [details] work in progress
Created attachment 329622 [details] starting to pass some tests
Attachment 329622 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:46: More than one command on the same line [whitespace/newline] [4] ERROR: Source/JavaScriptCore/bytecode/CodeBlock.cpp:376: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 2 in 25 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 329622 [details] starting to pass some tests Attachment 329622 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/5705830 New failing tests: stress/tail-call-recognize.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/spread-large-array.js.ftl-no-cjit-validate-sampling-profiler stress/arity-check-ftl-throw-more-args.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/new-array-buffer-dead.js.ftl-no-cjit-validate-sampling-profiler stress/arrowfunction-activation-sink-osrexit-default-value.js.ftl-no-cjit-validate-sampling-profiler stress/arrowfunction-lexical-this-activation-sink-osrexit.js.ftl-no-cjit-validate-sampling-profiler stress/new-int32-array-with-size.js.ftl-no-cjit-validate-sampling-profiler stress/spread-forward-call-varargs-stack-overflow.js.ftl-no-cjit-validate-sampling-profiler stress/spread-forward-varargs-stack-overflow.js.ftl-no-cjit-validate-sampling-profiler stress/array-push-with-force-exit.js.ftl-no-cjit-validate-sampling-profiler stress/array-slice-jettison-on-constructor-change.js.ftl-no-cjit-validate-sampling-profiler microbenchmarks/direct-tail-call-arity-mismatch.js.ftl-no-cjit-validate-sampling-profiler stress/async-await-basic.js.ftl-no-cjit-validate-sampling-profiler stress/fiat-double-to-int52-then-fail-to-fold.js.ftl-no-cjit-validate-sampling-profiler stress/activation-sink-osrexit-default-value.js.ftl-no-cjit-validate-sampling-profiler stress/activation-sink-osrexit.js.ftl-no-cjit-validate-sampling-profiler
Comment on attachment 329622 [details] starting to pass some tests Attachment 329622 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5705956 New failing tests: inspector/debugger/command-line-api-exception.html
Created attachment 329625 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329622 [details] starting to pass some tests Attachment 329622 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5705876 Number of test failures exceeded the failure limit.
Created attachment 329626 [details] Archive of layout-test-results from ews117 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 329622 [details] starting to pass some tests Attachment 329622 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5706017 New failing tests: inspector/debugger/command-line-api-exception.html
Created attachment 329627 [details] Archive of layout-test-results from ews106 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 330162 [details] bringing back the HashSet This version uses a HashSet of CodeBlocks because isLive was crashing when the CodeBlock was on the block that we were using for allocation. I now realize that I could possibly also have solved the problem using HeapCell::isLive(), which accounts for free-listing. Maybe I'll also try that.
Attachment 330162 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/heap/CodeBlockSet.cpp:46: More than one command on the same line [whitespace/newline] [4] Total errors found: 1 in 22 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Filip Pizlo from comment #12) > Created attachment 330162 [details] > bringing back the HashSet > > This version uses a HashSet of CodeBlocks because isLive was crashing when > the CodeBlock was on the block that we were using for allocation. > > I now realize that I could possibly also have solved the problem using > HeapCell::isLive(), which accounts for free-listing. Maybe I'll also try > that. No, because the SamplingProfiler needs to be able to asynchronously ask if a CodeBlock is live. HeapCell::isLive() won't be able to do that.
Created attachment 330343 [details] getting rid of CodeBlock's WeakReferenceHarvester and UnconditionalFinalizer
Created attachment 330356 [details] it is almost written
Created attachment 330358 [details] now it is written I almost forgot about making executables use edges.
Created attachment 330365 [details] it compiles
Created attachment 330432 [details] it runs splay
Created attachment 330508 [details] it's perf-neutral on speedometer
Attachment 330508 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:71: The parameter name "edge" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:73: The parameter name "codeBlock" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:75: The parameter name "codeBlock" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36: Code inside a namespace should not be indented. [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.h:50: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 6 in 65 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330581 [details] the patch
Attachment 330581 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:71: The parameter name "edge" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:73: The parameter name "codeBlock" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:75: The parameter name "codeBlock" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36: Code inside a namespace should not be indented. [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.h:50: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 6 in 65 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330591 [details] fix WeakMap
Attachment 330591 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:71: The parameter name "edge" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:73: The parameter name "codeBlock" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:75: The parameter name "codeBlock" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36: Code inside a namespace should not be indented. [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.h:50: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 6 in 65 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 330591 [details] fix WeakMap Attachment 330591 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5947717 Number of test failures exceeded the failure limit.
Created attachment 330605 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 330581 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=330581&action=review r=me with comments and questions > Source/JavaScriptCore/ChangeLog:31 > + There is no way that the GC's isLive could tell us of a CodeBlock that had already been of => if > Source/JavaScriptCore/ChangeLog:32 > + allocated has now been full constructed. full => fully > Source/JavaScriptCore/bytecode/CodeBlock.cpp:339 > + m_vm->heap.codeBlockSet().add(this); Maybe put this in finishCreationCommon and remove it from various Ctors? > Source/JavaScriptCore/bytecode/CodeBlock.cpp:849 > +void CodeBlock::finishCreationCommon(VM& vm) I think it's worth asserting that type() == CodeBlockType here given it's now necessary > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1132 > + (*iter)->propagateTransitions(visitor); Is it worth dethreading the bool that all of these return? (And maybe rename it? This name still confuses the hell out of me) > Source/JavaScriptCore/bytecode/CodeBlock.cpp:1182 > + // In rare and weird cases, this could be called on a baseline CodeBlock. One that I found was > + // that we might decide that the CodeBlock should be jettisoned due to old age, so the > + // isMarked check doesn't protect us. > + if (!JITCode::isOptimizingJIT(jitType())) > + return; Maybe I'm missing something, but why is this rare/weird? It seems like the edge just calls this on the output constraint. > Source/JavaScriptCore/bytecode/CodeBlock.cpp:3071 > - m_jitCode->dfgCommon()->installVMTrapBreakpoints(this); > + auto& commonData = *m_jitCode->dfgCommon(); > + commonData.installVMTrapBreakpoints(this); Why this change? > Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.cpp:33 > +const ClassInfo ExecutableToCodeBlockEdge::s_info = { "ExecutableToCodeBlockEdge", nullptr, nullptr, nullptr, CREATE_METHOD_TABLE(ExecutableToCodeBlockEdge) }; You should point to &Base::s_info here. > Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.cpp:53 > + if (!edge->m_isActive) { Using active as the name here is confusing to me. Can we perhaps pick a better name? As I understand it active: do the pseudo-weak pointer thing not active: make it a strong ptr Am I right? If so, "not active" meaning it's a strong ptr me. > Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.cpp:64 > + if (!Heap::isMarked(codeBlock)) > + vm.executableToCodeBlockEdgesWithFinalizers.add(edge); What about the else case here when it's already marked. Why do anything below if it's already marked? Won't that imply it already marked the things it needs to such as alternative, etc. Couldn't we just remove ourself from both finalizer set and the constraint set, and return early? > Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.cpp:106 > + edge->runConstraint(NoLockingNecessary, vm, visitor); Why don't we need to hold the lock here? > Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.cpp:173 > + if (Heap::isMarked(codeBlock)) > + vm.executableToCodeBlockEdgesWithConstraints.remove(this); Can you also remove it from the finalizer set here since finalizing doesn't do anything when marked besides remove itself from the sets? > Source/JavaScriptCore/heap/CodeBlockSet.h:65 > + template<typename Functor> void iterateViaSubspaces(VM&, const Functor&); This is not used. Remove? > Source/JavaScriptCore/heap/Heap.cpp:574 > + this->finalizeMarkedUnconditionalFinalizers<CodeBlock>(space.finalizerSet); Is there a static way to ensure no CodeBlock subclass implements finalizeUnconditionally? > Source/JavaScriptCore/heap/Heap.cpp:578 > + finalizeMarkedUnconditionalFinalizers<JSWeakSet>(vm()->weakMapSpace); Fil pointed out offline to me that he's changing this to JSWeakMap. Just writing this for posterity. > Source/JavaScriptCore/runtime/EvalExecutable.h:44 > + return bitwise_cast<EvalCodeBlock*>(ExecutableToCodeBlockEdge::unwrap(m_evalCodeBlock.get())); Why not jsCast? > Source/JavaScriptCore/runtime/FunctionExecutable.h:87 > + return bitwise_cast<FunctionCodeBlock*>(ExecutableToCodeBlockEdge::unwrap(m_codeBlockForCall.get())); ditto for jsCast > Source/JavaScriptCore/runtime/FunctionExecutable.h:97 > + return bitwise_cast<FunctionCodeBlock*>(ExecutableToCodeBlockEdge::unwrap(m_codeBlockForConstruct.get())); ditto > Source/JavaScriptCore/runtime/VM.h:390 > + // This should not include webAssemblyCodeBlockSpace because this is about subsclasses of > + // JSC::CodeBlock. Ugh, yeah. We really should rename that class. The name always confuses me. And it also represents a module of code, not a single function... > Source/WTF/wtf/Deque.h:79 > + template<typename U> bool contains(const U&); I think it's worth writing a warning comment that this is a linear search.
Created attachment 330830 [details] patch for landing I think that this fixes the debug assert.
Created attachment 330831 [details] rebased patch
Created attachment 330835 [details] fixed patch Fixed build issues in the last patch.
Attachment 330835 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:71: The parameter name "edge" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:73: The parameter name "codeBlock" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:75: The parameter name "codeBlock" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36: Code inside a namespace should not be indented. [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.h:50: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 6 in 65 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 330849 [details] patch for landing
Attachment 330849 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:71: The parameter name "edge" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:73: The parameter name "codeBlock" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/bytecode/ExecutableToCodeBlockEdge.h:75: The parameter name "codeBlock" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/heap/SimpleMarkingConstraint.cpp:36: Code inside a namespace should not be indented. [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.h:50: Wrong number of spaces before statement. (expected: 12) [whitespace/indent] [4] Total errors found: 6 in 65 files If any of these errors are false positives, please file a bug against check-webkit-style.
Landed for the first time in http://trac.webkit.org/changeset/226667/webkit
<rdar://problem/36390543>
I think that this broke my local build, even though bots are fine: ./heap/Heap.cpp:2708:10: error: lambda capture 'this' is not used [-Werror,-Wunused-lambda-capture] [this] (SlotVisitor& slotVisitor) { ^ 1 error generated.
(In reply to Filip Pizlo from comment #35) > Landed for the first time in http://trac.webkit.org/changeset/226667/webkit Build fix landed in r226673: <http://trac.webkit.org/changeset/226673/webkit>
(In reply to Alexey Proskuryakov from comment #37) > I think that this broke my local build, even though bots are fine: > > ./heap/Heap.cpp:2708:10: error: lambda capture 'this' is not used > [-Werror,-Wunused-lambda-capture] > [this] (SlotVisitor& slotVisitor) { > ^ > 1 error generated. It also broke internal bots with a newer clang compiler.
This also caused a crash in the test: webgl/1.0.2/conformance/uniforms/uniform-default-values.html. Crash: https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/r226718%20(6817)/webgl/1.0.2/conformance/uniforms/uniform-default-values-crash-log.txt https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=webgl%2F1.0.2%2Fconformance%2Funiforms%2Funiform-default-values.html
Re-opened since this is blocked by bug 181488
(In reply to Matt Lewis from comment #40) > This also caused a crash in the test: > webgl/1.0.2/conformance/uniforms/uniform-default-values.html. > > Crash: > https://build.webkit.org/results/Apple%20Sierra%20Release%20WK2%20(Tests)/ > r226718%20(6817)/webgl/1.0.2/conformance/uniforms/uniform-default-values- > crash-log.txt > > > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=webgl%2F1.0.2%2Fconformance%2Funiforms%2Funiform- > default-values.html According this this dashboard, this crash was happening in past revisions. For example: https://build.webkit.org/results/Apple%20High%20Sierra%20Release%20WK2%20(Tests)/r226660%20(2136)/results.html https://build.webkit.org/results/Apple%20El%20Capitan%20Release%20WK1%20(Tests)/r226666%20(7511)/results.html That's r226660 and r226666, which are before http://trac.webkit.org/changeset/226667/webkit I think that based on this, I'm going to roll this back in later today. Can you double-check my logic?
That seems correct. I must have misread the numbers in the revision! Sorry about that. Feel free to roll them back in.
Relanded in http://trac.webkit.org/changeset/226783/webkit