Bug 78586 - Event dispatching should use the composed shadow DOM tree instead of original DOM tree.
Summary: Event dispatching should use the composed shadow DOM tree instead of original...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hayato Ito
URL:
Keywords:
: 85854 (view as bug list)
Depends on:
Blocks: 59805
  Show dependency treegraph
 
Reported: 2012-02-14 01:12 PST by Hayato Ito
Modified: 2012-06-18 16:00 PDT (History)
5 users (show)

See Also:


Attachments
WIP. concept of new spec. (38.15 KB, patch)
2012-05-07 03:53 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
WIP. Fix an underlining bug. Every existing test should pass now. (36.32 KB, patch)
2012-05-09 19:25 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
WIP. Fix the calculation of adjusted target. Now it considers InsertionPoints correctly. (37.58 KB, patch)
2012-05-10 01:35 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
WIP. New ajusting relatedTarget algorithm. (34.75 KB, patch)
2012-05-10 02:55 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
WIP. Now event dispatching is stopped where target == relatedTarget. (34.46 KB, patch)
2012-05-10 04:06 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
WIP. Find an available relatedTarget from ancestor scope. (34.48 KB, patch)
2012-05-10 04:58 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
WIP. Introduce an EventAncestors class. (48.31 KB, patch)
2012-05-11 01:44 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
WIP. Introduce an EventRelatedTargetAdjuster. (34.45 KB, patch)
2012-05-14 06:43 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Benchmark for event-dispatching. (3.23 KB, text/html)
2012-05-14 06:46 PDT, Hayato Ito
no flags Details
WIP. (35.27 KB, patch)
2012-05-15 05:43 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
modify EventDispatcher for benchmark (2.01 KB, application/octet-stream)
2012-05-15 05:46 PDT, Hayato Ito
no flags Details
ready for reviews (43.24 KB, patch)
2012-05-16 02:25 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
ready for reviews. Refine a changelog. (43.54 KB, patch)
2012-05-16 04:23 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (47.34 KB, patch)
2012-05-16 20:38 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff
Patch for landing (47.34 KB, patch)
2012-05-16 20:40 PDT, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2012-02-14 01:12:23 PST
The spec is here:
https://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#event-dispatch

We should update the event dispatch so that it reflects the composite tree.
Comment 1 Hayato Ito 2012-05-07 03:53:16 PDT
Created attachment 140505 [details]
WIP. concept of new spec.
Comment 2 Hayato Ito 2012-05-07 03:57:13 PDT
As we explained in 2012 WebKit contributors meeting, this is a concept of new implementation of event dispatching.

The spec is not updated yet.

The document we used in the meeting is here:
https://docs.google.com/document/d/1PY7KkTRI-doU9Hew3Mud9JBgjj5xtVqH8sY0Ui305nw/edit
Comment 3 Hayato Ito 2012-05-07 19:11:22 PDT
Hi Dimitri and Shadow DOM guys,

Could you take a look at WIP patch? I won't land this patch as is. It's just a implementation of new algorithm. A final patch should have equivalent results of this WIP patch.

I hope this patch would help Dimitri to update the Event Dispatching spec in Shadow DOM.

Although I'll continue improve this patch, especially the performance, if you have any comments or find any significant fault in the approach, please let me know it.
Comment 4 Dimitri Glazkov (Google) 2012-05-07 20:57:29 PDT
Comment on attachment 140505 [details]
WIP. concept of new spec.

I think the general approach is good. I am sad to see EventContext grow, but it's not something we can avoid.
Comment 5 Hayato Ito 2012-05-07 22:53:28 PDT
*** Bug 85854 has been marked as a duplicate of this bug. ***
Comment 6 Hayato Ito 2012-05-08 00:31:01 PDT
Hi Dimitri,

Okay. Let me proceed using the current approach. Before trying to improve the performance seriously, I'd like to have some benchmark to compare the performance.

AFAIR, you've told me that you used a micro benchmark when you tried to modify event dispatching before. Could you give me the used benchmark if you still had it? I think a micro benchmark is enough for now and it is not difficult to write such a benchmark. But I'd be happy if you gave me that :)

