Closed Bug 1373857 Opened 7 years ago Closed 7 years ago

The hashtable lookup in nsContentUtils::GetEventMessageAndAtom() can show up in profiles

Categories

(Core :: DOM: Events, enhancement)

Product:
Core
Component:
DOM: Events
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(4 files, 5 obsolete files)

Part 4: Optimize the selectionchange async event dispatcher to use direct WidgetEvent dispatch
1.51 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 2: Add some AsyncEventDispatcher helpers for dispatching WidgetEvents directly
5.21 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 1: Add some nsContentUtils helpers for dispatching WidgetEvents directly
13.19 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Part 3: Optimize the PostHandleEvent() events that we dispatch for checkboxes and radiocontrols when they get toggled in order to make it use direct WidgetEvent dispatch
9.25 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
See this profile for example: http://bit.ly/2sIc3bw

This can specifically queue up in the form of a bunch of AsyncEventDispatcher runnables in front of the setTImeout(0) runnable of the async test callback in bug making those async tests sometimes take a few ms longer than normal.

It is pretty easy to avoid this overhead by using a static atom instead of a string from the get go, I think.
Assignee: nobody → ehsan
I assume the relevant events are
http://searchfox.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#4332-4342
It would be way better to just dispatch those events using WidgetEvents.
These ones too for similar reasons: http://searchfox.org/mozilla-central/source/dom/html/HTMLInputElement.cpp#4346-4355

And also the selectionchange events which get dispatched to the document the script changes the selection for example by focusing things.  That also shows up in the profiles.  My patches optimize those events and make them go away from the profiles.
Comment on attachment 8878825 [details] [diff] [review]
Part 1: Add some nsContentUtils helpers for dispatching WidgetEvents directly


>+// Because of SVG/SMIL we have several atoms mapped to the same
>+// id, but we can rely on MESSAGE_TO_EVENT to map id to only one atom.
>+static bool
>+ShouldAddEventToStringEventTable(const EventNameMapping& aMapping)
>+{
>+  return GetEventTypeFromMessage(aMapping.mMessage) == aMapping.mAtom;
Could you assert in this method that aMapping.mAtom isn't null
> }


> nsresult
>+nsContentUtils::DispatchTrustedEvent(nsIDocument* aDoc, nsISupports* aTarget,
>+                                     EventMessage aEventMessage,
>+                                     bool aCanBubble, bool aCancelable,
>+                                     bool *aDefaultAction,
* goes with the type.

>+                                     bool aOnlyChromeDispatch)
>+{
>+  return DispatchEvent(aDoc, aTarget, aEventMessage, aCanBubble, aCancelable,
>+                       true, aDefaultAction, aOnlyChromeDispatch);
>+}
>+
>+// static
>+nsresult
> nsContentUtils::DispatchUntrustedEvent(nsIDocument* aDoc, nsISupports* aTarget,
>                                        const nsAString& aEventName,
>                                        bool aCanBubble, bool aCancelable,
>                                        bool *aDefaultAction)
ditto


