Bug 180884 - CodeBlocks should be in IsoSubspaces
Summary: CodeBlocks should be in IsoSubspaces
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on: 181488
Blocks:
  Show dependency treegraph
 
Reported: 2017-12-15 14:35 PST by Filip Pizlo
Modified: 2018-01-11 08:43 PST (History)
15 users (show)

See Also:


Attachments
work in progress (28.37 KB, patch)
2017-12-15 14:35 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
work in progress (37.71 KB, patch)
2017-12-17 12:03 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
starting to pass some tests (42.53 KB, patch)
2017-12-17 13:55 PST, Filip Pizlo
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (2.14 MB, application/zip)
2017-12-17 15:02 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (3.06 MB, application/zip)
2017-12-17 15:07 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (2.57 MB, application/zip)
2017-12-17 15:17 PST, EWS Watchlist
no flags Details
bringing back the HashSet (37.73 KB, patch)
2017-12-23 12:15 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
getting rid of CodeBlock's WeakReferenceHarvester and UnconditionalFinalizer (87.75 KB, patch)
2018-01-02 15:06 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it is almost written (93.94 KB, patch)
2018-01-02 17:13 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
now it is written (104.75 KB, patch)
2018-01-02 17:28 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it compiles (114.34 KB, patch)
2018-01-02 19:02 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it runs splay (118.89 KB, patch)
2018-01-03 19:32 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
it's perf-neutral on speedometer (131.96 KB, patch)
2018-01-04 18:40 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (132.14 KB, patch)
2018-01-05 14:12 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
fix WeakMap (132.13 KB, patch)
2018-01-05 15:04 PST, Filip Pizlo
saam: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-elcapitan (1.88 MB, application/zip)
2018-01-05 16:39 PST, EWS Watchlist
no flags Details
patch for landing (133.20 KB, patch)
2018-01-09 10:28 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
rebased patch (133.21 KB, patch)
2018-01-09 10:38 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
fixed patch (133.44 KB, patch)
2018-01-09 10:48 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
patch for landing (133.44 KB, patch)
2018-01-09 14:30 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2017-12-15 14:35:10 PST
Patch forthcoming.
Comment 1 Filip Pizlo 2017-12-15 14:35:38 PST
Created attachment 329519 [details]
work in progress
Comment 2 Filip Pizlo 2017-12-17 12:03:53 PST
Created attachment 329617 [details]
work in progress
Comment 3 Filip Pizlo 2017-12-17 13:55:41 PST
Created attachment 329622 [details]
starting to pass some tests
Comment 4 EWS Watchlist 2017-12-17 13:57:36 PST
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 5 EWS Watchlist 2017-12-17 14:58:33 PST
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 6 EWS Watchlist 2017-12-17 15:02:51 PST
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
Comment 7 EWS Watchlist 2017-12-17 15:02:52 PST
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 8 EWS Watchlist 2017-12-17 15:07:36 PST
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.
Comment 9 EWS Watchlist 2017-12-17 15:07:37 PST
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 10 EWS Watchlist 2017-12-17 15:17:18 PST
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
Comment 11 EWS Watchlist 2017-12-17 15:17:20 PST
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
Comment 12 Filip Pizlo 2017-12-23 12:15:20 PST
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.
Comment 13 EWS Watchlist 2017-12-23 12:23:37 PST
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.
Comment 14 Filip Pizlo 2017-12-27 13:36:30 PST
(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.
Comment 15 Filip Pizlo 2018-01-02 15:06:57 PST
Created attachment 330343 [details]
getting rid of CodeBlock's WeakReferenceHarvester and UnconditionalFinalizer
Comment 16 Filip Pizlo 2018-01-02 17:13:17 PST
Created attachment 330356 [details]
it is almost written
Comment 17 Filip Pizlo 2018-01-02 17:28:46 PST
Created attachment 330358 [details]
now it is written

I almost forgot about making executables use edges.
Comment 18 Filip Pizlo 2018-01-02 19:02:02 PST
Created attachment 330365 [details]
it compiles
Comment 19 Filip Pizlo 2018-01-03 19:32:27 PST
Created attachment 330432 [details]
it runs splay
Comment 20 Filip Pizlo 2018-01-04 18:40:40 PST
Created attachment 330508 [details]
it's perf-neutral on speedometer
Comment 21 EWS Watchlist 2018-01-05 11:01:11 PST
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.
Comment 22 Filip Pizlo 2018-01-05 14:12:11 PST
Created attachment 330581 [details]
the patch
Comment 23 EWS Watchlist 2018-01-05 14:15:29 PST
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.
Comment 24 Filip Pizlo 2018-01-05 15:04:25 PST
Created attachment 330591 [details]
fix WeakMap
Comment 25 EWS Watchlist 2018-01-05 15:07:38 PST
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 26 EWS Watchlist 2018-01-05 16:39:29 PST
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.
Comment 27 EWS Watchlist 2018-01-05 16:39:31 PST
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 28 Saam Barati 2018-01-05 17:37:37 PST
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.
Comment 29 Filip Pizlo 2018-01-09 10:28:51 PST
Created attachment 330830 [details]
patch for landing

I think that this fixes the debug assert.
Comment 30 Filip Pizlo 2018-01-09 10:38:57 PST
Created attachment 330831 [details]
rebased patch
Comment 31 Filip Pizlo 2018-01-09 10:48:02 PST
Created attachment 330835 [details]
fixed patch

Fixed build issues in the last patch.
Comment 32 EWS Watchlist 2018-01-09 10:50:16 PST
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.
Comment 33 Filip Pizlo 2018-01-09 14:30:43 PST
Created attachment 330849 [details]
patch for landing
Comment 34 EWS Watchlist 2018-01-09 14:48:09 PST
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.
Comment 35 Filip Pizlo 2018-01-09 16:34:55 PST
Landed for the first time in http://trac.webkit.org/changeset/226667/webkit
Comment 36 Radar WebKit Bug Importer 2018-01-09 16:35:23 PST
<rdar://problem/36390543>
Comment 37 Alexey Proskuryakov 2018-01-09 17:25:52 PST
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.
Comment 38 David Kilzer (:ddkilzer) 2018-01-09 19:05:33 PST
(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>
Comment 39 David Kilzer (:ddkilzer) 2018-01-09 19:06:19 PST
(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.
Comment 41 WebKit Commit Bot 2018-01-10 11:32:20 PST
Re-opened since this is blocked by bug 181488
Comment 42 Filip Pizlo 2018-01-10 15:02:30 PST
(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?
Comment 43 Matt Lewis 2018-01-10 15:09:08 PST
That seems correct. I must have misread the numbers in the revision! Sorry about that. Feel free to roll them back in.
Comment 44 Filip Pizlo 2018-01-11 08:43:44 PST
Relanded in http://trac.webkit.org/changeset/226783/webkit