Bug 163226 - [JSC][DOMJIT] IC should have a way to handle megamorphic DOM accessor calls
Summary: [JSC][DOMJIT] IC should have a way to handle megamorphic DOM accessor calls
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
: 163864 (view as bug list)
Depends on: 163005 163223 163716
Blocks: 162544 164074 162980
  Show dependency treegraph
 
Reported: 2016-10-10 11:19 PDT by Yusuke Suzuki
Modified: 2018-02-14 10:38 PST (History)
15 users (show)

See Also:


Attachments
Patch (16.05 KB, patch)
2016-10-12 19:17 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
WIP (46.68 KB, patch)
2016-10-17 21:47 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
WIP (63.50 KB, patch)
2016-10-18 21:24 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
WIP (66.03 KB, patch)
2016-10-19 00:14 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
WIP (101.46 KB, patch)
2016-10-20 16:28 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
WIP: But maybe, it should work! (105.65 KB, patch)
2016-10-20 18:07 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
It should work. But still tuning many things... (113.04 KB, patch)
2016-10-21 01:25 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (148.96 KB, patch)
2016-10-21 22:34 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (148.49 KB, patch)
2016-10-21 22:46 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (148.19 KB, patch)
2016-10-21 22:54 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (148.60 KB, patch)
2016-10-24 20:19 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (149.66 KB, patch)
2016-10-24 20:32 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (187.30 KB, patch)
2016-10-25 12:04 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (188.37 KB, patch)
2016-10-25 13:37 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (181.38 KB, patch)
2016-10-28 17:11 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (180.61 KB, patch)
2016-10-28 17:18 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (182.17 KB, patch)
2016-10-28 18:15 PDT, Yusuke Suzuki
beidson: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2016-10-10 11:19:12 PDT
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").
Comment 1 Filip Pizlo 2016-10-10 13:12:29 PDT
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?
Comment 2 Yusuke Suzuki 2016-10-10 13:30:04 PDT
(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.
Comment 3 Filip Pizlo 2016-10-10 13:57:55 PDT
(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.
Comment 4 Yusuke Suzuki 2016-10-10 17:29:53 PDT
(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.
Comment 5 Yusuke Suzuki 2016-10-10 19:02:00 PDT
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.
Comment 6 Yusuke Suzuki 2016-10-10 19:03:08 PDT
(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.
Comment 7 Filip Pizlo 2016-10-10 19:07:09 PDT
(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.
Comment 8 Saam Barati 2016-10-11 15:28:00 PDT
(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?
Comment 9 Yusuke Suzuki 2016-10-11 15:49:41 PDT
(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.
Comment 10 Yusuke Suzuki 2016-10-11 22:34:44 PDT
*** Bug 163223 has been marked as a duplicate of this bug. ***
Comment 11 Yusuke Suzuki 2016-10-12 19:17:15 PDT
Created attachment 291440 [details]
Patch

WIP
Comment 12 Yusuke Suzuki 2016-10-17 21:47:11 PDT
Created attachment 291917 [details]
WIP
Comment 13 Yusuke Suzuki 2016-10-18 21:24:53 PDT
Created attachment 292035 [details]
WIP
Comment 14 Yusuke Suzuki 2016-10-19 00:14:06 PDT
Created attachment 292053 [details]
WIP
Comment 15 Yusuke Suzuki 2016-10-20 16:28:54 PDT
Created attachment 292278 [details]
WIP
Comment 16 Yusuke Suzuki 2016-10-20 18:07:32 PDT
Created attachment 292300 [details]
WIP: But maybe, it should work!
Comment 17 Yusuke Suzuki 2016-10-21 01:25:15 PDT
Created attachment 292331 [details]
It should work. But still tuning many things...
Comment 18 Yusuke Suzuki 2016-10-21 15:49:08 PDT
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
Comment 19 Yusuke Suzuki 2016-10-21 22:34:31 PDT
Created attachment 292465 [details]
Patch
Comment 20 WebKit Commit Bot 2016-10-21 22:36:54 PDT
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.
Comment 21 Yusuke Suzuki 2016-10-21 22:46:30 PDT
Created attachment 292468 [details]
Patch
Comment 22 WebKit Commit Bot 2016-10-21 22:49:28 PDT
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.
Comment 23 Yusuke Suzuki 2016-10-21 22:54:00 PDT
Created attachment 292472 [details]
Patch
Comment 24 WebKit Commit Bot 2016-10-21 22:56:44 PDT
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.
Comment 25 Yusuke Suzuki 2016-10-24 16:42:19 PDT
Updating the patch with https://trac.webkit.org/changeset/207787.
Comment 26 Yusuke Suzuki 2016-10-24 19:08:20 PDT
Comment on attachment 292472 [details]
Patch

Updating the patch with the effects one.
Comment 27 Yusuke Suzuki 2016-10-24 20:19:59 PDT
Created attachment 292707 [details]
Patch
Comment 28 Yusuke Suzuki 2016-10-24 20:22:09 PDT
OK, ready!
Comment 29 WebKit Commit Bot 2016-10-24 20:22:53 PDT
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.
Comment 30 Yusuke Suzuki 2016-10-24 20:32:40 PDT
Created attachment 292710 [details]
Patch
Comment 31 WebKit Commit Bot 2016-10-24 20:34:22 PDT
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.
Comment 32 Yusuke Suzuki 2016-10-25 12:04:22 PDT
Created attachment 292797 [details]
Patch
Comment 33 Yusuke Suzuki 2016-10-25 13:37:30 PDT
Created attachment 292811 [details]
Patch
Comment 34 WebKit Commit Bot 2016-10-25 13:39:17 PDT
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.
Comment 35 Yusuke Suzuki 2016-10-25 13:54:46 PDT
*** Bug 163864 has been marked as a duplicate of this bug. ***
Comment 36 Yusuke Suzuki 2016-10-28 16:35:18 PDT
Other patch changes things. I'll just rebaseline this patch.
Comment 37 Yusuke Suzuki 2016-10-28 17:11:29 PDT
Created attachment 293250 [details]
Patch

Rebaselined
Comment 38 WebKit Commit Bot 2016-10-28 17:14:24 PDT
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.
Comment 39 Yusuke Suzuki 2016-10-28 17:18:17 PDT
Created attachment 293252 [details]
Patch
Comment 40 WebKit Commit Bot 2016-10-28 17:20:35 PDT
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.
Comment 41 Yusuke Suzuki 2016-10-28 18:15:55 PDT
Created attachment 293264 [details]
Patch
Comment 42 WebKit Commit Bot 2016-10-28 18:19:34 PDT
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.
Comment 43 Yusuke Suzuki 2016-10-28 22:09:45 PDT
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.
Comment 44 Yusuke Suzuki 2016-10-28 22:34:10 PDT
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%.
Comment 45 Yusuke Suzuki 2016-10-28 22:35:41 PDT
(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.)
Comment 46 Ryosuke Niwa 2016-10-28 22:58:26 PDT
(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...
Comment 47 Yusuke Suzuki 2016-10-28 23:30:35 PDT
(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
Comment 48 Yusuke Suzuki 2016-10-28 23:33:16 PDT
We should optimize it in some way.
Super rough idea I've come up with is offering a patchpoint for NodeList index access.
Comment 49 Ryosuke Niwa 2016-10-29 00:11:25 PDT
(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 :(
Comment 50 Yusuke Suzuki 2016-10-31 14:43:36 PDT
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.
Comment 51 Brady Eidson 2018-02-14 10:38:16 PST
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.