> nsresult
>+nsContentUtils::DispatchUntrustedEvent(nsIDocument* aDoc, nsISupports* aTarget,
>+                                       EventMessage aEventMessage,
>+                                       bool aCanBubble, bool aCancelable,
>+                                       bool *aDefaultAction,
ditto
>+nsresult
>+nsContentUtils::DispatchEvent(nsIDocument* aDoc, nsISupports* aTarget,
>+                              EventMessage aEventMessage,
>+                              bool aCanBubble, bool aCancelable,
>+                              bool aTrusted, bool *aDefaultAction,
>+                              bool aOnlyChromeDispatch)
>+{
>+  MOZ_ASSERT_IF(aOnlyChromeDispatch, aTrusted);
>+
>+  WidgetEvent event(aTrusted, aEventMessage);
Need to assert here that the particular aEventMessage uses WidgetEvent and not some of its subclasses.
You can for example go through EventNameList.h and check that the type maps to eBasicEventClass

>+nsresult
>+nsContentUtils::DispatchChromeEvent(nsIDocument *aDoc,
>+                                    nsISupports *aTarget,
>+                                    EventMessage aEventMessage,
>+                                    bool aCanBubble, bool aCancelable,
>+                                    bool *aDefaultAction)
>+{
>+  WidgetEvent event(true, aEventMessage);
Same here

>   /**
>+   * This method creates and dispatches a trusted event using an event message.
>+   * @param aDoc           The document which will be used to create the event.
>+   * @param aTarget        The target of the event, should be QIable to
>+   *                       nsIDOMEventTarget.
EventTarget


>-  if (aEvent->mMessage == eUnidentifiedEvent) {
>+  // In case aEvent->mMessage or aListener->mEventMessage is
>+  // eUnidentifiedEvent, we should use the atoms for comparison.
>+  if (aEvent->mMessage == eUnidentifiedEvent ||
>+      aListener->mEventMessage == eUnidentifiedEvent) {
Why this change?
If aEvent-mMessage != eUnidentifiedEvent, then AddEventListenerInternal should have found the right message for the relevant listener.
Attachment #8878825 - Flags: review?(bugs) → review-
Comment on attachment 8878825 [details] [diff] [review]
Part 1: Add some nsContentUtils helpers for dispatching WidgetEvents directly

So this supports only WidgetEvents, not any subclasses of it, and events like
input event needs InternalEditorInputEvent
Comment on attachment 8878826 [details] [diff] [review]
Part 2: Add some AsyncEventDispatcher helpers for dispatching WidgetEvents directly


>   mTarget->AsyncEventRunning(this);
>+  if (mEventMessage != eUnidentifiedEvent) {
>+    return nsContentUtils::DispatchUntrustedEvent(node->OwnerDoc(), mTarget,
>+                                                  mEventMessage, mBubbles,
>+                                                  false /* aCancelable */,
>+                                                  nullptr /* aDefaultAction */,
>+                                                  mOnlyChromeDispatch);
>+  }
This dispatches untrusted

>   RefPtr<Event> event = mEvent ? mEvent->InternalDOMEvent() : nullptr;
>   if (!event) {
>     event = NS_NewDOMEvent(mTarget, nullptr, nullptr);
>     event->InitEvent(mEventType, mBubbles, false);
>     event->SetTrusted(true);
but this trusted.
Attachment #8878826 - Flags: review?(bugs) → review-
Comment on attachment 8878827 [details] [diff] [review]
Part 3: Optimizie the PostHandleEvent() events that we dispatch for checkboxes and radiocontrols when they get toggled in order to make it use direct WidgetEvent dispatch

So because of part 1, this dispatches wrong kinds of events when eEditorInput is passed.
Attachment #8878827 - Flags: review?(bugs) → review-
Comment on attachment 8878828 [details] [diff] [review]
Part 4: Optimize the selectionchange async event dispatcher to use direct WidgetEvent dispatch

This is fine, but requires part 2 to be fixed.
Attachment #8878828 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #8)
> >+  // In case aEvent->mMessage or aListener->mEventMessage is
> >+  // eUnidentifiedEvent, we should use the atoms for comparison.
> >+  if (aEvent->mMessage == eUnidentifiedEvent ||
> >+      aListener->mEventMessage == eUnidentifiedEvent) {
> Why this change?
> If aEvent-mMessage != eUnidentifiedEvent, then AddEventListenerInternal
> should have found the right message for the relevant listener.

I had to add this because this test was failing: https://searchfox.org/mozilla-central/source/dom/tests/mochitest/general/test_selectevents.html

When this event listener gets registered <https://searchfox.org/mozilla-central/rev/b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/tests/mochitest/general/frameSelectEvents.html#67> we get to this code <https://searchfox.org/mozilla-central/rev/b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/events/EventListenerManager.cpp#722>.  There, nsContentUtils::GetEventMessageAndAtomForListener() tries to use sStringEventTable to find "selectionchange" but it fails.  So it hands off its work to nsContentUtils::GetEventMessageAndAtom().  That function does a second hashtable lookup on sStringEventTable (as far as I can tell this is *exactly* the same hashtable lookup as its caller performed...) which of course fails in the same way.  Then <https://searchfox.org/mozilla-central/rev/b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/base/nsContentUtils.cpp#4257> when running the test standalone at least returns 111, so we choose eUnidentifiedEvent for our listener here: <https://searchfox.org/mozilla-central/rev/b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/base/nsContentUtils.cpp#4265> and add a hashtable entry to sStringEventTable for it.  But note that the hashtable entry will have eUnidentifiedEvent as the message due to <https://searchfox.org/mozilla-central/rev/b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/base/nsContentUtils.cpp#4270>, so the second recursive call to find the newly inserted hashtable entry again (from <https://searchfox.org/mozilla-central/rev/b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/base/nsContentUtils.cpp#4315>) just finds eUnidentifiedEvent for our listener message type again.

Therefore, without the change above, for events that are dispatched using WidgetEvents directly but have handlers registered using a string event type we'd get listeners with message type set to eUnidentifiedEvent but with the atom set, unless if their hashtable entry was inserted from nsContentUtils::InitializeEventTable() for window-only and non-IDL events (which do get the correct event class ID as far as I can tell.)
See Also: → 1374072
(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 8878827 [details] [diff] [review]
> Part 3: Optimizie the PostHandleEvent() events that we dispatch for
> checkboxes and radiocontrols when they get toggled in order to make it use
> direct WidgetEvent dispatch
> 
> So because of part 1, this dispatches wrong kinds of events when
> eEditorInput is passed.

What do you mean?
Attachment #8878825 - Attachment is obsolete: true
Attachment #8878826 - Attachment is obsolete: true
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #14)
> (In reply to Olli Pettay [:smaug] from comment #11)
> > Comment on attachment 8878827 [details] [diff] [review]
> > Part 3: Optimizie the PostHandleEvent() events that we dispatch for
> > checkboxes and radiocontrols when they get toggled in order to make it use
> > direct WidgetEvent dispatch
> > 
> > So because of part 1, this dispatches wrong kinds of events when
> > eEditorInput is passed.
> 
> What do you mean?

You aren't supposed to dispatch eEditorInput using WidgetEvent, but InternalEditorInputEvent
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #13)
> (In reply to Olli Pettay [:smaug] from comment #8)
> > >+  // In case aEvent->mMessage or aListener->mEventMessage is
> > >+  // eUnidentifiedEvent, we should use the atoms for comparison.
> > >+  if (aEvent->mMessage == eUnidentifiedEvent ||
> > >+      aListener->mEventMessage == eUnidentifiedEvent) {
> > Why this change?
> > If aEvent-mMessage != eUnidentifiedEvent, then AddEventListenerInternal
> > should have found the right message for the relevant listener.
> 
> I had to add this because this test was failing:
> https://searchfox.org/mozilla-central/source/dom/tests/mochitest/general/
> test_selectevents.html
> 
> When this event listener gets registered
> <https://searchfox.org/mozilla-central/rev/
> b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/tests/mochitest/general/
> frameSelectEvents.html#67> we get to this code
> <https://searchfox.org/mozilla-central/rev/
> b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/events/EventListenerManager.
> cpp#722>.  There, nsContentUtils::GetEventMessageAndAtomForListener() tries
> to use sStringEventTable to find "selectionchange" but it fails.  So it
> hands off its work to nsContentUtils::GetEventMessageAndAtom().  That
> function does a second hashtable lookup on sStringEventTable (as far as I
> can tell this is *exactly* the same hashtable lookup as its caller
> performed...) which of course fails in the same way.  Then
> <https://searchfox.org/mozilla-central/rev/
> b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/base/nsContentUtils.cpp#4257>
> when running the test standalone at least returns 111, so we choose
> eUnidentifiedEvent for our listener here:
> <https://searchfox.org/mozilla-central/rev/
> b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/base/nsContentUtils.cpp#4265>
> and add a hashtable entry to sStringEventTable for it.  But note that the
> hashtable entry will have eUnidentifiedEvent as the message due to
> <https://searchfox.org/mozilla-central/rev/
> b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/base/nsContentUtils.cpp#4270>,
> so the second recursive call to find the newly inserted hashtable entry
> again (from
> <https://searchfox.org/mozilla-central/rev/
> b95a1a95838cc6e85ea5d9b9e3ae39b4cd69d8ef/dom/base/nsContentUtils.cpp#4315>)
> just finds eUnidentifiedEvent for our listener message type again.
> 
> Therefore, without the change above, for events that are dispatched using
> WidgetEvents directly but have handlers registered using a string event type
> we'd get listeners with message type set to eUnidentifiedEvent but with the
> atom set, unless if their hashtable entry was inserted from
> nsContentUtils::InitializeEventTable() for window-only and non-IDL events
> (which do get the correct event class ID as far as I can tell.)

But the event should have then eUnidentifiedEvent too.
If that is not the case, the bug is elsewhere than in EventListenerManager.
Looks like nsContentUtils doesn't use DOCUMENT_ONLY_EVENTs when creating the hashtables.
Comment on attachment 8878906 [details] [diff] [review]
Part 1: Add some nsContentUtils helpers for dispatching WidgetEvents directly

So the ELM part shows issue elsewhere. ELM shouldn't need to be changed.
Attachment #8878906 - Flags: review?(bugs) → review-
Comment on attachment 8878907 [details] [diff] [review]
Part 2: Add some AsyncEventDispatcher helpers for dispatching WidgetEvents directly

>+  /**
>+   * If aOnlyChromeDispatch is true, the event is dispatched to only
>+   * chrome node. In that case, if aTarget is already a chrome node,
>+   * the event is dispatched to it, otherwise the dispatch path starts
>+   * at the first chrome ancestor of that target.
>+   */
>+  AsyncEventDispatcher(nsINode* aTarget,
>+                       mozilla::EventMessage aEventMessage,
>+                       bool aBubbles, bool aOnlyChromeDispatch)
>+    : mTarget(aTarget)
>+    , mEventMessage(aEventMessage)
>+    , mBubbles(aBubbles)
>+    , mOnlyChromeDispatch(aOnlyChromeDispatch)
>+  {
Assert that mEventMessage != eUnidentifiedEvent


>+  AsyncEventDispatcher(dom::EventTarget* aTarget,
>+                       mozilla::EventMessage aEventMessage,
>+                       bool aBubbles)
>+    : mTarget(aTarget)
>+    , mEventMessage(aEventMessage)
>+    , mBubbles(aBubbles)
>+  {
ditto
Attachment #8878907 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #18)
> But the event should have then eUnidentifiedEvent too.
> If that is not the case, the bug is elsewhere than in EventListenerManager.

The event has eSelectionChange.  After part 4, eSelectionChange is passed to the widget event which is set on mMessage on the event object, which is then dispatched.  Is that not the right thing to do?

> Looks like nsContentUtils doesn't use DOCUMENT_ONLY_EVENTs when creating the
> hashtables.

No.  Should it?
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #17)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #14)
> > (In reply to Olli Pettay [:smaug] from comment #11)
> > > Comment on attachment 8878827 [details] [diff] [review]
> > > Part 3: Optimizie the PostHandleEvent() events that we dispatch for
> > > checkboxes and radiocontrols when they get toggled in order to make it use
> > > direct WidgetEvent dispatch
> > > 
> > > So because of part 1, this dispatches wrong kinds of events when
> > > eEditorInput is passed.
> > 
> > What do you mean?
> 
> You aren't supposed to dispatch eEditorInput using WidgetEvent, but
> InternalEditorInputEvent

Oh I didn't realize that.  That would make this much less useful for Speedometer.  :-(

Should I make these helpers a template that takes the type of widget event class as a template argument?  If we give InternalEditorInputEvent a two-argument constructor similar to WidgetEvent, then we can use the dispatch both types of widget events quite easily, I think.
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #21)
> (In reply to Olli Pettay [:smaug] from comment #18)
> > But the event should have then eUnidentifiedEvent too.
> > If that is not the case, the bug is elsewhere than in EventListenerManager.
> 
> The event has eSelectionChange.  After part 4, eSelectionChange is passed to
> the widget event which is set on mMessage on the event object, which is then
> dispatched.  Is that not the right thing to do?
> 
> > Looks like nsContentUtils doesn't use DOCUMENT_ONLY_EVENTs when creating the
> > hashtables.
> 
> No.  Should it?
Yes, so that the hashtables know about the eSelectionChange <-> nsGkAtoms::onselectionchange <-> "selectionchange" mapping
Flags: needinfo?(bugs)
(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #22)
> Should I make these helpers a template that takes the type of widget event
> class as a template argument?
Yeah, I think that is the easiest approach.
Also assert in method implementation that right class is passed for the particular message.
Attachment #8878906 - Attachment is obsolete: true
Attachment #8878827 - Attachment is obsolete: true
Comment on attachment 8879308 [details] [diff] [review]
Part 3: Optimizie the PostHandleEvent() events that we dispatch for checkboxes and radiocontrols when they get toggled in order to make it use direct WidgetEvent dispatch


> #ifdef ACCESSIBILITY
>       // Fire an event to notify accessibility
>       if (mType == NS_FORM_INPUT_CHECKBOX) {
>         FireEventForAccessibility(this, aVisitor.mPresContext,
>-                                  NS_LITERAL_STRING("CheckboxStateChange"));
>+                                  eFormCheckboxStateChange);
>       } else {
>         FireEventForAccessibility(this, aVisitor.mPresContext,
>-                                  NS_LITERAL_STRING("RadioStateChange"));
>+                                  eFormRadioStateChange);
>         // Fire event for the previous selected radio.
>         nsCOMPtr<nsIDOMHTMLInputElement> previous =
>           do_QueryInterface(aVisitor.mItemData);
>         if (previous) {
>           FireEventForAccessibility(previous, aVisitor.mPresContext,
>-                                    NS_LITERAL_STRING("RadioStateChange"));
>+                                    eFormRadioStateChange);
So these events are RadioStateChange and RadioStateChange, but you change them to be checkboxstatechange and radiostatechange
Attachment #8879308 - Flags: review?(bugs) → review-
Attachment #8879307 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #27)
> > #ifdef ACCESSIBILITY
> >       // Fire an event to notify accessibility
> >       if (mType == NS_FORM_INPUT_CHECKBOX) {
> >         FireEventForAccessibility(this, aVisitor.mPresContext,
> >-                                  NS_LITERAL_STRING("CheckboxStateChange"));
> >+                                  eFormCheckboxStateChange);
> >       } else {
> >         FireEventForAccessibility(this, aVisitor.mPresContext,
> >-                                  NS_LITERAL_STRING("RadioStateChange"));
> >+                                  eFormRadioStateChange);
> >         // Fire event for the previous selected radio.
> >         nsCOMPtr<nsIDOMHTMLInputElement> previous =
> >           do_QueryInterface(aVisitor.mItemData);
> >         if (previous) {
> >           FireEventForAccessibility(previous, aVisitor.mPresContext,
> >-                                    NS_LITERAL_STRING("RadioStateChange"));
> >+                                    eFormRadioStateChange);
> So these events are RadioStateChange and RadioStateChange, but you change
> them to be checkboxstatechange and radiostatechange

Huh...  For some reason I always assumed these are case insensitive, and try also passed, sorry about that.  I filed bug 1374502 to get some tests added for these events that at least if someone totally broke them they would realize that!
Attachment #8879308 - Attachment is obsolete: true
Comment on attachment 8879369 [details] [diff] [review]
Part 3: Optimize the PostHandleEvent() events that we dispatch for checkboxes and radiocontrols when they get toggled in order to make it use direct WidgetEvent dispatch

>+EVENT(CheckboxStateChange,
>+      eFormCheckboxStateChange,
>+      EventNameType_HTMLXUL,
>+      eBasicEventClass)
>+EVENT(RadioStateChange,
>+      eFormRadioStateChange,
>+      EventNameType_HTMLXUL,
>+      eBasicEventClass)
Not EventNameType_HTMLXUL but EventNameType_None
DOM attributes shouldn't support there as handlers

Is this patch on top of some other patch which templetizes dispatch methods?
Attachment #8879369 - Flags: review?(bugs) → review+
Yes, it is on top of attachment 8879307 [details] [diff] [review].
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ddbba90e78b
Part 1: Add some nsContentUtils helpers for dispatching WidgetEvents directly; r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c74c5b585e6
Part 2: Add some AsyncEventDispatcher helpers for dispatching WidgetEvents directly; r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6210679c913
Part 3: Optimize the PostHandleEvent() events that we dispatch for checkboxes and radiocontrols when they get toggled in order to make it use direct WidgetEvent dispatch; r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1dee370e1a
Part 4: Optimize the selectionchange async event dispatcher to use direct WidgetEvent dispatch; r=smaug
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: