Bug 163226 – [JSC][DOMJIT] IC should have a way to handle megamorphic DOM accessor calls

• Component: JavaScriptCore
• Status: NEW

[Yusuke Suzuki]

This inlining is super effective if we run some microbenchmarks. However, Dromaeo does not
show so much performance benefit. This is because Dromaeo's dom-traverse.html is super
polymorphic call site where JSC gives up optimization. For example, in the following
dromaeo's benchmark, we can see so various DOM nodes at the `cur.firstChild` site, like,
HTMLDivElement, HTMLAnchorElement, Text, Comment etc. JSC gives up optimization since we
encounter so many Structures. This should be optimized since they share the large part of
prototype-chain and they hit the exactly same CustomGetter, Node.prototype.firstChild.

               test( "firstChild", function(){
                   var nodes = document.body.childNodes, nl = nodes.length;

                   for ( var i = 0; i < num; i++ ) {
                       for ( var j = 0; j < nl; j++ ) {
                           var cur = nodes[j];
                           while ( cur )
                               cur = cur.firstChild;
                           ret = cur;
                       }
                   }
               });

This is unfortunate because this call site has very interesting features.

1. All the look up hits the exact same accessor of the same object (Node.prototype).

This call site hits the same Node.prototype.firstChild custom accessor. Inlining has a chance to optimize it.

2. Structure chain of polymorphic ones are very very similar.

While we see various Structures at this call site, their ancestors are quite similar.

HTMLDivElement -> HTMLDivElement.prototype -> HTMLElement.prototype -> Element.prototype -> Node.prototype -> ...
HTMLAnchorElement -> HTMLAnchorElement.prototype -> HTMLElement.prototype -> Element.prototype -> Node.prototype -> ...
Text -> Text.prototype -> CharacterData.prototype -> Node.prototype -> ...

We can ensure that Node.prototype, Element.prototype, and HTMLElement.prototype are not changed by using watchpoint. And typically they are not changed so frequently.
Caching Structures of HTMLDivElement, HTMLAnchorElement, and Text is the current design. But it does not work because there are so many structures.

So, what we should do is ensuring non watched Structures (e.g. HTMLDivElement -> HTMLDivElement.prototype) does not have the property with the specific name ("firstChild").

After discussing with rniwa and keith_miller, currently, I'm planning to add a bloom filter for each Structure.
This filter says "This Structure must not have a property with the specific name".
And by using this, we can query the given Structure chain does not have a property with the specific name ("firstChild").

[Filip Pizlo]

Would the fast path code consult the bloom filter?  That seems like a lot of work!

I wonder if we can watchpoint this.  What if we allowed DOM classes to maintain a per-global-object mapping from property name to WatchpointSet.  If a property is in the set, then if you try to add a property to a DOM node, then you will have to first fire the watchpoint set.  This is effectively the inverse of the impure property map.

I like this because:
- Amazing symmetry to the impure property map.
- Zero additional checks.

What do you think?

[Yusuke Suzuki]

