568955 - Optimize nsDOMEvent::SetEventType

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Patch coming once I've sorted out a bug it causes.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

This has still a bit ugly nsRemoveTemporaryEventMapping.
I may just remove that and let uncommon event type init be slower in
tight loops. may.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

I'll probably change nsRemoveTemporaryEventMapping to be triggered when CC runs
or something.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Simpler.
I'll upload this to tryserver.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

ooops, the patch contains some garbage.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 448415 [details] [diff] [review]
patch 

Tryserver seems to like this.

This removes MOZ_MEDIA since that is actually unused.
Otherwise this makes nsDOMEvent::SetEventType to use the
information nsContentUtils has about events.
A new hashtable which has string as key is added.

sUserDefinedEvents array is a bit ugly, but should work reasonable well, unless someone dispatches lots of differently named user defined events.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What is the purpose of UserDefinedEvent? It seems like it supplies the same functionality as an nsRefPtr. I.e. you could make sUserDefinedEvents be an nsCOMArray<nsIAtom>. 

Though the purpose of sUserDefinedEvents is a bit unclear to me too. Mind giving a short explanation of what its goal is?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Just putting it in here so I don't forget. You should use nsDependentAtomString if all you're doing is grabbing a substring. I.e. Substring(nsDependentAtomString(someAtom))

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #7)
> What is the purpose of UserDefinedEvent? It seems like it supplies the same
> functionality as an nsRefPtr. I.e. you could make sUserDefinedEvents be an
> nsCOMArray<nsIAtom>. 
I need to get the address where the pointer to the atom is stored.


> Though the purpose of sUserDefinedEvents is a bit unclear to me too. Mind
> giving a short explanation of what its goal is?
There are cases when some random event name is used in a row and initEvent
is called a lot. So trying to be fast in those cases.


(In reply to comment #8)
> Just putting it in here so I don't forget. You should use nsDependentAtomString
> if all you're doing is grabbing a substring. I.e.
> Substring(nsDependentAtomString(someAtom))
Uh, right. I thought the string data would be shared, but seems like not.
Will fix.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 448415 [details] [diff] [review]
patch 

This looks good, but I'd love it if we could store nsIAtoms rather than nsIAtom** and get rid of sUserDefinedEvents.

If you do that, feel free to ask for a re-review (no need for interdiff)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I'll change nsGkAtoms initialization so that I can use nsIAtom* instead of nsIAtom**.
I'll keep using sUserDefinedEvents so that I can keep fifo behavior.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: this

Thanks for the review!

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why is fifo desirable? If for example we fire a event in chrome every so often then we don't want to purge that just because it was the first thing that got fired.

Fifo seems as good as just nuking everything when the cache fills up. Which I think would be an ok thing to do.

To get anything better than that I think LRU is needed.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

The array is an easy way to keep the atoms alive.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

And fifo should work reasonable well in common cases.
LRU would be better, but would need more bookkeeping.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

How is Fifo better than clear-all-on-full?

Another way to keep atoms alive is to make all user-defines mappings own their atoms.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #16)
> How is Fifo better than clear-all-on-full?
Might not be better at all. Not a big thing anyway.

> Another way to keep atoms alive is to make all user-defines mappings own their
> atoms.
Which would just force one to use manual AddRef/Release, which is error prone.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I actually want to allow NS_USER_DEFINED_EVENTs in the static array.

So I'm going to push this, with the fifo handling. I could change
that to clear-all if really needed.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think clear-all is needed, but I also don't think that fifo is needed. The only reason i'm arguing for clear-all is that it's simpler to no (to me) perceivable downside.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/6dd26520c9a6