Bug 185652

Summary: JSC should have InstanceOf inline caching
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 185663, 185695    
Attachments:
Description Flags
it begins
none
a bit more
none
more
none
a little more
none
close to done
none
maybe it is written
none
and now with tests
none
it compiles
none
it's starting to do things
none
better patch
none
starting to make some sense
none
more
none
fixed more bugs
ews-watchlist: commit-queue-
Archive of layout-test-results from ews112 for mac-sierra
none
Archive of layout-test-results from ews102 for mac-sierra
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
make more tests pass
ews-watchlist: commit-queue-
Archive of layout-test-results from ews117 for mac-sierra
none
fix debug
none
starting to fix 32-bit
none
getting to the bottom of 32-bit test failures
none
the patch
saam: review+
the patch
none
patch for landing none

Description Filip Pizlo 2018-05-15 10:05:59 PDT 
Patch forthcoming.
Comment 1 Filip Pizlo 2018-05-15 10:06:25 PDT 
Created attachment 340417 [details]
it begins
Comment 2 Filip Pizlo 2018-05-15 10:31:50 PDT 
Created attachment 340419 [details]
a bit more
Comment 3 Radar WebKit Bug Importer 2018-05-15 14:24:25 PDT 
<rdar://problem/40271131>
Comment 4 Filip Pizlo 2018-05-15 16:10:00 PDT 
Created attachment 340440 [details]
more
Comment 5 Filip Pizlo 2018-05-15 16:47:28 PDT 
Created attachment 340446 [details]
a little more
Comment 6 Filip Pizlo 2018-05-16 11:00:28 PDT 
Created attachment 340499 [details]
close to done

Just need to do FTL.
Comment 7 Filip Pizlo 2018-05-16 12:01:20 PDT 
Created attachment 340511 [details]
maybe it is written
Comment 8 Filip Pizlo 2018-05-16 12:12:06 PDT 
Created attachment 340512 [details]
and now with tests

I haven't tried compiling it yet but it's written.  At least on 64-bit.
Comment 9 EWS Watchlist 2018-05-16 15:07:28 PDT 
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.
Comment 10 Filip Pizlo 2018-05-16 16:02:31 PDT 
Created attachment 340537 [details]
it compiles
Comment 11 Filip Pizlo 2018-05-16 18:57:47 PDT 
Created attachment 340548 [details]
it's starting to do things
Comment 12 Filip Pizlo 2018-05-16 19:00:34 PDT 
Created attachment 340549 [details]
better patch
Comment 13 Filip Pizlo 2018-05-16 19:34:18 PDT 
Created attachment 340551 [details]
starting to make some sense
Comment 14 Filip Pizlo 2018-05-16 19:35:54 PDT 
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.
Comment 15 Filip Pizlo 2018-05-16 21:58:43 PDT 
Created attachment 340555 [details]
more
Comment 16 Filip Pizlo 2018-05-16 22:25:59 PDT 
Created attachment 340556 [details]
fixed more bugs

It's passing so many tests.
Comment 17 EWS Watchlist 2018-05-17 03:46:01 PDT 
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.
Comment 18 EWS Watchlist 2018-05-17 04:57:53 PDT 
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.
Comment 19 EWS Watchlist 2018-05-17 04:57:54 PDT 
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
Comment 20 EWS Watchlist 2018-05-17 05:02:15 PDT 
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
Comment 21 EWS Watchlist 2018-05-17 05:04:27 PDT 
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
Comment 22 EWS Watchlist 2018-05-17 05:04:28 PDT 
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
Comment 23 EWS Watchlist 2018-05-17 05:38:29 PDT 
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
Comment 24 EWS Watchlist 2018-05-17 05:38:30 PDT 
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
Comment 25 EWS Watchlist 2018-05-17 07:09:12 PDT 
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
Comment 26 EWS Watchlist 2018-05-17 07:09:14 PDT 
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
Comment 27 Filip Pizlo 2018-05-17 08:30:35 PDT 
Created attachment 340585 [details]
make more tests pass
Comment 28 EWS Watchlist 2018-05-17 08:32:30 PDT 
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.
Comment 29 EWS Watchlist 2018-05-17 09:37:32 PDT 
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.
Comment 30 EWS Watchlist 2018-05-17 09:37:34 PDT 
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
Comment 31 EWS Watchlist 2018-05-17 09:53:05 PDT 
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
Comment 32 Filip Pizlo 2018-05-17 11:51:58 PDT 
Created attachment 340616 [details]
fix debug
Comment 33 Filip Pizlo 2018-05-17 14:03:44 PDT 
Created attachment 340636 [details]
starting to fix 32-bit
Comment 34 EWS Watchlist 2018-05-17 14:07:46 PDT 
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.
Comment 35 Filip Pizlo 2018-05-17 14:46:30 PDT 
Created attachment 340649 [details]
getting to the bottom of 32-bit test failures
Comment 36 EWS Watchlist 2018-05-17 14:52:46 PDT 
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.
Comment 37 Filip Pizlo 2018-05-17 16:30:18 PDT 
Created attachment 340665 [details]
the patch
Comment 38 EWS Watchlist 2018-05-17 16:33:02 PDT 
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.
Comment 39 Saam Barati 2018-05-17 16:35:58 PDT 
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
Comment 40 Saam Barati 2018-05-17 18:14:00 PDT 
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.
Comment 41 Filip Pizlo 2018-05-18 09:09:00 PDT 
(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.
Comment 42 Filip Pizlo 2018-05-18 09:54:23 PDT 
Created attachment 340703 [details]
the patch
Comment 43 Filip Pizlo 2018-05-18 09:55:11 PDT 
Created attachment 340704 [details]
patch for landing
Comment 44 EWS Watchlist 2018-05-18 09:58:27 PDT 
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.
Comment 45 Filip Pizlo 2018-05-18 10:30:16 PDT 
Landed in https://trac.webkit.org/changeset/231961/webkit