Bug 185652 – JSC should have InstanceOf inline caching

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 340417 [details]
it begins

[Filip Pizlo]

Created attachment 340419 [details]
a bit more

[Radar WebKit Bug Importer]

<rdar://problem/40271131>

[Filip Pizlo]

Created attachment 340440 [details]
more

[Filip Pizlo]

Created attachment 340446 [details]
a little more

[Filip Pizlo]

Created attachment 340499 [details]
close to done

Just need to do FTL.

[Filip Pizlo]

Created attachment 340511 [details]
maybe it is written

[Filip Pizlo]

Created attachment 340512 [details]
and now with tests

I haven't tried compiling it yet but it's written.  At least on 64-bit.

[EWS Watchlist]

Attachment 340512 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:80:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:180:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:181:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:182:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:455:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:456:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:457:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:81:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:135:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:61:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.h:48:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 13 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 340537 [details]
it compiles

[Filip Pizlo]

Created attachment 340548 [details]
it's starting to do things

[Filip Pizlo]

Created attachment 340549 [details]
better patch

[Filip Pizlo]

Created attachment 340551 [details]
starting to make some sense

[Filip Pizlo]

Comment on attachment 340551 [details]
starting to make some sense

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

> Source/JavaScriptCore/runtime/Options.h:293
> -    v(unsigned, numberOfDFGCompilerThreads, computeNumberOfWorkerThreads(3, 2) - 1, Normal, nullptr) \
> +    v(unsigned, numberOfDFGCompilerThreads, computeNumberOfWorkerThreads(MAXIMUM_NUMBER_OF_FTL_COMPILER_THREADS, 2) - 1, Normal, nullptr) \

I will revert.

[Filip Pizlo]

Created attachment 340555 [details]
more

[Filip Pizlo]

Created attachment 340556 [details]
fixed more bugs

It's passing so many tests.

[EWS Watchlist]

Attachment 340556 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/Repatch.h:48:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:76:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:455:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:456:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:457:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:81:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:135:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:61:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 10 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 340556 [details]
fixed more bugs

Attachment 340556 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7707834

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 340573 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340556 [details]
fixed more bugs

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

New failing tests:
stress/instanceof-dynamic-proxy-loop.js.dfg-eager-no-cjit-validate
stress/instanceof-dynamic-proxy-loop.js.ftl-eager-no-cjit
stress/instanceof-dynamic-proxy-loop.js.dfg-eager
stress/instanceof-dynamic-proxy-loop.js.no-ftl
stress/instanceof-dynamic-proxy-loop.js.default
stress/instanceof-dynamic-proxy-loop.js.ftl-no-cjit-no-put-stack-validate
stress/instanceof-dynamic-proxy-loop.js.ftl-no-cjit-no-inline-validate
stress/instanceof-dynamic-proxy-loop.js.dfg-maximal-flush-validate-no-cjit
stress/instanceof-dynamic-proxy-loop.js.ftl-no-cjit-validate-sampling-profiler
stress/instanceof-dynamic-proxy-loop.js.ftl-eager
stress/instanceof-dynamic-proxy-loop.js.ftl-eager-no-cjit-b3o1
stress/instanceof-dynamic-proxy-loop.js.ftl-no-cjit-b3o1
stress/instanceof-dynamic-proxy-loop.js.ftl-no-cjit-small-pool
stress/instanceof-dynamic-proxy-loop.js.no-llint
stress/instanceof-dynamic-proxy-loop.js.no-cjit-collect-continuously
stress/instanceof-dynamic-proxy-loop.js.no-cjit-validate-phases

[EWS Watchlist]

Comment on attachment 340556 [details]
fixed more bugs

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

New failing tests:
jquery/core.html
jquery/manipulation.html
media/modern-media-controls/scrubber-support/scrubber-support-drag.html
imported/w3c/web-platform-tests/dom/interfaces.html

[EWS Watchlist]

Created attachment 340574 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340556 [details]
fixed more bugs

Attachment 340556 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7707938

New failing tests:
jquery/core.html
imported/w3c/web-platform-tests/dom/interfaces.html

[EWS Watchlist]

Created attachment 340576 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 340556 [details]
fixed more bugs

Attachment 340556 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7708673

