407442 - nsEventListenerManager is allocation happy

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P1)

Description

[Stuart Parmenter]

Two parts to this, but they both revolve around mListeners.

mListeners holds nsListenerStruct*'s -- this seems pretty dumb, it should hold nsListenerStruct's instead -- we're resizing the array, might as well resize it to hold the elements as well.

the second part, which I haven't got any real numbers on aside to say that I don't see any VoidArray allocations bigger than 64 bytes happening on mListeners, is that mListeners should probably be an nsAutoTArray<nsListenerStruct, 16> (maybe smaller, how many nsEventListenerManagers do we have -- if just one, 16 is probably fine -- we can always adjust it later)

Comment 1

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

Every DOMNode (+ window) may have an ELM. I *guess* the average listener count is
closer to 1 (maybe 1-3), or in case of |window| object perhaps something between
5 and 10.

Comment 2

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

Yup, i don't think we want to use an nsAutoTArray here unfortunately. Or possibly use one with only 1 slot in it. The downside with having 1 slot would of course be that it'd be wasted every time there is more than one listener.

We definitely should store nsListenerStructs though. And eventually stick all this stuff into the same arena as the nodes, once we have that up and running.

Comment 3

[[e-mail]]

i started the browser, visited about 8 sites on trunk, and closed down. in
total, there were 1898 ELM's constructed. i looked at the maximum size of
mListeners; if we use an nsAutoTArray<, 4> we'll avoid alloc in 91% of cases;
<, 8> would get us to 97%. nsListenerStructs aren't small though (24 bytes on
32-bit), so assuming all 1898 are live at a given time (e.g. if all 8 sites
were open in tabs), which i didn't confirm, then we'll be wasting at
most ~80kb with a <, 4>. too much?

sicking, what kind of arena are you thinking of? i'm curious if/how you'll be handling deallocations.

Comment 4

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

Did you by any chance get a full histogram of the distribution? Would be great if you could post or attach it here if so. Also, were you checking the highest number of listeners an ELM had, or the number of listeners when it was killed? It probably won't differ much in the common case, but it's the former that is most interesting.

My guess is that using <, 1> is a good number. Did you see how many ELMs had exactly 1 listener?

Comment 5

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

Oh, and the arena work is going on in bug 403830. It'll use the existing PLArena for now, but we'll probably write a new one once bug 403830 is landed.

Comment 6

[[e-mail]]

i measured the maximum size of mListeners; many listeners did indeed have elements removed at some point (or the list cleared; in fact there were 5181 calls to mListeners.Clear() - a pretty well-hit path!). the histogram of maximum sizes is:

1: 958
2: 595
3: 110
4: 87
5: 22
6: 12
7: 7
8: 1
9: 23
10: 10
11: 3
12: 2
13: 1
14: 0
15: 1
16: 1
17: 3
18+: 0

Comment 7

[[e-mail]]

(In reply to comment #5)
> Oh, and the arena work is going on in bug 403830. It'll use the existing
> PLArena for now, but we'll probably write a new one once bug 403830 is landed.

i've run into the "need-an-easy-way-to-mark-and-deallocate-individual-arena-pages" problem before, so if we can make PLArenas more awesome and get some standardized support in for something like that, i'd love to help out. ;)

Comment 8

[[e-mail]]

Attachment: like so

pretty simple, the only slightly yuck part was in nsEventListenerManager::HandleEvent() - we save a copy of the entire mListeners array, then enter a loop that "does stuff". i'm not sure exactly what, because i'm no content expert, but i'm guessing the loop can modify the mListeners array itself (i put an XXX comment there). the old code checked to see if elements are removed by doing a pointer comparison between elements of the original and current mListener arrays, but now we've gotta do a full-on equality check between the nsListerStruct entries themselves. :(

this whole thing is O(n^2) anyway, and there are better ways to do it, but i'm gonna defer to the experts. sicking, jst, do you have any ideas? (at the very least, maybe the equality check doesn't need to compare all the elements of the struct...)

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  nsTArray<nsListenerStruct> originalListeners(mListeners);

Why isn't this an nsAutoTArray?

Comment 10

[Peter Van der Beken [:peterv]]

