Bug 176391 – Implement polymorphic prototypes

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

The plan is to allow object's to store their prototypes in their property storage instead of their structure. This will make code like this faster:
```
function foo() {
    class C { 
        constructor() {
            this.x = 20;
            this.y = 30;
        }
    }
    return new C;
}

function bar(a) { return a.x + a.y; }

let a = [foo(), foo(), foo(), foo(), foo(), foo(), foo(), foo()]
for (let I = 0; I < 1000; I++)
    for (let x of a) bar(x);
```

The reason is each invocation of foo will create a new C class. Each result of foo() will end up with an object with a different structure, each with a different prototype, each pointing to a separate C.protoype field. That means that bar's ICs for ".x" and ".y" will become megamorphic and go to the slow path. However, the structure for the resulting object in each invocation of foo() will only be different in their prototype field. If we decided to store the prototype of the object in its property storage instead of on its structure, the C constructor can always return the same structure. This will make the inline caches in bar monomorphic.

This will probably require a lot of changes elsewhere in the system. I'll probably need to rewrite object property condition in some way. We'll also need to take precautions to not go poly-proto under every circumstance. For example, consider this program:

```
function foo() {
    return function C() { }
}
```
let c1 = foo();
c1.prototype = {x};

let c2 = foo();
c2.prototype = {y};