New failing tests:
jquery/core.html
jquery/manipulation.html
media/modern-media-controls/scrubber-support/scrubber-support-drag.html
imported/w3c/web-platform-tests/dom/interfaces.html

[EWS Watchlist]

Created attachment 340580 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Filip Pizlo]

Created attachment 340585 [details]
make more tests pass

[EWS Watchlist]

Attachment 340585 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/Repatch.h:48:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:75:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:455:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:456:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:457:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:81:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:135:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:61:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 10 in 57 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 340585 [details]
make more tests pass

Attachment 340585 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7710180

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 340593 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 340585 [details]
make more tests pass

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

New failing tests:
stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager
apiTests

[Filip Pizlo]

Created attachment 340616 [details]
fix debug

[Filip Pizlo]

Created attachment 340636 [details]
starting to fix 32-bit

[EWS Watchlist]

Attachment 340636 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/Repatch.h:48:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:75:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:455:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:456:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:457:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:81:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:135:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:61:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 10 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 340649 [details]
getting to the bottom of 32-bit test failures

[EWS Watchlist]

Attachment 340649 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/Repatch.h:48:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:75:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:455:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:456:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:457:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:81:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:135:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:61:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 10 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 340665 [details]
the patch

[EWS Watchlist]

Attachment 340665 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/Repatch.h:48:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:75:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:455:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:456:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:457:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:81:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:135:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:61:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 10 in 60 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 340665 [details]
the patch

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

gonna keep reviewing

> JSTests/ChangeLog:113
> +        (Foo0):
> +        (Foo1):
> +        (Foo2):
> +        (Foo3):
> +        (Foo4):
> +        (Foo5):
> +        (Foo6):
> +        (Foo7):
> +        (Foo8):
> +        (Foo9):
> +        (Foo10):
> +        (Foo11):
> +        (Foo12):
> +        (Foo13):
> +        (Foo14):
> +        (Foo15):
> +        (Foo16):
> +        (Foo17):
> +        (Foo18):
> +        (Foo19):
> +        (Foo20):
> +        (Foo21):
> +        (Foo22):
> +        (Foo23):
> +        (Foo24):
> +        (Foo25):
> +        (Foo26):
> +        (Foo27):
> +        (Foo28):
> +        (Foo29):
> +        (Foo30):
> +        (Foo31):
> +        (Foo32):
> +        (Foo33):
> +        (Foo34):
> +        (Foo35):
> +        (Foo36):
> +        (Foo37):
> +        (Foo38):
> +        (Foo39):
> +        (Foo40):
> +        (Foo41):
> +        (Foo42):
> +        (Foo43):
> +        (Foo44):
> +        (Foo45):
> +        (Foo46):
> +        (Foo47):
> +        (Foo48):
> +        (Foo49):
> +        (Foo50):
> +        (Foo51):
> +        (Foo52):
> +        (Foo53):
> +        (Foo54):
> +        (Foo55):
> +        (Foo56):
> +        (Foo57):
> +        (Foo58):
> +        (Foo59):
> +        (Foo60):
> +        (Foo61):
> +        (Foo62):
> +        (Foo63):
> +        (Foo64):
> +        (Foo65):
> +        (Foo66):
> +        (Foo67):
> +        (Foo68):
> +        (Foo69):
> +        (Foo70):
> +        (Foo71):
> +        (Foo72):
> +        (Foo73):
> +        (Foo74):
> +        (Foo75):
> +        (Foo76):
> +        (Foo77):
> +        (Foo78):
> +        (Foo79):
> +        (Foo80):
> +        (Foo81):
> +        (Foo82):
> +        (Foo83):
> +        (Foo84):
> +        (Foo85):
> +        (Foo86):
> +        (Foo87):
> +        (Foo88):
> +        (Foo89):
> +        (Foo90):
> +        (Foo91):
> +        (Foo92):
> +        (Foo93):
> +        (Foo94):
> +        (Foo95):
> +        (Foo96):
> +        (Foo97):
> +        (Foo98):
> +        (Foo99):

Probably can remove this and below too in the changelog.