(In reply to comment #4)
> (From update of attachment 140505 [details])
> I think the general approach is good. I am sad to see EventContext grow, but it's not something we can avoid.
Comment 7 Dimitri Glazkov (Google) 2012-05-08 09:24:27 PDT
(In reply to comment #6)
> Hi Dimitri,
> 
> Okay. Let me proceed using the current approach. Before trying to improve the performance seriously, I'd like to have some benchmark to compare the performance.
> 
> AFAIR, you've told me that you used a micro benchmark when you tried to modify event dispatching before. Could you give me the used benchmark if you still had it? I think a micro benchmark is enough for now and it is not difficult to write such a benchmark. But I'd be happy if you gave me that :)
> 
> (In reply to comment #4)
> > (From update of attachment 140505 [details] [details])
> > I think the general approach is good. I am sad to see EventContext grow, but it's not something we can avoid.

I think it's somewhere in a bug under this tree: https://bugs.webkit.org/showdependencytree.cgi?id=48698, but I couldn't locate it easily.
Comment 8 Hayato Ito 2012-05-08 15:53:45 PDT
Thank you! Let me find it.

(In reply to comment #7)
> I think it's somewhere in a bug under this tree: https://bugs.webkit.org/showdependencytree.cgi?id=48698, but I couldn't locate it easily.
Comment 9 Hayato Ito 2012-05-09 19:25:16 PDT
Created attachment 141073 [details]
WIP. Fix an underlining bug. Every existing test should pass now.
Comment 10 Dimitri Glazkov (Google) 2012-05-09 20:16:09 PDT
Comment on attachment 141073 [details]
WIP. Fix an underlining bug. Every existing test should pass now.

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

> Source/WebCore/dom/EventContext.h:57
> +    bool m_shouldDispatch;

Isn't this overkill? Once we see an ancestor on which we shouldn't dispatch an event, we can just exit early. Do we really need to store an extra member on the context?
Comment 11 Hayato Ito 2012-05-09 20:40:15 PDT
Welcome to the complex world. I thought the same thing at first. But we can create an example where we should continue to climb up ancestors even if nodes in the middle should not be dispatched.

Thats' exactly the case2 in the https://docs.google.com/document/d/1PY7KkTRI-doU9Hew3Mud9JBgjj5xtVqH8sY0Ui305nw/edit.

Event should not be dispatched on G, but should be dispatched on A and B.


(In reply to comment #10)
> (From update of attachment 141073 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141073&action=review
> 
> > Source/WebCore/dom/EventContext.h:57
> > +    bool m_shouldDispatch;
> 
> Isn't this overkill? Once we see an ancestor on which we shouldn't dispatch an event, we can just exit early. Do we really need to store an extra member on the context?
Comment 12 Dimitri Glazkov (Google) 2012-05-09 20:56:49 PDT
(In reply to comment #11)
> Welcome to the complex world. I thought the same thing at first. But we can create an example where we should continue to climb up ancestors even if nodes in the middle should not be dispatched.
> 
> Thats' exactly the case2 in the https://docs.google.com/document/d/1PY7KkTRI-doU9Hew3Mud9JBgjj5xtVqH8sY0Ui305nw/edit.
> 
> Event should not be dispatched on G, but should be dispatched on A and B.
> 
> 
> (In reply to comment #10)
> > (From update of attachment 141073 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=141073&action=review
> > 
> > > Source/WebCore/dom/EventContext.h:57
> > > +    bool m_shouldDispatch;
> > 
> > Isn't this overkill? Once we see an ancestor on which we shouldn't dispatch an event, we can just exit early. Do we really need to store an extra member on the context?

But that diagram is incorrect. G is never an adjusted target in that subtree. H is.
Comment 13 Dimitri Glazkov (Google) 2012-05-09 20:57:28 PDT
(In reply to comment #12)
> But that diagram is incorrect. G is never an adjusted target in that subtree. H is.

Sorry, it's K.
Comment 14 Dimitri Glazkov (Google) 2012-05-09 20:58:20 PDT
Can you look over stuff I wrote here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16176#c10 and see if you can poke holes in that thinking?
Comment 15 Hayato Ito 2012-05-09 21:05:57 PDT
Yeah, I am now taking a look at that. Let me address your comment soon.

In current WIP patch, insertionPoint and ShadowRoot can receive events, but I've not written tests for that.
The doc might be out-of-date and doesn't reflect recent code. The doc should have insertionPoint (and ShadowRoot) in the picture and table. Let me update that too.

(In reply to comment #14)
> Can you look over stuff I wrote here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16176#c10 and see if you can poke holes in that thinking?
Comment 16 Hayato Ito 2012-05-09 23:04:51 PDT
I've added a comment to https://www.w3.org/Bugs/Public/show_bug.cgi?id=16176.
Now I am sure I understand your ideas. Let me reflect your ideas in a following patch.

(In reply to comment #15)
> Yeah, I am now taking a look at that. Let me address your comment soon.
> 
> In current WIP patch, insertionPoint and ShadowRoot can receive events, but I've not written tests for that.
> The doc might be out-of-date and doesn't reflect recent code. The doc should have insertionPoint (and ShadowRoot) in the picture and table. Let me update that too.
> 
> (In reply to comment #14)
> > Can you look over stuff I wrote here: https://www.w3.org/Bugs/Public/show_bug.cgi?id=16176#c10 and see if you can poke holes in that thinking?
Comment 17 Hayato Ito 2012-05-09 23:26:29 PDT
Let me note what should be modified:

 - Update adjusting target algorithm so that it can handle InsertionPoint correctly.
 - We don't need lowest common boundary any more.
 - For each scope of target ancestors, calculate a related target.
 - If target == relatedTarget, stop event dispatching.
 - Remove m_shouldBeDispatched from EventContext.
Comment 18 Hayato Ito 2012-05-10 01:35:28 PDT
Created attachment 141109 [details]
WIP. Fix the calculation of adjusted target. Now it considers InsertionPoints correctly.
Comment 19 Hayato Ito 2012-05-10 02:55:17 PDT
Created attachment 141123 [details]
WIP. New ajusting relatedTarget algorithm.
Comment 20 Hayato Ito 2012-05-10 04:06:38 PDT
Created attachment 141138 [details]
WIP. Now event dispatching is stopped where target == relatedTarget.
Comment 21 Hayato Ito 2012-05-10 04:08:57 PDT
Most of them are done.

We should define what should be set as relatedTarget if there is no ancestor of relatedTarget in the current scope.

(In reply to comment #17)
> Let me note what should be modified:
> 
>  - Update adjusting target algorithm so that it can handle InsertionPoint correctly.
>  - We don't need lowest common boundary any more.
>  - For each scope of target ancestors, calculate a related target.
>  - If target == relatedTarget, stop event dispatching.
>  - Remove m_shouldBeDispatched from EventContext.
Comment 22 Hayato Ito 2012-05-10 04:58:53 PDT
Created attachment 141146 [details]
WIP. Find an available relatedTarget from ancestor scope.
Comment 23 Hayato Ito 2012-05-10 05:00:31 PDT
Now event dispatcher can find a relatedTarget from parent tree scope, recursively.

(In reply to comment #21)
> Most of them are done.
> 
> We should define what should be set as relatedTarget if there is no ancestor of relatedTarget in the current scope.
> 
> (In reply to comment #17)
> > Let me note what should be modified:
> > 
> >  - Update adjusting target algorithm so that it can handle InsertionPoint correctly.
> >  - We don't need lowest common boundary any more.
> >  - For each scope of target ancestors, calculate a related target.
> >  - If target == relatedTarget, stop event dispatching.
> >  - Remove m_shouldBeDispatched from EventContext.
Comment 24 Dimitri Glazkov (Google) 2012-05-10 12:42:17 PDT
Comment on attachment 141146 [details]
WIP. Find an available relatedTarget from ancestor scope.

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

This looks very encouraging. Does it pass all tests? :)

We should probably approach this in pieces, to minimize regressions. Chunk ideas:
1) update ensureEventAncestors logic. I can easily see that the stack + logic structure could be a nice self-contained class.
2) land the logic for calculating relative relatedTarget -- maybe even not use it for the moment
3) switch implementation to use the new relatedTarget logic.

WDYT?

> Source/WebCore/dom/EventDispatcher.cpp:139
> +    EventTarget* relatedTarget = findRelatedTarget(scope->parentTreeScope(), relatedTargetMap);

This is neat, I haven't thought of this! Though I wold rather avoid recursion here. This logic + data + transient state seem like a good candidate for a helper class.
Comment 25 Hayato Ito 2012-05-10 19:20:19 PDT
(In reply to comment #24)
> (From update of attachment 141146 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141146&action=review
> 
> This looks very encouraging. Does it pass all tests? :)

Yes, it passed all existing tests.

> 
> We should probably approach this in pieces, to minimize regressions. Chunk ideas:
> 1) update ensureEventAncestors logic. I can easily see that the stack + logic structure could be a nice self-contained class.
> 2) land the logic for calculating relative relatedTarget -- maybe even not use it for the moment
> 3) switch implementation to use the new relatedTarget logic.
> 
> WDYT?

I agree. I'd like to proceed in incremental way. Let me think how we should divide this patch into smaller pieces further.

> > Source/WebCore/dom/EventDispatcher.cpp:139
> > +    EventTarget* relatedTarget = findRelatedTarget(scope->parentTreeScope(), relatedTargetMap);
> 
> This is neat, I haven't thought of this! Though I wold rather avoid recursion here. This logic + data + transient state seem like a good candidate for a helper class.

This, trying to find a related target in upper tree scope, is just one idea. I've not decided what should be done if there is no available related target ancestor in the current scope.
At first attempt, I just set relatedTarget to 'null' if there is no related target in the current scope. That is another idea.

I am not sure what is better or not.

If we try to find a relatedTarget from upper scopes,  I agree we should avoid recursion. Let me rethink implementation again.
I am also afraid that using HashMap might be bad here from the view of performance. Since the number of TreeScope is small in general, it might be faster that using some other linear search.
Comment 26 Dimitri Glazkov (Google) 2012-05-10 21:38:24 PDT
(In reply to comment #25)

> 
> I am not sure what is better or not.
> 
I will spec this for you tomorrow.
Comment 27 Hayato Ito 2012-05-10 22:48:24 PDT
I am afraid that if we land 1) without 2), we might leak nodes through a relatedTarget.
We should proceed carefully...

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 141146 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=141146&action=review
> > 
> > This looks very encouraging. Does it pass all tests? :)
> 
> Yes, it passed all existing tests.
> 
> > 
> > We should probably approach this in pieces, to minimize regressions. Chunk ideas:
> > 1) update ensureEventAncestors logic. I can easily see that the stack + logic structure could be a nice self-contained class.
> > 2) land the logic for calculating relative relatedTarget -- maybe even not use it for the moment
> > 3) switch implementation to use the new relatedTarget logic.
> > 
> > WDYT?
> 
> I agree. I'd like to proceed in incremental way. Let me think how we should divide this patch into smaller pieces further.
> 
> > > Source/WebCore/dom/EventDispatcher.cpp:139
> > > +    EventTarget* relatedTarget = findRelatedTarget(scope->parentTreeScope(), relatedTargetMap);
> > 
> > This is neat, I haven't thought of this! Though I wold rather avoid recursion here. This logic + data + transient state seem like a good candidate for a helper class.
> 
> This, trying to find a related target in upper tree scope, is just one idea. I've not decided what should be done if there is no available related target ancestor in the current scope.
> At first attempt, I just set relatedTarget to 'null' if there is no related target in the current scope. That is another idea.
> 
> I am not sure what is better or not.
> 
> If we try to find a relatedTarget from upper scopes,  I agree we should avoid recursion. Let me rethink implementation again.
> I am also afraid that using HashMap might be bad here from the view of performance. Since the number of TreeScope is small in general, it might be faster that using some other linear search.
Comment 28 Hayato Ito 2012-05-10 22:52:26 PDT
(In reply to comment #27)
> I am afraid that if we land 1) without 2), we might leak nodes through a relatedTarget.

The reason is that current algorithm of calculating relatedTarget does not consider InsertionPoint, by which we can cross lower boundaries.
I am afraid that I have to land that at once. Hmm...

> We should proceed carefully...
> 
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (From update of attachment 141146 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=141146&action=review
> > > 
> > > This looks very encouraging. Does it pass all tests? :)
> > 
> > Yes, it passed all existing tests.
> > 
> > > 
> > > We should probably approach this in pieces, to minimize regressions. Chunk ideas:
> > > 1) update ensureEventAncestors logic. I can easily see that the stack + logic structure could be a nice self-contained class.
> > > 2) land the logic for calculating relative relatedTarget -- maybe even not use it for the moment
> > > 3) switch implementation to use the new relatedTarget logic.
> > > 
> > > WDYT?
> > 
> > I agree. I'd like to proceed in incremental way. Let me think how we should divide this patch into smaller pieces further.
> > 
> > > > Source/WebCore/dom/EventDispatcher.cpp:139
> > > > +    EventTarget* relatedTarget = findRelatedTarget(scope->parentTreeScope(), relatedTargetMap);
> > > 
> > > This is neat, I haven't thought of this! Though I wold rather avoid recursion here. This logic + data + transient state seem like a good candidate for a helper class.
> > 
> > This, trying to find a related target in upper tree scope, is just one idea. I've not decided what should be done if there is no available related target ancestor in the current scope.
> > At first attempt, I just set relatedTarget to 'null' if there is no related target in the current scope. That is another idea.
> > 
> > I am not sure what is better or not.
> > 
> > If we try to find a relatedTarget from upper scopes,  I agree we should avoid recursion. Let me rethink implementation again.
> > I am also afraid that using HashMap might be bad here from the view of performance. Since the number of TreeScope is small in general, it might be faster that using some other linear search.
Comment 29 Hayato Ito 2012-05-11 01:44:41 PDT
Created attachment 141355 [details]
WIP. Introduce an EventAncestors class.
Comment 30 Hayato Ito 2012-05-11 01:51:25 PDT
I've started to introduce EventAncestors class, which encapsulates all operations and data which are related to ancestors of an event target.

