Optimize nsDOMEvent::SetEventType

RESOLVED FIXED

Status

()

Core
DOM: Events
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug)

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Patch coming once I've sorted out a bug it causes.
Created attachment 448124 [details] [diff] [review]
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.
I'll probably change nsRemoveTemporaryEventMapping to be triggered when CC runs
or something.
Created attachment 448409 [details] [diff] [review]
patch

Simpler.
I'll upload this to tryserver.
Attachment #448124 - Attachment is obsolete: true
ooops, the patch contains some garbage.
Created attachment 448415 [details] [diff] [review]
patch
Attachment #448409 - Attachment is obsolete: true
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.
Attachment #448415 - Flags: review?(jonas)
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?
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))
(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 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)
Attachment #448415 - Flags: review?(jonas) → review+
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.
Created attachment 449950 [details] [diff] [review]
this

Thanks for the review!
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.
The array is an easy way to keep the atoms alive.
And fifo should work reasonable well in common cases.
LRU would be better, but would need more bookkeeping.
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.
(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.
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.
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.
http://hg.mozilla.org/mozilla-central/rev/6dd26520c9a6
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.