> Source/JavaScriptCore/API/tests/testapi.mm:-1500
> -    @autoreleasepool {
> -        JSContext *context = [[JSContext alloc] init];
> -        JSVirtualMachine *vm = [context virtualMachine];
> -        [vm shrinkFootprint]; // Make sure that when we allocate a ton of memory below we reuse at little as possible.
> -
> -        std::optional<size_t> footprintBefore = WTF::memoryFootprint();
> -        RELEASE_ASSERT(footprintBefore);
> -
> -        [context evaluateScript:@"for (let i = 0; i < 10000; ++i) { eval(`myVariable_${i} = [i]`); }"];
> -
> -        static constexpr size_t approximateBytes = 10000 * sizeof(int);
> -        std::optional<size_t> footprintMiddle = WTF::memoryFootprint();
> -        RELEASE_ASSERT(footprintMiddle);
> -        checkResult(@"Footprint is larger than what we allocated", *footprintMiddle > approximateBytes);
> -        checkResult(@"Footprint got larger as we allocated a ton of stuff", *footprintMiddle > *footprintBefore);
> -        size_t allocationDelta = *footprintMiddle - *footprintBefore;
> -        checkResult(@"We allocated as much or more memory than what we expected to", allocationDelta >= approximateBytes);
> -
> -        [context evaluateScript:@"for (let i = 0; i < 10000; ++i) { eval(`myVariable_${i} = null`); }"];
> -        [vm shrinkFootprint];
> -        std::optional<size_t> footprintAfter = WTF::memoryFootprint();
> -        RELEASE_ASSERT(footprintAfter);
> -        checkResult(@"Footprint got smaller after we shrank the VM", *footprintAfter < *footprintMiddle);
> -        size_t freeDelta = *footprintMiddle - *footprintAfter;
> -        checkResult(@"Shrinking the footprint of the VM actually freed memory", freeDelta > (approximateBytes / 2));
> -    }

Let's do this in its own patch

[Saam Barati]

Comment on attachment 340665 [details]
the patch

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

r=me

> JSTests/stress/instanceof-prototype-change.js:21
> +Bar.prototype.__proto__ = {}

Can you also add a test where you cut off the prototype to null somewhere in the prototype chain?
Can you also add a test where you insert a prototype in a InstanceOfMiss case? (If you don't have such a test already)

>> Source/JavaScriptCore/API/tests/testapi.mm:-1500
>> -    }
> 
> Let's do this in its own patch

I opened https://bugs.webkit.org/show_bug.cgi?id=185754 for this

> Source/JavaScriptCore/b3/B3Effects.h:112
> +    static Effects forReadOnlyCall()
> +    {
> +        Effects result;
> +        result.exitsSideways = true;
> +        result.controlDependent = true;
> +        result.reads = HeapRange::top();
> +        result.readsPinned = true;
> +        result.fence = true;
> +        return result;
> +    }

This is unused. Did you mean to use it in FTL lower?

> Source/JavaScriptCore/bytecode/AccessCase.cpp:534
> +        // Legend: value = `base instanceof this`.

I actually think it'd be helpful to say something like this:
value = `a instanceof b`
where
base = a
this = b.prototype

> Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:265
> +        

please revert

> Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:251
> +    

please revert