.... and so on
```

Since c1, c2, etc, will all produce structure's with truly different prototypes, it makes sense not to go poly proto under such circumstances. I think this means we'll want the structure representing "function.prototype" to have an InferredValue that infers its constructor. We only go poly-proto if the inferred value is invalidated. In the above program, it won't be invalidated, since c1/c2.protoytpe will point to object's of different structures. However, in the first program, the prototype for each C will have the same structure, leading to the inferred value for C.prototype's structure to be invalidated since it'll see more than one constructor.

[Keith Miller]

Nice! I think I also have a bug open for this. I'll dup to yours.

[Saam Barati]

(In reply to Keith Miller from comment #1)
> Nice! I think I also have a bug open for this. I'll dup to yours.

Thanks. I thought we might have one already open, but I couldn't find it.

[Keith Miller]

(In reply to Saam Barati from comment #0)
> The plan is to allow object's to store their prototypes in their property
> storage instead of their structure.

Side question: Does it make sense to ensure that the prototype is always in inline storage, at least for final objects?

[Saam Barati]

(In reply to Keith Miller from comment #3)
> (In reply to Saam Barati from comment #0)
> > The plan is to allow object's to store their prototypes in their property
> > storage instead of their structure.
> 
> Side question: Does it make sense to ensure that the prototype is always in
> inline storage, at least for final objects?

Probably no for the final objects with non-poly proto, but probably yes for the final objects that have poly proto.

[Saam Barati]

(In reply to Saam Barati from comment #4)
> (In reply to Keith Miller from comment #3)
> > (In reply to Saam Barati from comment #0)
> > > The plan is to allow object's to store their prototypes in their property
> > > storage instead of their structure.
> > 
> > Side question: Does it make sense to ensure that the prototype is always in
> > inline storage, at least for final objects?
> 
> Probably no for the final objects with non-poly proto, but probably yes for
> the final objects that have poly proto.

Actually, this may require more thought. It obviously depends on the access patterns for this type of object. I'll need to think about this a bit more.

[Keith Miller]

*** Bug 150983 has been marked as a duplicate of this bug. ***

[Filip Pizlo]

*** Bug 174523 has been marked as a duplicate of this bug. ***

[Filip Pizlo]

(In reply to Keith Miller from comment #3)
> (In reply to Saam Barati from comment #0)
> > The plan is to allow object's to store their prototypes in their property
> > storage instead of their structure.
> 
> Side question: Does it make sense to ensure that the prototype is always in
> inline storage, at least for final objects?

No because we don't want to waste the space.

[Saam Barati]

Created attachment 320086 [details]
WIP

it begins

[Saam Barati]

Created attachment 321144 [details]
WIP

[Saam Barati]

Created attachment 321157 [details]
WIP

[Saam Barati]

Created attachment 321235 [details]
WIP

[Saam Barati]

Created attachment 321280 [details]
WIP

I'm about to start hacking on our ICs.

[Saam Barati]

Created attachment 321301 [details]
WIP

[Saam Barati]

Created attachment 321360 [details]
WIP

It did a poly proto load, but things are quite hacky ATM. I need to find the right abstractions for things.

[Saam Barati]

Created attachment 321378 [details]
patch

It did a poly proto load.

[Saam Barati]

Created attachment 321396 [details]
WIP

I'm starting to think of splitting this work in two patches.

The first patch:
- implements poly proto vs mono proto constructor inference, along with detecting when to transition a create_this to make a poly proto object
- teaches the runtime about poly proto, and wires it through everywhere.
- It'll treat ObjectPropertyConditionSet and PrototypeAccessChain as mutually exclusive data structures to consult depending on poly proto state
- loads with poly proto anywhere in the chain will just emit GetById in the DFG
- we'll do the stupid thing where we load each prototype (even if mono proto), and check that the structure is what we expect it to be, inside AccessCase

And then in the second patch:
- Merge PrototypeAccessChain and ObjectPropertyConditionSet. The reason for this is: you may have mono proto structures in the middle of a prototype access that has poly proto somewhere in it. We can use normal OPC for the prototypes pointed to by mono proto structures since we know the prototype object won't change. There is no fundamental need to do the dumb thing stated above. I think if I pace things out this way, it'll be a nice succession of patches, without having to rewrite code.
- Teach the DFG about this hybrid data structure. We should still be able to emit GetByOffset for prototype loads. The DFG will learn to dynamically load the prototype in the case of poly proto, and emit necessary StructureCheck on it.

Anybody have thoughts on this plan?

[Saam Barati]

Created attachment 321452 [details]
WIP

[Saam Barati]

Created attachment 321485 [details]
WIP

It can run ARES6

[Saam Barati]

Created attachment 321504 [details]
WIP

instanceof works again

[Saam Barati]

Created attachment 321526 [details]
WIP

many tests pass

[Saam Barati]

Created attachment 321528 [details]
WIP

[Saam Barati]

Created attachment 321559 [details]
WIP

It passes almost all (or maybe all) JSC stress tests. It's time to start cleaning things up, auditing some more code, and writing a lot of tests.

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:392:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:393:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:564:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:631:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/PrototypeAccessChain.cpp:100:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:292:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:247:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:249:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/Structure.cpp:302:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/Structure.cpp:312:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3253:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:45:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:46:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:47:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:48:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:69:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:74:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/StructureChain.h:65:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.h:51:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.h:51:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.h:77:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:250:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 25 in 61 files


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

[Build Bot]

Comment on attachment 321559 [details]
WIP

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

New failing tests:
ChakraCore.yaml/ChakraCore/test/StackTrace/ErrorPrototype.js.default
typeProfiler.yaml/typeProfiler/deltablue-for-of.js.ftl-no-cjit-type-profiler
ChakraCore.yaml/ChakraCore/test/ControlFlow/forInObjectWithPrototype.js.default
typeProfiler.yaml/typeProfiler/deltablue-for-of.js.ftl-type-profiler

[Build Bot]

Comment on attachment 321559 [details]
WIP

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

New failing tests:
imported/w3c/web-platform-tests/dom/nodes/Document-createElement.html
imported/w3c/web-platform-tests/dom/nodes/Document-createElementNS.html

[Build Bot]

Created attachment 321574 [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

[Build Bot]

Comment on attachment 321559 [details]
WIP

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

New failing tests:
imported/w3c/web-platform-tests/dom/nodes/Document-createElement.html
imported/w3c/web-platform-tests/dom/nodes/Document-createElementNS.html

[Build Bot]

Created attachment 321576 [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

[Saam Barati]

Created attachment 321588 [details]
WIP

More stuff.

[Saam Barati]

Created attachment 321659 [details]
WIP

making gets that go through primitives work.

[Saam Barati]

Created attachment 321660 [details]
WIP

make sure we hash-cons AccessCases doing a poly proto load

[Saam Barati]

Created attachment 321666 [details]
WIP

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:52:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:397:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:398:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:410:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:412:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:414:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:574:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:641:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:264:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:265:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:271:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:272:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:270:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:276:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:277:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/jit/Repatch.cpp:296:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:247:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:248:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:249:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:250:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp:252:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/Structure.cpp:302:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/Structure.cpp:312:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/JSObject.cpp:1636:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3253:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:43:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:48:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:52:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:53:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:54:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:55:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:76:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:81:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/runtime/StructureChain.h:65:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.h:51:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.h:51:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.h:77:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/dfg/DFGOperations.cpp:250:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 40 in 69 files


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

[Build Bot]

Comment on attachment 321666 [details]
WIP

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

New failing tests:
typeProfiler.yaml/typeProfiler/deltablue-for-of.js.ftl-no-cjit-type-profiler
typeProfiler.yaml/typeProfiler/deltablue-for-of.js.ftl-type-profiler

[Build Bot]

Comment on attachment 321666 [details]
WIP

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

New failing tests:
imported/w3c/web-platform-tests/dom/nodes/Document-createElement.html
imported/w3c/web-platform-tests/dom/nodes/Document-createElementNS.html

[Build Bot]

Created attachment 321668 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

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

[Build Bot]

Comment on attachment 321666 [details]
WIP

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

New failing tests:
imported/w3c/web-platform-tests/dom/nodes/Document-createElement.html
imported/w3c/web-platform-tests/dom/nodes/Document-createElementNS.html

[Build Bot]

Created attachment 321669 [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]

Created attachment 321785 [details]
WIP

[Saam Barati]

Created attachment 321787 [details]
WIP

[Saam Barati]

Created attachment 321897 [details]
WIP

[Saam Barati]

Created attachment 321908 [details]
WIP

[Saam Barati]

Created attachment 321934 [details]
WIP

I'm forcing all create_this things to be poly proto as a testing strategy. Should help find some bugs

[Saam Barati]

Created attachment 321939 [details]
WIP

[Saam Barati]

Created attachment 322048 [details]
WIP

[Saam Barati]

Created attachment 322053 [details]
WIP

We now do caching for InHit/InMiss. Going to write caching for puts now.

[Saam Barati]

Created attachment 322057 [details]
WIP

poly proto setters may be done. I need to still test custom value setters/getters and custom accessor setters/getters.

[Saam Barati]

Created attachment 322058 [details]
WIP

some more microbenchmarks.

[Saam Barati]

Created attachment 322152 [details]
WIP

Now with tests for custom accessors / custom value getter/setters. I think we may have had zero tests for this inside jsc stress tests.

[Saam Barati]

Created attachment 322154 [details]
WIP

Really close to being ready for review. I think I just need to write a changelog and verify a couple more things.

[Saam Barati]

Created attachment 322160 [details]
WIP

Almost done I think, just verifying performance is faster on microbenchmarks. It is indeed significantly faster. I need to run benchmark suites now.

[Saam Barati]

Created attachment 322161 [details]
WIP

[Saam Barati]

Octane's delta blue falls right into our poly-proto heuristic:
```
Object.defineProperty(Object.prototype, "inheritsFrom", {
  
  value: function (shuper) {
    function Inheriter() { }
    Inheriter.prototype = shuper.prototype;
    this.prototype = new Inheriter();
    this.superConstructor = shuper;
  }
});
```
Poly proto happens when the same "shuper" is passed in, which happens frequently.

This patch makes that test 65% slower, which leads to a 4% regression on Octane. Also likely leading to a JetStream regression. I'm not sure if I should bother even changing the poly-proto heuristic. Perhaps the right way to land this patch is to just integrate it with OPC set.

[Saam Barati]

Kraken and Octane besides delta blue look neutral. This patch may also be a 1-2% ARES6 speedup.

[Saam Barati]

(In reply to Saam Barati from comment #54)
> Octane's delta blue falls right into our poly-proto heuristic:
> ```
> Object.defineProperty(Object.prototype, "inheritsFrom", {
>   
>   value: function (shuper) {
>     function Inheriter() { }
>     Inheriter.prototype = shuper.prototype;
>     this.prototype = new Inheriter();
>     this.superConstructor = shuper;
>   }
> });
> ```
> Poly proto happens when the same "shuper" is passed in, which happens
> frequently.
> 
> This patch makes that test 65% slower, which leads to a 4% regression on
> Octane. Also likely leading to a JetStream regression. I'm not sure if I
> should bother even changing the poly-proto heuristic. Perhaps the right way
> to land this patch is to just integrate it with OPC set.

Actually I'm not sure if a combined PrototypeAccessChain and OPC set would solve the performance here. We end up with a prototype chain like so:

base (mono proto) => parent (poly proto) => parent^2 (mono proto)
Or we end up with:
base (mono proto) => parent (poly proto) => parent^2 (poly proto)
where we're loading from parent^2

In both cases, if the link in the chain before the thing we're loading from is poly proto, we can't say much about the identity of the thing above it. In this case, the thing before parent^2 is always poly proto, so we can't say anything about parent^2.

[Saam Barati]

I wonder if the solution here is to just emit OPC when the base isn't poly proto. If the base isn't poly proto, but inherits from something poly proto, we're likely going to keep seeing the same prototype chain.

[Saam Barati]

Created attachment 322286 [details]
WIP

The start of a new heuristic of when to go poly proto. Basically how it's going to work is:
we let profiling from ICs tell us when it thinks two structures should become poly proto because they're structurally equivalent sans having the same prototype. Then, we communicate this back to the executable.

[Saam Barati]

Created attachment 322326 [details]
WIP

[Saam Barati]

Seems to be crashing some tests. Will look into why

[Saam Barati]

Created attachment 322353 [details]
WIP

Try it out on EWS. I think it's really close to being ready for review. The last major thing to do is write a changelog.

[Saam Barati]

Created attachment 322354 [details]
WIP

[Saam Barati]

Created attachment 322355 [details]
WIP

rebased

[Build Bot]

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


ERROR: Source/JavaScriptCore/jit/Repatch.cpp:577:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/runtime/StructureChain.h:65:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:584:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:585:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/AccessCase.cpp:586:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:52:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:64:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:65:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:66:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:71:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:73:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:75:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:76:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:79:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.cpp:87:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/bytecode/PrototypeAccessChain.h:45:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/PrototypeAccessChain.h:45:  The parameter name "slot" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/PrototypeAccessChain.h:46:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:52:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.h:51:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.h:51:  The parameter name "globalObject" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/ObjectAllocationProfile.h:77:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:3253:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 25 in 92 files


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

[Build Bot]

Comment on attachment 322355 [details]
WIP

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 322360 [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

[Saam Barati]

Created attachment 322403 [details]
WIP

let's try this on EWS. I still haven't done 32-bit, but that should be really easy.

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:52:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 3 in 92 files


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

[Saam Barati]

Created attachment 322409 [details]
WIP

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:52:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 3 in 92 files


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

[Build Bot]

Comment on attachment 322409 [details]
WIP

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

New failing tests:
imported/w3c/web-platform-tests/html/browsers/the-window-object/apis-for-creating-and-navigating-browsing-contexts-by-name/open-features-tokenization-screenx-screeny.html

[Build Bot]

Created attachment 322438 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

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

[Saam Barati]

Created attachment 322466 [details]
patch

I still need to write the 32-bit parts of this patch. It should be mostly trivial. Anyways, it's ready for review minus 32-bit parts.

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:52:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 3 in 96 files


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

[Saam Barati]

Comment on attachment 322466 [details]
patch

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

> Source/JavaScriptCore/runtime/StructureRareData.h:85
> +    WriteBarrier<InferredValue> m_inferredConstructor;

will remove

[Saam Barati]

Created attachment 322475 [details]
patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:52:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 3 in 94 files


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

[Saam Barati]

Created attachment 322478 [details]
patch

With 32-bit written. I think it works, but I'm testing it locally.

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:52:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 3 in 95 files


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

[Saam Barati]

Created attachment 322479 [details]
patch

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:52:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 3 in 95 files


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

[Saam Barati]

Created attachment 322563 [details]
patch for landing

[Filip Pizlo]

Comment on attachment 322563 [details]
patch for landing

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

> Source/JavaScriptCore/ChangeLog:96
> +        to determine if two structures originated from the same executable. This
> +        pruning will severely limit the chances of getting a hash conflict in practice.
> +
> +        * JavaScriptCore.xcodeproj/project.pbxproj:
> +        * Sources.txt:
> +        * builtins/BuiltinNames.cpp:

Can you include perf data in the changelog?

[Build Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetterSetterAccessCase.h:52:  std::unique_ptr is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/bytecode/ProxyableAccessCase.cpp:35:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 3 in 95 files


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

[Saam Barati]

(In reply to Filip Pizlo from comment #83)
> Comment on attachment 322563 [details]
> patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322563&action=review
> 
> > Source/JavaScriptCore/ChangeLog:96
> > +        to determine if two structures originated from the same executable. This
> > +        pruning will severely limit the chances of getting a hash conflict in practice.
> > +
> > +        * JavaScriptCore.xcodeproj/project.pbxproj:
> > +        * Sources.txt:
> > +        * builtins/BuiltinNames.cpp:
> 
> Can you include perf data in the changelog?

Yeah. I just kicked off a run of run-jsc-benchmarks. After that, I'll collect perf on the microbenchmarks I added.

[Saam Barati]

Created attachment 322594 [details]
traditional perf benchmark results

seems neutral

[Saam Barati]

Created attachment 322601 [details]
patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 322601 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 322601 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 322601 [details]
patch for landing

Clearing flags on attachment: 322601

Committed r222827: <http://trac.webkit.org/changeset/222827>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34804756>

[Yusuke Suzuki]

Oh, wow cool
https://arewefastyet.com/#machine=29&view=single&suite=ares6&subtest=Air-steadyState

[Saam Barati]

(In reply to Yusuke Suzuki from comment #93)
> Oh, wow cool
> https://arewefastyet.com/#machine=29&view=single&suite=ares6&subtest=Air-
> steadyState

👍🏽