Bug 180884 – CodeBlocks should be in IsoSubspaces

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 329519 [details]
work in progress

[Filip Pizlo]

Created attachment 329617 [details]
work in progress

[Filip Pizlo]

Created attachment 329622 [details]
starting to pass some tests

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Filip Pizlo]

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.

[EWS Watchlist]

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.

[Filip Pizlo]

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

[Filip Pizlo]

Created attachment 330343 [details]
getting rid of CodeBlock's WeakReferenceHarvester and UnconditionalFinalizer

[Filip Pizlo]

Created attachment 330356 [details]
it is almost written

[Filip Pizlo]

Created attachment 330358 [details]
now it is written

I almost forgot about making executables use edges.

[Filip Pizlo]

Created attachment 330365 [details]
it compiles

[Filip Pizlo]

Created attachment 330432 [details]
it runs splay

[Filip Pizlo]

Created attachment 330508 [details]
it's perf-neutral on speedometer

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 330581 [details]
the patch

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 330591 [details]
fix WeakMap

[EWS Watchlist]

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.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Saam Barati]

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.

[Filip Pizlo]

Created attachment 330830 [details]
patch for landing

I think that this fixes the debug assert.

[Filip Pizlo]

Created attachment 330831 [details]
rebased patch

[Filip Pizlo]

Created attachment 330835 [details]
fixed patch

Fixed build issues in the last patch.

[EWS Watchlist]

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.

[Filip Pizlo]

Created attachment 330849 [details]
patch for landing

[EWS Watchlist]

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.

[Filip Pizlo]

Landed for the first time in http://trac.webkit.org/changeset/226667/webkit

[Radar WebKit Bug Importer]

<rdar://problem/36390543>

[Alexey Proskuryakov]

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.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

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

[Matt Lewis]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 181488

[Filip Pizlo]

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

[Matt Lewis]

That seems correct. I must have misread the numbers in the revision! Sorry about that. Feel free to roll them back in.

[Filip Pizlo]

Relanded in http://trac.webkit.org/changeset/226783/webkit