But it turned out that it was not so worth as I expected. That causes redundant indirection at least.

WDYT? To go or not to go?

(In reply to comment #29)
> Created an attachment (id=141355) [details]
> WIP. Introduce an EventAncestors class.
Comment 31 Dimitri Glazkov (Google) 2012-05-11 10:29:11 PDT
Comment on attachment 141355 [details]
WIP. Introduce an EventAncestors class.

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

Yes, this is getting unwieldy. I think the problem here is that you treat EventAncestors as a data+function encapsulation primitive, but it really just acts as a function encapsulation primitive. What about something like EventAncestorsBuilder class, which only exists to create eventAncestors  list. Similarly, the RelatedTargetAdjuster class will only operate on the eventAncestors to adjust the list according to relatedTarget data?

> Source/WebCore/dom/EventDispatcher.cpp:142
> +    // Per XBL 2.0 spec, mutation events should never cross shadow DOM boundary:
> +    // http://dev.w3.org/2006/xbl2/#event-flow-and-targeting-across-shadow-s
> +    if (event->hasInterface(eventNames().interfaceForMutationEvent))
> +        return StayInsideShadowDOM;

We don't fire mutation events inside of the shadow DOM subtrees anymore, so this is not needed.

> Source/WebCore/dom/EventDispatcher.cpp:153
> +void EventAncestors::adjustByRelatedTarget(Event* event, PassRefPtr<EventTarget> prpRelatedTarget)

This looks outside of EventsAncestor helper.

> Source/WebCore/dom/EventDispatcher.h:57
> +    void adjustByRelatedTarget(Event*, PassRefPtr<EventTarget> prpRelatedTarget);

I don't like the addition of "By" here. It doesn't clarify the name.
Comment 32 Hayato Ito 2012-05-13 22:29:11 PDT
Thank you for the review.

(In reply to comment #31)
> (From update of attachment 141355 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141355&action=review
> 
> Yes, this is getting unwieldy. I think the problem here is that you treat EventAncestors as a data+function encapsulation primitive, but it really just acts as a function encapsulation primitive. What about something like EventAncestorsBuilder class, which only exists to create eventAncestors  list. Similarly, the RelatedTargetAdjuster class will only operate on the eventAncestors to adjust the list according to relatedTarget data?

That sounds nice, but I am not sure whether it is worth or not to separe these functions into separate classes since these functions are not so long now (about 20 lines). But if separating makes our intention clear, we should do it. Let me try it.

> 
> > Source/WebCore/dom/EventDispatcher.cpp:142
> > +    // Per XBL 2.0 spec, mutation events should never cross shadow DOM boundary:
> > +    // http://dev.w3.org/2006/xbl2/#event-flow-and-targeting-across-shadow-s
> > +    if (event->hasInterface(eventNames().interfaceForMutationEvent))
> > +        return StayInsideShadowDOM;
> 
> We don't fire mutation events inside of the shadow DOM subtrees anymore, so this is not needed.
> 

Thank you. Let me remove it from the next patch.

> > Source/WebCore/dom/EventDispatcher.cpp:153
> > +void EventAncestors::adjustByRelatedTarget(Event* event, PassRefPtr<EventTarget> prpRelatedTarget)
> 
> This looks outside of EventsAncestor helper.
> 
> > Source/WebCore/dom/EventDispatcher.h:57
> > +    void adjustByRelatedTarget(Event*, PassRefPtr<EventTarget> prpRelatedTarget);
> 
> I don't like the addition of "By" here. It doesn't clarify the name.

Let me remove 'By' and name it simply 'adjustRelatedTarget'.
Comment 33 Hayato Ito 2012-05-14 06:43:45 PDT
Created attachment 141719 [details]
WIP. Introduce an EventRelatedTargetAdjuster.
Comment 34 Hayato Ito 2012-05-14 06:46:40 PDT
Created attachment 141720 [details]
Benchmark for event-dispatching.
Comment 35 Hayato Ito 2012-05-14 07:04:19 PDT
I updated the patch. The differences are:

- Reverted the previous attempt which introduced EventAncestors class.
- Introduce an EventRelatedTargetAdjuster
- Optimize the performance of calculating adjusted related target. We now skip looking up relatedTarget if treeScope does not change.

And I've just done very rough benchmark. I've attached the benchmark here:
https://bugs.webkit.org/attachment.cgi?id=141720

The benchmark measures overall performance of event dispatching.
I've not isolated the part of calculating event ancestors and adjusting related-target yet. That requires C++ code change.

The results of benchmark looks same before/after this patch. Let me paste the results in the bottom.
I am afraid that I could not isolate the effect of this patch. Let me think more sophisticated benchmark later if required. 



--- Before this patch ----

Benchmark for event dispatching.

Shadow-Free Tree (height=10)
 Took 9.3 ms.  (93/10)
Teee with ShadowHost (height=10)
 Took 8.9 ms.  (89/10)
Shadow-Free Tree (height=20)
 Took 16.6 ms.  (166/10)
Teee with ShadowHost (height=20)
 Took 14.3 ms.  (143/10)
Shadow-Free Tree (height=30)
 Took 28.3 ms.  (283/10)
Teee with ShadowHost (height=30)
 Took 26.2 ms.  (262/10)
Shadow-Free Tree (height=50)
 Took 58.6 ms.  (586/10)
Teee with ShadowHost (height=50)
 Took 57.6 ms.  (576/10)
Shadow-Free Tree (height=100)
 Took 230.6 ms.  (2306/10)
Teee with ShadowHost (height=100)
 Took 212.3 ms.  (2123/10)
Shadow-Free Tree (height=200)
 Took 885.9 ms.  (8859/10)
Teee with ShadowHost (height=200)
 Took 806.8 ms.  (8068/10)
Shadow-Free Tree (height=300)
 Took 2028 ms.  (20280/10)
Teee with ShadowHost (height=300)
 Took 1721.7 ms.  (17217/10)

#EOF



--- After this patch ----

Benchmark for event dispatching.

Shadow-Free Tree (height=10)
 Took 9.9 ms.  (99/10)
Teee with ShadowHost (height=10)
 Took 9.7 ms.  (97/10)
Shadow-Free Tree (height=20)
 Took 17.7 ms.  (177/10)
Teee with ShadowHost (height=20)
 Took 15.6 ms.  (156/10)
Shadow-Free Tree (height=30)
 Took 29.5 ms.  (295/10)
Teee with ShadowHost (height=30)
 Took 28.1 ms.  (281/10)
Shadow-Free Tree (height=50)
 Took 61.7 ms.  (617/10)
Teee with ShadowHost (height=50)
 Took 61.9 ms.  (619/10)
Shadow-Free Tree (height=100)
 Took 248.9 ms.  (2489/10)
Teee with ShadowHost (height=100)
 Took 219.3 ms.  (2193/10)
Shadow-Free Tree (height=200)
 Took 925.6 ms.  (9256/10)
Teee with ShadowHost (height=200)
 Took 818.4 ms.  (8184/10)
Shadow-Free Tree (height=300)
 Took 2092 ms.  (20920/10)
Teee with ShadowHost (height=300)
 Took 1770.8 ms.  (17708/10)
Comment 36 Dimitri Glazkov (Google) 2012-05-14 09:27:02 PDT
Comment on attachment 141719 [details]
WIP. Introduce an EventRelatedTargetAdjuster.

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

I really like the way this patch is heading....

However, I apologize for trying to force my design ideas upon you. This is your work and I trust your design sense to come up with the optimal solution. Please feel free to discard the notion of helper classes. I will not be upset :)

> Source/WebCore/dom/EventDispatcher.h:53
> +    void build(Node*, Vector<EventContext>&);

Should probably contain the stack and the walker as members.

> Source/WebCore/dom/EventDispatcher.h:58
> +    void adjust(Vector<EventContext>&, Node* relatedTarget);

Looks like relatedTarget could be a constructor argument. This way, you could initialized the walker as part of the constructor.
Comment 37 Dimitri Glazkov (Google) 2012-05-14 09:52:00 PDT
(In reply to comment #35)
> Let me think more sophisticated benchmark later if required. 

Sure. FWIW, the benchmark code looks like it's doing what it's supposed to.
Comment 38 Hayato Ito 2012-05-15 05:43:15 PDT
Created attachment 141936 [details]
WIP.
Comment 39 Hayato Ito 2012-05-15 05:46:06 PDT
Created attachment 141937 [details]
modify EventDispatcher for benchmark
Comment 40 Hayato Ito 2012-05-15 05:57:22 PDT
Thank you for the review.

(In reply to comment #36)
> (From update of attachment 141719 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141719&action=review
> 
> I really like the way this patch is heading....
> 
> However, I apologize for trying to force my design ideas upon you. This is your work and I trust your design sense to come up with the optimal solution. Please feel free to discard the notion of helper classes. I will not be upset :)

Don't worry. I won't extract EventDispatcher::ensureEventAncestors into a separate class. It's gone.

> 
> > Source/WebCore/dom/EventDispatcher.h:53
> > +    void build(Node*, Vector<EventContext>&);
> 
> Should probably contain the stack and the walker as members.
> 
> > Source/WebCore/dom/EventDispatcher.h:58
> > +    void adjust(Vector<EventContext>&, Node* relatedTarget);
> 
> Looks like relatedTarget could be a constructor argument. This way, you could initialized the walker as part of the constructor.

Done.
As for the walker, I'd like to use the walker as a local variable in EventRelatedTargetAdjuster::adjust() since walker has internal state and I don't want to make walkers' lifecycle survive beyond the function.


I have one more thing. I've done another benchmark in C++. To isolate the effect of WIP patch, I've inserted code to EventDispatcher::adjustRelatedTarget(..) so that we can measure how much time this function takes. The diff is here: https://bugs.webkit.org/attachment.cgi?id=141937
I've also inserted the similar benchmark for ToT so that we compare the results.

As a result, it turned out that this function occupies most time of event dispatching in both cases.
Let me paste the results, including my comments to explain the results.

Good news:
The results looks almost same before/after applying WIP patch in every cases. I think the WIP patch has already enough competitive performance so that we can land this.

Bad news:
Nothing so far as long as I did a correct benchmark.

We are very close to goal finally. Let me clean up the patch for the review. I'll add a detailed ChangeLog entry in next patch, including the benchmark results.



--------------------------------
Before applying WIP patch.

# The following are how much time EventDispatcher::adjustRelatedTarget(..) takes in each case. (Measured in C++).

 Took 18.821045 ms 
 Took 17.198975 ms 
 Took 33.204102 ms 
 Took 28.000977 ms 
 Took 54.199951 ms 
 Took 52.866943 ms 
 Took 117.178955 ms 
 Took 114.033447 ms 
 Took 461.781982 ms 
 Took 420.091064 ms 
 Took 1761.498047 ms 
 Took 1577.511230 ms 
 Took 3835.390137 ms 
 Took 3359.852051 ms 


# The followings are how much time whole event dispatching takes in each case. (Measured from JavaScript using event-benchmark.html).

Shadow-Free Tree (height=10)
 Took 22 ms. 
Teee with ShadowHost (height=10)
 Took 18 ms. 
Shadow-Free Tree (height=20)
 Took 34 ms. 
Teee with ShadowHost (height=20)
 Took 28 ms. 
Shadow-Free Tree (height=30)
 Took 55 ms. 
Teee with ShadowHost (height=30)
 Took 54 ms. 
Shadow-Free Tree (height=50)
 Took 119 ms. 
Teee with ShadowHost (height=50)
 Took 115 ms. 
Shadow-Free Tree (height=100)
 Took 467 ms. 
Teee with ShadowHost (height=100)
 Took 425 ms. 
Shadow-Free Tree (height=200)
 Took 1780 ms. 
Teee with ShadowHost (height=200)
 Took 1596 ms. 
Shadow-Free Tree (height=300)
 Took 3875 ms. 
Teee with ShadowHost (height=300)
 Took 3401 ms. 

#EOF


--------------------------------
After applying WIP patch.


 Took 20.304199 ms 
 Took 19.757080 ms 
 Took 33.982910 ms 
 Took 30.224854 ms 
 Took 55.161865 ms 
 Took 55.628906 ms 
 Took 117.801758 ms 
 Took 119.816895 ms 
 Took 462.904053 ms 
 Took 423.419189 ms 
 Took 1721.790039 ms 
 Took 1581.446777 ms 
 Took 3957.707275 ms 
 Took 3439.345947 ms 


Shadow-Free Tree (height=10)
 Took 23 ms. 
Teee with ShadowHost (height=10)
 Took 20 ms. 
Shadow-Free Tree (height=20)
 Took 35 ms. 
Teee with ShadowHost (height=20)
 Took 31 ms. 
Shadow-Free Tree (height=30)
 Took 56 ms. 
Teee with ShadowHost (height=30)
 Took 57 ms. 
Shadow-Free Tree (height=50)
 Took 120 ms. 
Teee with ShadowHost (height=50)
 Took 122 ms. 
Shadow-Free Tree (height=100)
 Took 468 ms. 
Teee with ShadowHost (height=100)
 Took 429 ms. 
Shadow-Free Tree (height=200)
 Took 1741 ms. 
Teee with ShadowHost (height=200)
 Took 1599 ms. 
Shadow-Free Tree (height=300)
 Took 3999 ms. 
Teee with ShadowHost (height=300)
 Took 3483 ms. 

#EOF
Comment 41 Hayato Ito 2012-05-16 02:25:37 PDT
Created attachment 142203 [details]
ready for reviews
Comment 42 Hayato Ito 2012-05-16 04:23:43 PDT
Created attachment 142219 [details]
ready for reviews. Refine a changelog.
Comment 43 Dimitri Glazkov (Google) 2012-05-16 09:14:01 PDT
Comment on attachment 142219 [details]
ready for reviews. Refine a changelog.

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

Maybe it's because I spent a lot of time on this part of the spec, but the new code reads _a lot_ better than the old one to me. Good work! A few nits -- up to you to fix before or after landing.

> Source/WebCore/ChangeLog:90
> +        I've also conducted a micro benchmark using both
> +        Shadow-Free-DOM-Tree and DOM-Tree-With-Shadow-Host.
> +        See https://bugs.webkit.org/show_bug.cgi?id=78586#c40 for the results.
> +        It seems that the new implementation has more capabilities, but
> +        doesn't sacrifice a performance of event dispatching in either cases.

You really didn't pull any punches on this ChangeLog, did you :)

> Source/WebCore/dom/ComposedShadowTreeWalker.cpp:212
> +    if (node->isShadowRoot()) {
> +        const ShadowRoot* shadowRoot = toShadowRoot(node);

This would probably look a bit better as early return.

> Source/WebCore/dom/EventContext.cpp:50
> +static MouseEvent* toMouseEvent(Event* event)
> +{
> +    ASSERT(event && event->isMouseEvent());
> +    return static_cast<MouseEvent*>(event);
> +}

This seems like it belongs in MouseEvent.h?
Comment 44 Hayato Ito 2012-05-16 20:25:04 PDT
Thank you for the review. Let me land this patch after fixing what you commented and updating Skipped files on some platforms.

(In reply to comment #43)
> (From update of attachment 142219 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142219&action=review
> 
> Maybe it's because I spent a lot of time on this part of the spec, but the new code reads _a lot_ better than the old one to me. Good work! A few nits -- up to you to fix before or after landing.
> 
> > Source/WebCore/ChangeLog:90
> > +        I've also conducted a micro benchmark using both
> > +        Shadow-Free-DOM-Tree and DOM-Tree-With-Shadow-Host.
> > +        See https://bugs.webkit.org/show_bug.cgi?id=78586#c40 for the results.
> > +        It seems that the new implementation has more capabilities, but
> > +        doesn't sacrifice a performance of event dispatching in either cases.
> 
> You really didn't pull any punches on this ChangeLog, did you :)

I've learned the expression of 'pull any punches'. :)

> 
> > Source/WebCore/dom/ComposedShadowTreeWalker.cpp:212
> > +    if (node->isShadowRoot()) {
> > +        const ShadowRoot* shadowRoot = toShadowRoot(node);
> 
> This would probably look a bit better as early return.

Thank you. I'll fix it.

> 
> > Source/WebCore/dom/EventContext.cpp:50
> > +static MouseEvent* toMouseEvent(Event* event)
> > +{
> > +    ASSERT(event && event->isMouseEvent());
> > +    return static_cast<MouseEvent*>(event);
> > +}
> 
> This seems like it belongs in MouseEvent.h?

Yeah, that should be. I'll move it.
Comment 45 Hayato Ito 2012-05-16 20:38:00 PDT
Created attachment 142403 [details]
Patch for landing
Comment 46 Hayato Ito 2012-05-16 20:40:51 PDT
Created attachment 142404 [details]
Patch for landing
Comment 47 WebKit Review Bot 2012-05-16 21:37:03 PDT
Comment on attachment 142404 [details]
Patch for landing

Clearing flags on attachment: 142404

Committed r117394: <http://trac.webkit.org/changeset/117394>
Comment 48 WebKit Review Bot 2012-05-16 21:37:10 PDT
All reviewed patches have been landed.  Closing bug.