(In reply to comment #1)
> Would the fast path code consult the bloom filter?  That seems like a lot of
> work!
> 
> I wonder if we can watchpoint this.  What if we allowed DOM classes to
> maintain a per-global-object mapping from property name to WatchpointSet. 
> If a property is in the set, then if you try to add a property to a DOM
> node, then you will have to first fire the watchpoint set.  This is
> effectively the inverse of the impure property map.
> 
> I like this because:
> - Amazing symmetry to the impure property map.
> - Zero additional checks.
> 
> What do you think?

Great! I considered about several edge cases. And I think your solution could work!

1. __proto__ change case

var obj = { firstChild: 42 };
obj.__proto__ = Node.prototype;

In this case, CheckDOM should reject this object since it is not a DOM node.

2. Overriding

When overriding occurs, we fire the watchpoint.
And in this case, if the application overrides these accessors, they do not receive any perf benefit.
But it seems super rare. Custom Element and polymer may add a new accessor, but it may not override the existing very basic ones. (firstChild etc.)
And even if they do that, their performance become the same to the current performance.

[Filip Pizlo]

(In reply to comment #2)
> (In reply to comment #1)
> > Would the fast path code consult the bloom filter?  That seems like a lot of
> > work!
> > 
> > I wonder if we can watchpoint this.  What if we allowed DOM classes to
> > maintain a per-global-object mapping from property name to WatchpointSet. 
> > If a property is in the set, then if you try to add a property to a DOM
> > node, then you will have to first fire the watchpoint set.  This is
> > effectively the inverse of the impure property map.
> > 
> > I like this because:
> > - Amazing symmetry to the impure property map.
> > - Zero additional checks.
> > 
> > What do you think?
> 
> Great! I considered about several edge cases. And I think your solution
> could work!
> 
> 1. __proto__ change case
> 
> var obj = { firstChild: 42 };
> obj.__proto__ = Node.prototype;
> 
> In this case, CheckDOM should reject this object since it is not a DOM node.

Right - no problem there.

> 
> 2. Overriding
> 
> When overriding occurs, we fire the watchpoint.
> And in this case, if the application overrides these accessors, they do not
> receive any perf benefit.
> But it seems super rare. Custom Element and polymer may add a new accessor,
> but it may not override the existing very basic ones. (firstChild etc.)
> And even if they do that, their performance become the same to the current
> performance.

Maybe we should add both the bloom filter and the watchpoints!  The bloom filter would help in that corner case, and dampen the fall off the perf cliff.

[Yusuke Suzuki]

(In reply to comment #1)
> Would the fast path code consult the bloom filter?  That seems like a lot of
> work!
> 
> I wonder if we can watchpoint this.  What if we allowed DOM classes to
> maintain a per-global-object mapping from property name to WatchpointSet. 

Do you mean that JSGlobalObject (/JSDOMGlobalObject) will have a map [property name] => [watchpoint set] and all the DOM prototypes (like HTMLDivElement.prototype) will maintain
this one mapping in JSGlobalObject, right?

> If a property is in the set, then if you try to add a property to a DOM
> node, then you will have to first fire the watchpoint set.  This is
> effectively the inverse of the impure property map.
> 
> I like this because:
> - Amazing symmetry to the impure property map.
> - Zero additional checks.
> 
> What do you think?

My current possible design is (rough one, there are a lot of missing ones I think).

JSDOMGlobalObject has a mapping, propertyName -> [ClassInfo, WatchpointSet]. And JSDOMWrappers maintain this structure.
For example, there is HTMLDivElement.prototype.
And when we perform ::put onto this with "firstChild".

1. we query the above mapping with propertyName and find a pair [JSNode::info(), WatchpointSet].
2. we perform HTMLDivElement::info()->inherits(JSNode::info()). If the result is true, we fire watchpoint set.

[Yusuke Suzuki]

The problem of the design I describe is the following case,

var div = document.createElement('div');
div.__proto__ = Object.create(div.__proto__);  // insert an object in prototype chain.
Object.defineProperty(div.__proto__, "firstChild", {
    value: 42
});

As a first step, we can just fire the watchpoint for that case.

[Yusuke Suzuki]

(In reply to comment #5)
> As a first step, we can just fire the watchpoint for that case.

When storing non-specially-handled-object to __proto__ of Node (div), give up optimization.

[Filip Pizlo]

(In reply to comment #4)
> (In reply to comment #1)
> > Would the fast path code consult the bloom filter?  That seems like a lot of
> > work!
> > 
> > I wonder if we can watchpoint this.  What if we allowed DOM classes to
> > maintain a per-global-object mapping from property name to WatchpointSet. 
> 
> Do you mean that JSGlobalObject (/JSDOMGlobalObject) will have a map
> [property name] => [watchpoint set] and all the DOM prototypes (like
> HTMLDivElement.prototype) will maintain
> this one mapping in JSGlobalObject, right?

That's what I was thinking.

> 
> > If a property is in the set, then if you try to add a property to a DOM
> > node, then you will have to first fire the watchpoint set.  This is
> > effectively the inverse of the impure property map.
> > 
> > I like this because:
> > - Amazing symmetry to the impure property map.
> > - Zero additional checks.
> > 
> > What do you think?
> 
> My current possible design is (rough one, there are a lot of missing ones I
> think).
> 
> JSDOMGlobalObject has a mapping, propertyName -> [ClassInfo, WatchpointSet].
> And JSDOMWrappers maintain this structure.
> For example, there is HTMLDivElement.prototype.
> And when we perform ::put onto this with "firstChild".
> 
> 1. we query the above mapping with propertyName and find a pair
> [JSNode::info(), WatchpointSet].
> 2. we perform HTMLDivElement::info()->inherits(JSNode::info()). If the
> result is true, we fire watchpoint set.

Yeah!  You're right that we can key this by any of the following.  By suggesting a propertyName->set map in global object, I was effectively suggesting to key the sets by (propertyName, globalObject).  But we can use any of these:

propertyName - i.e. a global propertyName->set map.
(propertyName, globalObject) - i.e. what I suggested.
(propertyName, globalObject, classInfo) - i.e. what you suggested.
(propertyName, classInfo) - interesting!
classInfo - super cheap
(classInfo, globalObject) - probably not better than (propertyName, globalObject) but who knows.

Note that if you want to actually key by (propertyName, classInfo) then I suggest making that they key, rather than having a propertyName->[(classInfo, set)] mapping, since the latter requires a O(n) search to find your classInfo.  Or a second hash lookup.

I don't want to bikeshed this on your behalf though.  I just mean to say that there are a lot of options and none of them are obviously bad.

[Saam Barati]

(In reply to comment #5)
> The problem of the design I describe is the following case,
> 
> var div = document.createElement('div');
> div.__proto__ = Object.create(div.__proto__);  // insert an object in
> prototype chain.
> Object.defineProperty(div.__proto__, "firstChild", {
>     value: 42
> });
> 
> As a first step, we can just fire the watchpoint for that case.

Will every put in the system need to be aware of this mapping?

If so, how do we make it easy to add new code without forgetting to check if we need to fire a watchpoint?

[Yusuke Suzuki]

(In reply to comment #8)
> (In reply to comment #5)
> > The problem of the design I describe is the following case,
> > 
> > var div = document.createElement('div');
> > div.__proto__ = Object.create(div.__proto__);  // insert an object in
> > prototype chain.
> > Object.defineProperty(div.__proto__, "firstChild", {
> >     value: 42
> > });
> > 
> > As a first step, we can just fire the watchpoint for that case.
> 
> Will every put in the system need to be aware of this mapping?
> 
> If so, how do we make it easy to add new code without forgetting to check if
> we need to fire a watchpoint?

No. Only objects that is used for DOMXXX.prototype and DOMXXX instance should be aware of this.
DOMXXX.prototype thing is super easy, since it is now generated by IDL generator!
And for DOMXXX instance case, I think putting the check in JSDOMWrapper::put is fine.

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Created attachment 291440 [details]
Patch

WIP

[Yusuke Suzuki]

Created attachment 291917 [details]
WIP

[Yusuke Suzuki]

Created attachment 292035 [details]
WIP

[Yusuke Suzuki]

Created attachment 292053 [details]
WIP

[Yusuke Suzuki]

Created attachment 292278 [details]
WIP

[Yusuke Suzuki]

Created attachment 292300 [details]
WIP: But maybe, it should work!

[Yusuke Suzuki]

Created attachment 292331 [details]
It should work. But still tuning many things...

[Yusuke Suzuki]

in jslib-event-jquery.html

Baseline:
jQuery - bind:Runs -> [1146.4063886424135, 1175, 1275, 1328, 1396] runs/s
    mean: 1264.0812777284827 runs/s
    median: 1275 runs/s
    stdev: 104.1516015260607 runs/s
    min: 1146.4063886424135 runs/s
    max: 1396 runs/s

jQuery - trigger:Runs -> [0.1314060446780552, 0.13313806417254692, 0.13426423200859292, 0.13683634373289547, 0.13785497656465398] runs/s
    mean: 0.13469993223134888 runs/s
    median: 0.13426423200859292 runs/s
    stdev: 0.0026456341668879588 runs/s
    min: 0.1314060446780552 runs/s
    max: 0.13785497656465398 runs/s

jQuery - unbind x10:Runs -> [0.5449591280653951, 1408, 1412, 1421, 1443] runs/s
    mean: 1136.9089918256132 runs/s
    median: 1412 runs/s
    stdev: 635.3912217112367 runs/s
    min: 0.5449591280653951 runs/s
    max: 1443 runs/s


:Runs -> [0.10586634766918275, 0.13311039487748974, 0.1342373317516647, 0.1368090728657298, 0.13782819880910824] runs/s
    mean: 0.12957026919463505 runs/s
    median: 0.1342373317516647 runs/s
    stdev: 0.013386406711219075 runs/s
    min: 0.10586634766918275 runs/s
    max: 0.13782819880910824 runs/s

Patched:

jQuery - bind:Runs -> [1108, 1216, 1264, 1298.9076464746772, 1343] runs/s
    mean: 1245.9815292949356 runs/s
    median: 1264 runs/s
    stdev: 90.10579524877222 runs/s
    min: 1108 runs/s
    max: 1343 runs/s

jQuery - trigger:Runs -> [0.13457139012245997, 0.13719303059404583, 0.13738150844896277, 0.13941168269901016, 0.13997760358342665] runs/s
    mean: 0.1377070430895811 runs/s
    median: 0.13738150844896277 runs/s
    stdev: 0.0021368049941601387 runs/s
    min: 0.13457139012245997 runs/s
    max: 0.13997760358342665 runs/s

jQuery - unbind x10:Runs -> [0.5813953488372093, 2068, 2083, 2091, 2108] runs/s
    mean: 1670.1162790697676 runs/s
    median: 2083 runs/s
    stdev: 933.4099327132914 runs/s
    min: 0.5813953488372093 runs/s
    max: 2108 runs/s


:Runs -> [0.10926689800743725, 0.1371684549258067, 0.13735752011477645, 0.13938742898026402, 0.13995372319696944] runs/s
    mean: 0.13262680504505076 runs/s
    median: 0.13735752011477645 runs/s
    stdev: 0.013115651633778438 runs/s
    min: 0.10926689800743725 runs/s
    max: 0.13995372319696944 runs/s

[Yusuke Suzuki]

Created attachment 292465 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 66 files


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

[Yusuke Suzuki]

Created attachment 292468 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 66 files


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

[Yusuke Suzuki]

Created attachment 292472 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 66 files


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

[Yusuke Suzuki]

Updating the patch with https://trac.webkit.org/changeset/207787.

[Yusuke Suzuki]

Comment on attachment 292472 [details]
Patch

Updating the patch with the effects one.

[Yusuke Suzuki]

Created attachment 292707 [details]
Patch

[Yusuke Suzuki]

OK, ready!

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 66 files


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

[Yusuke Suzuki]

Created attachment 292710 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 66 files


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

[Yusuke Suzuki]

Created attachment 292797 [details]
Patch

[Yusuke Suzuki]

Created attachment 292811 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 71 files


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

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Other patch changes things. I'll just rebaseline this patch.

[Yusuke Suzuki]

Created attachment 293250 [details]
Patch

Rebaselined

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 68 files


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

[Yusuke Suzuki]

Created attachment 293252 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 67 files


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

[Yusuke Suzuki]

Created attachment 293264 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:44:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:45:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:47:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:49:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/JavaScriptCore/bytecode/GetByIdStatus.h:50:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 6 in 67 files


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

[Yusuke Suzuki]

Comment on attachment 293264 [details]
Patch

While some of benchmark shows significant improvement, dom-traverse does not show performance benefit. But in dom-traverse, we actually emit the metamorphic ones. We need to check further to validate the results.

[Yusuke Suzuki]

Comment on attachment 293264 [details]
Patch

Ah, OK. I've found the reason.
In dom-traverse, we already drop many overheads. So the largest bottle neck become changed. From firstChild operation, to accesses to childNode's nodes[i] access.
If we change these code to array by using Array.prototype.slice, we found that this patch improves the performance 23%.

[Yusuke Suzuki]

(In reply to comment #44)
> Comment on attachment 293264 [details]
> Patch
> 
> Ah, OK. I've found the reason.
> In dom-traverse, we already drop many overheads. So the largest bottle neck
> become changed. From firstChild operation, to accesses to childNode's
> nodes[i] access.
> If we change these code to array by using Array.prototype.slice, we found
> that this patch improves the performance 23%.

And some benchmark shows improvement. I think this is because this access is the largest bottleneck in such a benchmark. But in dom-traverse, now, it's not true. The access already becomes faster (by DOMJIT things.)

[Ryosuke Niwa]

(In reply to comment #44)
> Comment on attachment 293264 [details]
> Patch
> 
> Ah, OK. I've found the reason.
> In dom-traverse, we already drop many overheads. So the largest bottle neck
> become changed. From firstChild operation, to accesses to childNode's
> nodes[i] access.
> If we change these code to array by using Array.prototype.slice, we found
> that this patch improves the performance 23%.

So accessing array is a lot faster than accessing node lists? I guess we somehow need to JIT access to node list? That might be a bit tricky...

[Yusuke Suzuki]

(In reply to comment #46)
> (In reply to comment #44)
> > Comment on attachment 293264 [details]
> > Patch
> > 
> > Ah, OK. I've found the reason.
> > In dom-traverse, we already drop many overheads. So the largest bottle neck
> > become changed. From firstChild operation, to accesses to childNode's
> > nodes[i] access.
> > If we change these code to array by using Array.prototype.slice, we found
> > that this patch improves the performance 23%.
> 
> So accessing array is a lot faster than accessing node lists? I guess we
> somehow need to JIT access to node list? That might be a bit tricky...

Yeah, array access is completely handled in JSC.
It is much faster compared to the NodeList.

If you change `var nodes = document.body.childNodes` part in dom-traverse to
`var nodes = Array.prototype.slice.call(document.body.childNodes)`,

Before:
firstChild:Runs -> [1239, 1249, 1250, 1265, 1279] runs/s
    mean: 1256.4 runs/s
    median: 1250 runs/s
    stdev: 15.678010077812788 runs/s
    min: 1239 runs/s
    max: 1279 runs/s

After:
firstChild:Runs -> [3229, 3232, 3332, 3340, 3512] runs/s
    mean: 3329 runs/s
    median: 3332 runs/s
    stdev: 115.13904637437291 runs/s
    min: 3229 runs/s
    max: 3512 runs/s

[Yusuke Suzuki]

We should optimize it in some way.
Super rough idea I've come up with is offering a patchpoint for NodeList index access.

[Ryosuke Niwa]

(In reply to comment #48)
> We should optimize it in some way.
> Super rough idea I've come up with is offering a patchpoint for NodeList
> index access.

I guess another crazy route to take is implementing NodeList in JS especially for something as simple as childNodes.

One tricky thing there is that we need to somehow cache results & invalidate the cache from DOM code.

Another somewhat annoying problem is that for HTMLCollections & NodeList that don't return all elements & nodes, we may end up creating wrappers for more things :(

[Yusuke Suzuki]

I'll need to take some care of Custom element case.

class DerivedElement extends HTMLElement {
};

let derived = new DerivedElement;
I think this `derived` is

1. Inherits HTMLElement::info() since it is actually created in HTMLElement's constructor
2. But it has the normal object as its [[Prototype]]
3. Without firing the watchpoint.

I'll check it. And if it does not fire the watchpoint, I need to insert some code to fire the watchpoint.

[Brady Eidson]

Comment on attachment 293264 [details]
Patch

Patches that have been up for review since 2016 are almost certainly too stale to be relevant to trunk in their current form.

If this patch is still important please rebase it and post it for review again.

[Yusuke Suzuki]

We should revive this attempt.