(In reply to comment #8)
> but i'm guessing the loop can modify the mListeners
> array itself (i put an XXX comment there).

If not, we might as well use a stack-based nsAutoTArray (with N=16?) there.

Comment 11

[[e-mail]]

(In reply to comment #9)
> Why isn't this an nsAutoTArray?

i ran into compilation errors making it an nsAutoTArray - turns out there's no copy constructor defined:
http://mxr.mozilla.org/mozilla/source/xpcom/glue/nsTArray.h#716

so we either need to add one or eat the malloc. (there'll only be one, at least, since the array can size appropriately on construction...)

Comment 12

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

Note, nsEventListenerManager::HandleEvent is pretty hot method.

How slow or fast is nsTArray<nsListenerStruct> originalListeners(mListeners)
comparing to 
PRInt32 count = mListeners.Count();
nsVoidArray originalListeners(count);
originalListeners = mListeners;

The answer to your XXX question is yes.
That loop is calling event listeners which may execute anything (C++ or JS ...)
and call AddEventListener or RemoveEventListener.

== operator must check all the other members except 
mHandlerIsString and mTypeData.

Why are you removing mListenersRemoved? It is actually needed, because
mListenerRemoved isn't set to PR_TRUE when disconnecting from mTarget.

Comment 13

[[e-mail]]

(In reply to comment #12)
> Note, nsEventListenerManager::HandleEvent is pretty hot method.

ok. maybe we want to add a copy constructor to nsAutoTArray then? :/

> How slow or fast is nsTArray<nsListenerStruct> originalListeners(mListeners)

if you mean compared to the old code, pretty much the same - both do one malloc, although with patch we'll be calling the copy constructors of the nsListenerStructs, which will have overhead since it's more data...

> == operator must check all the other members except 
> mHandlerIsString and mTypeData.

thanks, i'll make that change.

> Why are you removing mListenersRemoved? It is actually needed, because
> mListenerRemoved isn't set to PR_TRUE when disconnecting from mTarget.

you're right, i misread what the loop was doing with it. i'll add it back.

Comment 14

[Peter Van der Beken [:peterv]]

(In reply to comment #13)
> (In reply to comment #12)
> > Note, nsEventListenerManager::HandleEvent is pretty hot method.
> 
> ok. maybe we want to add a copy constructor to nsAutoTArray then? :/

No, just use AppendElements(const nsTArray<Item>& array), it'll only allocate if you need more than N elements for nsAutoTArray<..., N>

Comment 15

[[e-mail]]

Attachment: v2

add back mListenersRemoved; shorten operator==; use AppendElements() (duh!)

Comment 16

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

Is 32 too much? Does the value affect to the event dispatch 'depth' test
http://mozilla.pettay.fi/moztests/events/event_loop.html
(Currently trunk build on x86 linux gives result 150, 1.8.1 build only 12.
IIRC x86_64 trunk builds are > 90 too)

Comment 17

[[e-mail]]

(In reply to comment #16)
> Is 32 too much? Does the value affect to the event dispatch 'depth' test

the actual value is transparent to consumers; it only affects how much stack storage the array includes internally. i chose 32 to catch the largest array size in my experiment (comment 6) with some healthy headroom. 32 * 24 = 768, which should be fine, though i could drop this down to 24...

(also, i get 98 for your test on my trunk x86_64 linux build.)

Comment 18

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

(In reply to comment #17) 
> the actual value is transparent to consumers; it only affects how much stack
> storage the array includes internally.
That is exactly the problem why branch can't handle deep event dispatch loops.
Not that I've seen any bugs filed on that, but when redesigning event 
dispatching, I kept that in mind.

Comment 19

[[e-mail]]

w/ patch @ <, 32> i get 79, @ <, 24> i get 82. is this an acceptable tradeoff, given that we're avoiding a malloc?

Comment 20

[[e-mail]]

also @ <, 8> i get 92. so since keeping this small matters, maybe we should use that given it catches 98% of cases.

Comment 21

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

Would it make sense to use the same value as in mListeners?
Even 2 should catch >90%. You could #define the size in .h.
And btw, please align nsEventListenerManager member variables properly.

Comment 22

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

Given that we, in a pretty hot codepath, copy all listeners into another array, I'm not sure that this approach is a good one as we'd both end up copying much more data, and do some extra refcounting.

Either we should focus on moving the nsListenerStructs into an arena, or we should first eliminate the copying.

We could use a solution similar to nsTObserverArray, but we'd need to make sure that newly added observers won't get notified. This could actually be done and still reuse the existing nsTObserverArray I think.

Comment 23

[[e-mail]]

(In reply to comment #22)
> We could use a solution similar to nsTObserverArray

excellent! i'll look into it. this is exactly what i was wishing we had... ;)

Comment 24

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

I'm actually already working on such a patch. Though if you want to take over that work I wouldn't mind attaching what I have so far :)

The other question is, if we can move the nsListenerStructs into the array itself, do we really want to? The thing is that if we move them into the array we can't allocate them from an arena since nsTArray can't do that.

Though, come to think about it, it's possible that we could tweak nsTArray to support using arenas for allocations, but I'll have to look into that in a separate bug.

Comment 25

[[e-mail]]

yeah, anything we do with nsTArray or nsTObserverArray is gonna be orthogonal to arenas. it'd be neat to add support for custom allocators into our arrays. just tack on an extra template argument for a class that provides allocation and deallocation methods, and provide default allocators for people who don't care?

we might also switch nsTObserverArray over to nsTArray, and provide an nsAutoTObserverArray or somesuch... what do you think?

Comment 26

[[e-mail]]

Comment on attachment 292574 [details] [diff] [review]
v2

obsolete per nsTObserverArray.

Comment 27

[[e-mail]]

(by my last sentence in comment 25, i meant switch the internal implementation of nsTObserverArray.)

Comment 28

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

Unfortunately it's not as simple as tacking on another template argument. For one allocations currently happen in a non-templated function. Also, for arenas you'd also need an extra member holding a pointer to which arena is being used. But you wouldn't want to pay the price for that extra member in arrays that are not using an arena.

It's doable, but not simple. Patches accepted, though I have some ideas in mind that I might write up if it turns out that we can get big wins.

Comment 29

[[e-mail]]

i have a patch that makes nsTObserverArray inherit (protectedly) from nsTArray instead of nsVoidArray, and provides an nsAutoTObserverArray. this would let us implement the original suggestion of storing nsListenerStructs directly. if you want, i can file another bug to get that part in, and then the patch here will be trivial. ;)

Comment 30

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

Please attach it all here.

Comment 31

[[e-mail]]

Attachment: make nsTObserverArray use nsTArray

as advertised. (there's some extra newlines in there so diff would produce something more readable; please ignore them.)

this makes nsTObserverArray inherit from nsTArray, and does some API tweaking/additions to make things more or less consistent with nsTArray. (most of the inline API methods are copy/pasted from nsTArray, so nothing special's going on here.) i only implemented a subset of what nsTArray provides, since we don't want to get too fancy with removing/inserting ranges of elements (updating iterators would get tricky). so we still only support prepending and appending. i also expanded out the iterator GetNext() methods into IsMore()/GetNext(), since we don't know if the template type is a pointer or a concrete class and hence can't return null through GetNext().

for nsAutoTArray to work, it turns out nsTArray_base assumes the auto buffer is stored immediately after nsTArray_base::mHdr - so nsTObserverArray can't have any member variables (they'll get put in between). that's why there's a separate IteratorList container class which nsTObserverArray inherits from first. not that elegant, but it works :/

and of course, this also rolls in the ELM change with a <, 2> autoarray.

Comment 32

[[e-mail]]

with this patch, on 64bit, i get 101 on smaug's eventloop test.

Comment 33

[[e-mail]]

oh; i also added a Contains() method to nsTArray, since it seemed like a more readable form for the common use case IndexOf() != -1.

Comment 34

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

I'm not a fan of the nsTObserverArray changes here. First of all, moving code from the .cpp file to inline in the .h file is going to increase binary size a lot.

Not sure why you want to make nsTObserverArray inherit nsTArray, that seems to only give you hassle.

A better implementation would IMHO to make nsTObserverArray contain an nsTArray. If you make it the second member you'll avoid all the hassle you currently have with having to use two base classes.

Also, why is nsTObserverArray implementing so many functions? It doesn't need to be a general-purpose array class, so I'd rather just keep a minimal amount of code.

Comment 35

[[e-mail]]

(In reply to comment #34)
> I'm not a fan of the nsTObserverArray changes here. First of all, moving code
> from the .cpp file to inline in the .h file is going to increase binary size a
> lot.

agreed, it'd be nice to avoid that. i'll factor the two methods in question back out into the .cpp as much as i can.

> Not sure why you want to make nsTObserverArray inherit nsTArray, that seems to
> only give you hassle.

it's necessary for nsAutoTObserverArray... that needs access to the protected Header member of nsTArray_base, which is only possible through inheritance. i wish we could just have nsTArray as a member, but i don't see a way... anyway, it's not that big a deal, it's just the one base class (which we'll have anyway, per your above suggestion) :/

> Also, why is nsTObserverArray implementing so many functions? It doesn't need
> to be a general-purpose array class, so I'd rather just keep a minimal amount
> of code.

fair enough - i just wanted to keep the interfaces as consistent as possible, since there's no codesize cost to doing so. i'll trim it down.

Comment 36

[[e-mail]]

Attachment: it's all about nsAutoTArray

talked with sicking, and changed things around a bit:
- provides a specialization nsAutoTArray<T, 0> that just inherits from nsTArray
- makes ns{Auto}TObserverArray use nsAutoTArray<T, N> as a member, relying on the goodness of aforementioned specialization to do the "right thing" for plain ol' nsTObserverArray

this is a less hackish solution overall, and avoids the inheritance mess with nsTArray.

he also asked me to store the nsListenerStructs as ordinary pointers again rather than complete classes, though personally i'd rather see the latter :)

Comment 37

[[e-mail]]

Attachment: round 2

... and this one actually works :)

Comment 38

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

Comment on attachment 293606 [details] [diff] [review]
round 2

>Index: xpcom/glue/nsTArray.h
>+// specialization for N = 0. this makes the inheritance model easier for
>+// classes extending nsTArray.

Hmm.. this isn't entirely true, is it? I'd say that it makes it easier for templated users of nsAutoTArray.

>Index: xpcom/glue/nsTObserverArray.h
>+    // This method provides direct access to the i'th element of the array.
>+    // The given index must be within the array bounds.
>+    // @param i  The index of an element in the array.
>+    // @return   A reference to the i'th element of the array.
>+    elem_type& ObserverAt(index_type i) {
>+      return mArray.ElementAt(i);
>+    }
>+
>+    // Same as above, but readonly.
>+    const elem_type& ObserverAt(index_type i) const {
>+      return mArray.ElementAt(i);
>+    }

Feel free to rename these to ElementAt etc. Feel free not to though if it's too much work.


>+    PRBool Contains(const elem_type& elem) const {
>+      return IndexOf(elem) != base_type::NoIndex;
>+    }

Do what nsTArray does here and templetize the elem_type argument.

>+    index_type IndexOf(const elem_type& elem, index_type start = 0) const {
>+      return mArray.IndexOf(elem, start);
>+    }

Same here.

>+    index_type LastIndexOf(const elem_type& elem,
>+                           index_type start = base_type::NoIndex) const {
>+      return mArray.LastIndexOf(elem, start);
>+    }

I'd leave this one out entirely.

>+    elem_type* PrependObserver(const elem_type& elem) {
>+      elem_type* result = mArray.InsertElementAt(0, elem);
>+      if (result)
>+        AdjustIterators(0, 1);
>+      return result;
>     }

Kill this

>+    elem_type* PrependObserver() {
>+      elem_type* result = mArray.InsertElementAt(0);
>+      if (result)
>+        AdjustIterators(0, 1);
>+      return result;
>+    }

And this. I'm not a big fan of the PrependObserver function, so lets just keep the minimum number of them.

>+    elem_type* AppendObserver(const elem_type& elem) {
>+      return mArray.AppendElement(elem);
>     }

Templetize here too.


>+    PRBool AppendObserverUnlessExists(const elem_type& elem) {
>+      return Contains(elem) || AppendObserver(elem) != nsnull;
>     }

And here.

>+        PRBool operator <(const ForwardIterator& aOther) const {
>+          return iter_type::mPosition < aOther.mPosition;
>+        }

Assert that they're both iterating the same array.

>+        PRBool IsMore() const {
>+          return iter_type::mPosition < iter_type::mArray.Length();
>         }

Rename to HasMore()

>+        PRBool IsMore() const {
>+          return *this < mEnd;
>+        }

Same here.


> // XXXbz I wish I didn't have to pass in the observer type, but I
> // don't see a way to get it out of array_.

Add that this only works if the elements are pointers to XPCOM objects.


>Index: content/events/src/nsEventListenerManager.cpp

> nsEventListenerManager::RemoveAllListeners()
> {
>-  PRInt32 count = mListeners.Count();
>+  PRInt32 count = mListeners.Length();
>   for (PRInt32 i = 0; i < count; i++) {
>-    delete mListeners.FastObserverAt(i);
>+    delete mListeners.ObserverAt(i);
>   }
>   mListeners.Clear();
>   return NS_OK;
> }

This should only need to be mListeners.Clear() if you use nsAutoPtr<>s.

>@@ -1152,21 +1152,21 @@ nsEventListenerManager::HandleEvent(nsPr
>      }
>      typeData = nsnull;
>      dispData = nsnull;
>     }
>   }
> 
> found:
> 
>-  nsTObserverArray<nsListenerStruct>::EndLimitedIterator iter(mListeners);
>+  nsAutoTObserverArray<nsListenerStruct*, 2>::EndLimitedIterator iter(mListeners);

Try doing |mListeners.EndLimitedIterator iter(mListeners)| here and see if that compiles. If it doesn't no worries, we'll suffer through it ;)


>Index: content/events/src/nsEventListenerManager.h

>+  nsAutoTObserverArray<nsListenerStruct*, 2> mListeners;

Make this an nsAutoTObserverArray<nsAutoPtr<nsListenerStruct>, 2>

Gotta love templates ;)


>Index: modules/libpr0n/src/imgRequest.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/modules/libpr0n/src/imgRequest.cpp,v
>retrieving revision 1.101
>diff -u -8 -p -r1.101 imgRequest.cpp
>--- modules/libpr0n/src/imgRequest.cpp	14 Dec 2007 01:41:49 -0000	1.101
>+++ modules/libpr0n/src/imgRequest.cpp	18 Dec 2007 01:27:18 -0000
>@@ -326,21 +326,22 @@ void imgRequest::RemoveFromCache()
>   LOG_SCOPE(gImgLog, "imgRequest::RemoveFromCache");
> 
>   if (mCacheEntry) {
>     mCacheEntry->Doom();
>     mCacheEntry = nsnull;
>   }
> }
> 
>-PRBool imgRequest::HaveProxyWithObserver(imgRequestProxy* aProxyToIgnore) const
>+PRBool imgRequest::HaveProxyWithObserver(imgRequestProxy* aProxyToIgnore)

If you const-cast in the iterator ctor you should be able to keep the const here.

> NS_IMETHODIMP imgRequest::OnStartDecode(imgIRequest *request)
> {
>   LOG_SCOPE(gImgLog, "imgRequest::OnStartDecode");
> 
>   mState |= onStartDecode;
> 
>-  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
>+  nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mObservers);
>   imgRequestProxy* proxy;
>-  while ((proxy = iter.GetNext())) {
>+  while (iter.IsMore()) {
>+    proxy = iter.GetNext();
>     proxy->OnStartDecode();
>   }

If |proxy| isn't used after the loop here, just get rid of it entirely. Same in a bunch of places further down.


Looks great otherwise, awesome patch!

Comment 39

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

nsCSSLoader::mObservers manually addrefs and releases objects put into the array. Feel free to turn the array into an nsTObserverArray<nsCOMPtr<nsICSSLoaderObserver>> instead.

Comment 40

[[e-mail]]

Attachment: round 3

nits fixed. builds and runs.

note that nsCSSLoader previously didn't release the elements in the mObservers array on destruction, which nsCOMPtr-ifying it will now do. i guess it relied on all its listeners removing themselves before destruction, but i thought i'd point it out (i've made the change).

thanks for all the suggestions and help sicking!

Comment 41

[[e-mail]]

Comment on attachment 293633 [details] [diff] [review]
round 3

requesting approval1.9... this will cut thousands of allocations from startup+pageload.

Comment 42

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 293633 [details] [diff] [review]
round 3

a=beltzner for 1.9

Comment 43

[[e-mail]]

patch appears to have regressed linux Ts (bl-bldlnx03) 3-4ms. i'm going to try relanding in chunks to isolate the change.

Comment 44

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

You tried to reland this right? What were the results?

Comment 45

[[e-mail]]

i relanded the core bits of the patch - that is, the nsTArray/nsTObserverArray changes, and the required API tweaks to consumer code, but nothing else. (i.e. leaving off the ELM nsAutoTObserverArray switch, the ELM nsAutoPtr switch, and the  nsCSSLoader nsCOMPtr switch.) this part had a much smaller Ts impact (maybe 1-2ms, if any, and again only on bl-bldlnx03 - it's pretty much in the noise).

to see if i could get back that 1-2ms, i looked at the differences between nsVoidArray and nsTArray implementation, and tried two changes to nsTArray: a) switching NS_Alloc and friends to malloc (just in case), and b) changing the growth algorithm to allocate a minimum chunk of 8 entries, to match nsVoidArray. (this was my most likely guess, since using nsTArray would result in quite a few more reallocs than nsVoidArray with the thousands of tiny arrays we're probably dealing with.) a) had no effect, and i think b) probably won back that 1-2ms, but we didn't really get enough cycles to be sure. in any case, it's not something we necessarily want to do.

wrt the rest of the regression, not sure where that comes from yet. i'll try to reland pieces tonight, starting with the Auto change to ELM, and investigate.

Comment 46

[Walter Meinl]

Dan, OS/2 build breaks here
-Wp,-MD,.deps/nsTObserverArray.pp E:/usr/src/mozilla/xpcom/glue/nsTObserverArray.cpp
In file included from E:/usr/src/mozilla/xpco/glue/nsTObserverArray.cpp:39:
E:/usr/src/mozilla/xpcom/glue/nsTObserverArray.h:47: error: `typedef PRUint32 nsTObserverArray_base::size_type' is protected
E:/usr/src/mozilla/xpcom/glue/nsTObserverArray.h:354: error: within this context
Probably due that we are using gcc-3.3.5
When I make "typedef PRUint32 size_type" public it compiles. However, I don't know if that's allowed.

Comment 47

[[e-mail]]

thanks, i checked in a fix to make the typedef public.

alright, all of this patch is checked in now, with no perf impact... looks like it was noise, or something else going on.

in my investigations, though, i was disappointed to find that gcc on linux avoids inlining like the plague at -Os (our default). so every single method on nsTObserverArray and nsTArray get their own calls, even the trivial ones that just forward to another method. :(

this is bad for codesize and perf in this case. i wonder if we can force gcc to a different optimization level within a unit (perhaps a #pragma wrapping the relevant .h files)? in fact, a better solution would probably be to decorate a few methods with an NS_INLINE macro, that expands to __attribute__ ((always_inline)) on gcc? this might be useful in e.g. nsCOMPtr etc too. (gcc ignores the "inline" keyword at -Os, since it's just a hint.)

Comment 48

[[e-mail]]

in fact, we already have an NS_ALWAYS_INLINE:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/string/public/nsUTF8Utils.h&rev=1.17&mark=55-59#55

perhaps we should move that to a more fundamental header, and expand it if necessary for other plats?

cc'ing dbaron, since he added this initially.

Comment 49

[[e-mail]]

Mossop helped me test mac, which by default builds at -O2 (not sure if sayrer's going to change that, though). gcc inlines things properly there.

Comment 50

[[e-mail]]

Attachment: allocate from content arena, v1

here's a stab at using smaug's arena for allocating ELM's and nsListenerStructs. we don't always have easy access to an nsINode's allocator (e.g. from nsWindowRoot and nsGlobalWindow, the two layout/ instantiators of ELM), so in those cases we just use the global operator new/delete. i factored this logic into custom Create and Destroy methods (pretty sure we can't overload operator delete to do this, which would be nice, since there's no way to pass in mAllocator). almost all the ELMs in practice originate from content anyway, so not having an allocator for the layout ones isn't a big deal. (if there's a way to get to one, maybe via nsPIDOMWindow::GetExtantDocument() and then through to the node, i'd love to know. another possibility might be providing a single "catch-all" allocator instance on nsNodeInfoManager - stored as a static pointer in the .cpp, lazily inited and such - not sure if this is a good idea though, could get fragmented after long usage?)

Comment 51

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

nsWindowRoot and nsGlobalWindow aren't in layout/ but in dom/.
I don't see much use for a catch-all allocator - or isn't the normal
allocator (malloc) actually a 'catch-all'.
nsPIDOMWindow::GetExtantDocument could be used, it would probably
work almost always.

Comment 52

[[e-mail]]

(In reply to comment #51)
> nsWindowRoot and nsGlobalWindow aren't in layout/ but in dom/.

er, right, my mistake - i was thinking of the factory glue for those which lives in layout/build.

Comment 53

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

Comment on attachment 294540 [details] [diff] [review]
allocate from content arena, v1

>RCS file: /cvsroot/mozilla/content/base/src/nsDocument.cpp,v
...
>-  nsresult rv = NS_NewEventListenerManager(getter_AddRefs(mListenerManager));
>+  nsresult rv = NS_NewEventListenerManager(getter_AddRefs(mListenerManager), mNodeInfo->NodeInfoManager()->NodeAllocator());
Too long line?

>+nsEventListenerManager::Create(nsDOMNodeAllocator* aAllocator)
>+{
>+  nsEventListenerManager* elm;
>+  if (aAllocator) {
>+    void* mem = aAllocator->Alloc(sizeof(nsEventListenerManager));
>+    elm = new (mem) nsEventListenerManager(aAllocator);
>+  } else {
>+    elm = new nsEventListenerManager(nsnull);
>+  }
>+  return elm;
>+}
Nit, why not
{
  if (aAllocator) {
    void* mem = aAllocator->Alloc(sizeof(nsEventListenerManager));
    return new (mem) nsEventListenerManager(aAllocator);
  }
  return new nsEventListenerManager(nsnull);
}

>+nsListenerStruct*
>+nsListenerStruct::Create(nsDOMNodeAllocator* aAllocator)
Same here

Comment 54

[[e-mail]]

Comment on attachment 294540 [details] [diff] [review]
allocate from content arena, v1

thanks - nits fixed locally.

since we only hit the no-allocator codepath about 20 times when starting and creating a bunch of windows/tabs, it's probably not worth the bother to use GetExtantDocument. but if anyone can think of testcases that justify it, i can add it in.

Comment 55

[[e-mail]]

(In reply to comment #54)
> since we only hit the no-allocator codepath about 20 times

oops, make that ~100.

Comment 56

[[e-mail]]

regarding comment 48, i think the best thing to do is just have the compiler inline things sanely (tall order for gcc?). filed bug 409803 to do that.

Comment 57

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

Comment on attachment 294540 [details] [diff] [review]
allocate from content arena, v1

>Index: content/base/src/nsContentUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsContentUtils.cpp,v
>retrieving revision 1.263
>diff -u -8 -p -r1.263 nsContentUtils.cpp
>--- content/base/src/nsContentUtils.cpp	18 Dec 2007 21:24:54 -0000	1.263
>+++ content/base/src/nsContentUtils.cpp	25 Dec 2007 20:47:58 -0000
>@@ -3118,17 +3118,18 @@ nsContentUtils::GetListenerManager(nsINo
>                                         PL_DHASH_ADD));
> 
>   if (!entry) {
>     return NS_ERROR_OUT_OF_MEMORY;
>   }
> 
>   if (!entry->mListenerManager) {
>     nsresult rv =
>-      NS_NewEventListenerManager(getter_AddRefs(entry->mListenerManager));
>+      NS_NewEventListenerManager(getter_AddRefs(entry->mListenerManager),
>+                                 aNode->GetOwnerDoc()->NodeInfoManager()->NodeAllocator());

Actually, wouldn't it be better to use the nodes allocator rather than the nodes ownerdocs allocator? Seems like the listenermanager will always have the same lifetime as the node, even if the node is moved between documents, potentially multiple times.

Comment 58

[Mike Schroepfer]

Moving to blocking list so we can wrap up footprint work...

Comment 59

[[e-mail]]

Attachment: v2

with nits fixed. carrying over r=smaug.

Comment 60

[[e-mail]]

sicking - you mentioned you wanted to wait on this a bit until after MH work, or somesuch - do we want this for b3?

Comment 61

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

Yes we do. We should get bug 408720 fixed first though.

Comment 62

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

Comment on attachment 296408 [details] [diff] [review]
v2

>Index: content/events/public/nsIEventListenerManager.h
>-NS_NewEventListenerManager(nsIEventListenerManager** aInstancePtrResult);
>+NS_NewEventListenerManager(nsIEventListenerManager** aInstancePtrResult,
>+                           nsDOMNodeAllocator*       aAllocator = nsnull);

I'm not a big fan of default arguments unless there are lots of callsites that would get significantly more readable by not specifying the argument.

Looks good otherwise, but please wait with landing this until bug 408720 is fully landed.

Comment 63

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

One idea to reduce the size of ELM. The allocator could be get from mTarget, when
mTarget is nsINode. When setting mTarget to null, mTarget could be set to
point to allocator. Some bit handling/unions needed to implement this, but should
work.

Comment 64

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

Stuart says that the continued life of the content arena is uncertain still. So please hold off landing this until you've talked to him.

Comment 65

[Mike Schroepfer]

Dumb question - why is this bug blocked on the content arena bug?

Comment 66

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

To give us an opportunity to fix the regression before we start muddy the numbers by putting more stuff in the arena.

Comment 67

[[e-mail]]

stuart said we may end up backing out content arenas, so i'm going to hold off on this for now. we can land after b3 if necessary.

Comment 68

[Mike Schroepfer]

setting TM to match last comment

Comment 69

[[e-mail]]

Comment on attachment 296408 [details] [diff] [review]
v2

obsolete per bug 414894. we'll go full-circle back to the original suggestion of nsAutoTObserverArray<nsListenerStruct, 2> here.

Comment 70

[[e-mail]]

Attachment: store struct instance, v1 (checked in)

as advertised.

Comment 71

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

Comment on attachment 300994 [details] [diff] [review]
store struct instance, v1 (checked in)

>@@ -183,22 +182,22 @@ protected:
>   PRBool PrepareToUseCaretPosition(nsIWidget* aEventWidget,
>                                    nsIPresShell* aShell,
>                                    nsPoint& aTargetPt);
>   void GetCoordinatesFor(nsIDOMElement *aCurrentEl, nsPresContext *aPresContext,
>                          nsIPresShell *aPresShell, nsPoint& aTargetPt);
>   nsresult GetDOM2EventGroup(nsIDOMEventGroup** aGroup);
>   PRBool ListenerCanHandle(nsListenerStruct* aLs, nsEvent* aEvent);
> 
>-  nsAutoTObserverArray<nsAutoPtr<nsListenerStruct>, 2> mListeners;
>-  nsISupports*                                         mTarget;  //WEAK
>-  PRUint32                                             mMayHaveMutationListeners : 1;
>+  nsAutoTObserverArray<nsListenerStruct, 2> mListeners;
>+  nsISupports*                              mTarget;  //WEAK
>+  PRUint32                                  mMayHaveMutationListeners : 1;
>   // These two member variables are used to cache the information
>   // about the last event which was handled but for which event listener manager
>   // didn't have event listeners.
>-  PRUint32                                             mNoListenerForEvent : 31;
>-  nsCOMPtr<nsIAtom>                                    mNoListenerForEventAtom;
>+  PRUint32                                  mNoListenerForEvent : 31;
>+  nsCOMPtr<nsIAtom>                         mNoListenerForEventAtom;
> 
>-  static PRUint32                                      mInstanceCount;
>-  static jsval                                         sAddListenerID;
>+  static PRUint32                           mInstanceCount;
>+  static jsval                              sAddListenerID;

Can we please stop this insanity and not keep members aligned. It just makes patches impossible to read :(

r/sr=me either way

Comment 72

[Brendan Eich [:brendan]]

Or don't mix style fixes into substantive fixes. Or, beware aligning all members when only one bad boy type is pushing the declarator or declarator name way over (too far to be a good layout if that overlong type were not added). Let the one long type push its member over, leave the rest sanely aligned on a lower column number.

/be

Comment 73

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

This is ready for check-in, right?

Comment 74

[[e-mail]]

-> fixed, since we've done about all we can here. (we could perhaps use a fixed-size allocator for ELMs themselves, but meh - we've got most of the benefit here already)

looks like A went down by a few K after checkin.

Comment 75

[Boris Zbarsky [:bzbarsky]]

Note that the PrependElementUnlessExists method added here was broken: it didn't adjust iterator indices.  The patch in bug 474369 should fix it.

Comment 76

[Boris Zbarsky [:bzbarsky]]

And in particular, going from round 2 to round 3 of the nsTObserverArray patch lost the iterator index adjustment code.