> Source/JavaScriptCore/bytecode/PropertyCondition.cpp:195
> +        if (structure->isDictionary()) {

Why do we care about this for HasPrototype? Don't we still transition dictionaries when we set their prototype?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3437
> +        m_jit.recordCallSiteAndGenerateExceptionHandlingOSRExitIfNeeded(
> +            node->origin.semantic, m_stream->size());

I don't think we need this. We can't throw from this IC. I think you just want: m_jit.addCallSite(CodeOrigin)

> Source/JavaScriptCore/jit/JITOperations.cpp:2204
> +    bool result = JSObject::defaultHasInstance(exec, value, proto);

Do we want an exception check here?

> Source/JavaScriptCore/runtime/JSCellInlines.h:218
> +    return m_type == ImpureProxyType || m_type == PureForwardingProxyType || m_type == ProxyObjectType;

It's a little weird this returns true for ES6 proxies. I feel like we treat that differently than JSProxy.

[Filip Pizlo]

(In reply to Saam Barati from comment #40)
> Comment on attachment 340665 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340665&action=review
> 
> r=me
> 
> > JSTests/stress/instanceof-prototype-change.js:21
> > +Bar.prototype.__proto__ = {}
> 
> Can you also add a test where you cut off the prototype to null somewhere in
> the prototype chain?
> Can you also add a test where you insert a prototype in a InstanceOfMiss
> case? (If you don't have such a test already)

Yeah, I'll add those tests.

> 
> >> Source/JavaScriptCore/API/tests/testapi.mm:-1500
> >> -    }
> > 
> > Let's do this in its own patch
> 
> I opened https://bugs.webkit.org/show_bug.cgi?id=185754 for this

Cool!

> 
> > Source/JavaScriptCore/b3/B3Effects.h:112
> > +    static Effects forReadOnlyCall()
> > +    {
> > +        Effects result;
> > +        result.exitsSideways = true;
> > +        result.controlDependent = true;
> > +        result.reads = HeapRange::top();
> > +        result.readsPinned = true;
> > +        result.fence = true;
> > +        return result;
> > +    }
> 
> This is unused. Did you mean to use it in FTL lower?

Heh.  Before I realized that InstanceOf had side effects, I was going to use this to describe the InstanceOf patchpoint.

> 
> > Source/JavaScriptCore/bytecode/AccessCase.cpp:534
> > +        // Legend: value = `base instanceof this`.
> 
> I actually think it'd be helpful to say something like this:
> value = `a instanceof b`
> where
> base = a
> this = b.prototype
> 
> > Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp:265
> > +        
> 
> please revert

Fixed.

> 
> > Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp:251
> > +    
> 
> please revert

Fixed.

> 
> > Source/JavaScriptCore/bytecode/PropertyCondition.cpp:195
> > +        if (structure->isDictionary()) {
> 
> Why do we care about this for HasPrototype? Don't we still transition
> dictionaries when we set their prototype?

Fixed.  Also fixed the same issue in Structure::prototypeQueriesAreCacheable().

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3437
> > +        m_jit.recordCallSiteAndGenerateExceptionHandlingOSRExitIfNeeded(
> > +            node->origin.semantic, m_stream->size());
> 
> I don't think we need this. We can't throw from this IC. I think you just
> want: m_jit.addCallSite(CodeOrigin)

Oh yeah!  Fixed.

> 
> > Source/JavaScriptCore/jit/JITOperations.cpp:2204
> > +    bool result = JSObject::defaultHasInstance(exec, value, proto);
> 
> Do we want an exception check here?

Yes.  Fixed.

> 
> > Source/JavaScriptCore/runtime/JSCellInlines.h:218
> > +    return m_type == ImpureProxyType || m_type == PureForwardingProxyType || m_type == ProxyObjectType;
> 
> It's a little weird this returns true for ES6 proxies. I feel like we treat
> that differently than JSProxy.

I checked all callsites.  They all meant to say "is internal JSProxy or ES6 proxy".  The fact that they didn't was asymptomatic because these were IC helper functions that only run if we decided to cache, and ES6 proxies make us fail caching earlier.  This patch revealed just one case of this "bug", in ObjectPropertyConditionSet.cpp's generateConditions().  More generally, those IC helper functions that use isProxy() don't make it obvious that they make this assumption, so I think that the new behavior is safer.

BTW here are all the callers of this method:

ObjectPropertyConditionSet.cpp's generateConditions(): uses it to determine if we need to bail out of an effectless prototype chain walk.  Before I fixed isProxy(), this would think that an ES6 proxy was simply the end of the prototype chain.

PolyProtoAccessChain::create(): ditto.  This is another effectless prototype chain walk that needs to know that it's gotta bail on ES6 proxies.  Note that this function also defended itself by checking if propertyAccessesAreCacheable, which returns false for ES6 proxies.

normalizePrototypeChain: ditto.  This is the ancient version of generateConditions(), for the old style of prototype chain caching, that is currently only used in a few places (I think some for-in optimization).

Overall, I think that this means that Structure::isProxy() only meant "proxy that isn't an ES6 proxy" in our imaginations.  The code was already OK with the new meaning.

[Filip Pizlo]

Created attachment 340703 [details]
the patch

[Filip Pizlo]

Created attachment 340704 [details]
patch for landing

[EWS Watchlist]

Attachment 340704 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/Repatch.h:48:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.cpp:75:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:455:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:456:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITOperations.h:457:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:81:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/jit/JITInlineCacheGenerator.h:135:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:60:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/InstanceOfAccessCase.cpp:61:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 10 in 58 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

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