847236 - Simplify the dead nsEventTargetChainItem objects cache

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Nicholas Nethercote [inactive]]

nsEventDispatcher.cpp has a complex memory allocation strategy for
nsEventTargetChainItems (ETCIs).

- It has |sEtciPool|, which is an nsFixedSizeAllocator, and |gCachedETCI|,
  which is a list of no-longer-used ETCIs.

- In nsEventTargetChainItem::Create, if |gCacheETCI| is non-empty we recycle
  one from there;  otherwise, we allocate space from |sEtciPool|.  Then we call
  the constructor on the memory.

- In nsEventTargetChainItem::Destroy, we call the ETCI's destructor and then
  put it onto the |gCacheETCI| list.  (Likewise for all of the ETCI's
  ancestors.)

- We keep track of the maximum number of live ETCIs, using |sCurrentEtciCount|
  and |sMaxEtciCount|.  Each time control flow leaves Dispatch() we check if
  the max number of live ETCIs has exceeded NS_CHAIN_POOL_SIZE (which is 128).
  If it has, and there are no live ETCIs (which we track via |sEtciPoolUsers|,
  which is necessary because Dispatch() is indirectly recursive) we destroy
  |sEtciPool| and empty |gCacheETCI|.

In other words, we hold onto up to 128 dead ETCIs for recycling.  What a
horribly complex way of doing it -- it took me about an hour of reading and
re-reading this code to realize it was doing something so conceptually simple.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Simplify the dead nsEventTargetChainItem objects cache.

This patch simplifies the dead ETCI cache greatly.

- It removes the nsFixedSizeAllocator;  vanilla malloc/free suffices.

- It encapsulates the dead ETCI entirely cache within nsEventTargetChainItem.
  In particular, it keeps track of the 128 limit in Destroy(), rather than upon
  leaving Dispatch().  This has several benefits.
    
  - No need for ChainItemPool (including |sEtciPoolUsers|).

  - No need to flush the entire cache when it fills up.

  - The 128 limit is a hard limit;  with the previous code we could exceed it
    when nested Dispatch() calls were in flight.

The only downside is that at shutdown we have to free the dead cache one ETCI
at a time, instead of freeing the whole pool at once.  But it's small enough
that this won't matter.

(BTW, I was tempted to just remove the dead ETCI cache altogether, but I figure
this code is fairly hot so I decided to be cautious.)

Comment 2

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

Comment on attachment 720486 [details] [diff] [review]
Simplify the dead nsEventTargetChainItem objects cache.

>+  // This is the dead ETCI cache, which is used to avoid some malloc/free
>+  // churn.  It's implemented as a linked list.
>+  static nsEventTargetChainItem* sDeadEtcis;
>+  static uint32_t sNumDeadEtcis;
Could you please initialize these explicitly to nullptr/0

And could you use term etciCache or some such. I don't like the work 'dead' here.

>+  static void ShutdownCache() {
{ goes to the next line

>+    while (sDeadEtcis) {
>+      nsEventTargetChainItem *tmp = sDeadEtcis;
nsEventTargetChainItem* tmp


I'd like to see a new patch without "dead" object cache, but just with cache

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Simplify the dead nsEventTargetChainItem objects cache.

I renamed it the "ETCI recycle pool".

Comment 4

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

Comment on attachment 720939 [details] [diff] [review]
Simplify the dead nsEventTargetChainItem objects cache.


>+      sNumRecycledEtcis--;
--sNumRecycledEtcis;

>+      if (sNumRecycledEtcis < kMaxNumRecycledEtcis) {
>+        item->mNext = sEtciRecyclePool;
>+        sEtciRecyclePool = item;
>+        sNumRecycledEtcis++;
++sNumRecycledEtcis

>+      } else {
>+        free(item);
>+      }
>       item = parent;
>     }
>   }
> 
>+  static void ShutdownRecyclePool() {
{ goes to the next line

>+    while (sEtciRecyclePool) {
>+      nsEventTargetChainItem *tmp = sEtciRecyclePool;
nsEventTargetChainItem* tmp

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/74eaeeec2b9d

Comment 6

[Ryan VanderMeulen [:RyanVM]]

It appears Windows didn't like this. Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3e79c49d75a

https://tbpl.mozilla.org/php/getParsedLog.php?id=20322640&tree=Mozilla-Inbound

19:37:38     INFO -  1141 INFO TEST-START | /tests/browser/base/content/test/test_contextmenu.html
19:37:39  WARNING -  TEST-UNEXPECTED-FAIL | /tests/browser/base/content/test/test_contextmenu.html | Exited with code -1073741819 during test run
19:37:39  WARNING -  This is a harness error.
19:37:39     INFO -  INFO | automation.py | Application ran for: 0:00:15.844000
19:37:39     INFO -  INFO | automation.py | Reading PID log: c:\docume~1\cltbld\locals~1\temp\tmpwhhojtpidlog
19:37:39     INFO -  ==> process 132 launched child process 1952
19:37:39     INFO -  INFO | automation.py | Checking for orphan process with PID: 1952
19:37:39     INFO -  Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1362450708/firefox-22.0a1.en-US.win32.crashreporter-symbols.zip
19:37:53     INFO -  PROCESS-CRASH | /tests/browser/base/content/test/test_contextmenu.html | application crashed [@ msvcr100.dll + 0x1f18]
19:37:53     INFO -  Crash dump filename: c:\docume~1\cltbld\locals~1\temp\tmpu5dw73\minidumps\1446917a-8f71-4a3d-959b-a1003c678513.dmp
19:37:53     INFO -  Operating system: Windows NT
19:37:53     INFO -                    5.1.2600 Service Pack 2
19:37:53     INFO -  CPU: x86
19:37:53     INFO -       GenuineIntel family 6 model 23 stepping 10
19:37:53     INFO -       2 CPUs
19:37:53     INFO -  Crash reason:  EXCEPTION_ACCESS_VIOLATION_WRITE
19:37:53     INFO -  Crash address: 0x11e12499
19:37:53     INFO -  Thread 0 (crashed)
19:37:53     INFO -   0  msvcr100.dll + 0x1f18
19:37:53     INFO -      eip = 0x78aa1f18   esp = 0x0012d274   ebp = 0x0012d27c   ebx = 0x00000001
19:37:53     INFO -      esi = 0x0012d2e4   edi = 0x11e12499   eax = 0x00000058   ecx = 0x00000001
19:37:53     INFO -      edx = 0x00000001   efl = 0x00010202
19:37:53     INFO -      Found by: given as instruction pointer in context
19:37:53     INFO -   1  xul.dll!AssignRangeAlgorithm<1,1>::implementation<nsHttpConnection *,nsHttpConnection *,unsigned int,unsigned int>(nsHttpConnection * *,unsigned int,unsigned int,nsHttpConnection * const *) [nsTArray.h:74eaeeec2b9d : 475 + 0x18]
19:37:53     INFO -      eip = 0x014b10b8   esp = 0x0012d284   ebp = 0x0012d290
19:37:53     INFO -      Found by: previous frame's frame pointer
19:37:53     INFO -   2  xul.dll!nsTArray_Impl<mozilla::dom::indexedDB::PIndexedDBDeleteDatabaseRequestChild *,nsTArrayInfallibleAllocator>::AppendElements<mozilla::dom::indexedDB::PIndexedDBDeleteDatabaseRequestChild *>(mozilla::dom::indexedDB::PIndexedDBDeleteDatabaseRequestChild * const *,unsigned int) [nsTArray.h:74eaeeec2b9d : 960 + 0xf]
19:37:53     INFO -      eip = 0x01475889   esp = 0x0012d298   ebp = 0x0012d2b4
19:37:53     INFO -      Found by: call frame info
19:37:53     INFO -   3  xul.dll!nsContentList::nsContentList(nsINode *,int,nsIAtom *,nsIAtom *,bool) [nsContentList.cpp:74eaeeec2b9d : 461 + 0x1b]
19:37:53     INFO -      eip = 0x015044cf   esp = 0x0012d2bc   ebp = 0x0012d2cc
19:37:53     INFO -      Found by: call frame info
19:37:53     INFO -   4  xul.dll!nsHTMLDocument::GetForms() [nsHTMLDocument.cpp:74eaeeec2b9d : 2382 + 0x26]
19:37:53     INFO -      eip = 0x01617527   esp = 0x0012d2d4   ebp = 0x0012d308
19:37:53     INFO -      Found by: call frame info
19:37:53     INFO -   5  xul.dll!nsHTMLDocument::Forms() [nsHTMLDocument.h:74eaeeec2b9d : 194 + 0xa]
19:37:53     INFO -      eip = 0x01bc0da1   esp = 0x0012d2f4   ebp = 0x0012d308
19:37:53     INFO -      Found by: call frame info
19:37:53     INFO -   6  xul.dll!mozilla::dom::HTMLDocumentBinding::get_forms [HTMLDocumentBinding.cpp:74eaeeec2b9d : 366 + 0xa]
19:37:53     INFO -      eip = 0x01bc17db   esp = 0x0012d2f8   ebp = 0x0012d308
19:37:53     INFO -      Found by: call frame info
19:37:53     INFO -   7  xul.dll!mozilla::dom::HTMLDocumentBinding::genericGetter [HTMLDocumentBinding.cpp:74eaeeec2b9d : 1587 + 0xe]
19:37:53     INFO -      eip = 0x01bc229b   esp = 0x0012d310   ebp = 0x0012d344
19:37:53     INFO -      Found by: call frame info
19:37:53     INFO -   8  mozjs.dll!js::InvokeKernel(JSContext *,JS::CallArgs,js::MaybeConstruct) [jsinterp.cpp:74eaeeec2b9d : 383 + 0x4a]
19:37:53     INFO -      eip = 0x00b1a0df   esp = 0x0012d34c   ebp = 0x0012d390
19:37:53     INFO -      Found by: call frame info

Comment 7

[Nicholas Nethercote [inactive]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #6)
> It appears Windows didn't like this. Backed out.

I did a try run: https://tbpl.mozilla.org/?tree=Try&rev=a966129f82b8.  The results were murky because the trees were having fits at the time, but the "Win debug" builds at least were all green at the time.  Huh.  I'll do another try run.

Comment 8

[Nicholas Nethercote [inactive]]

> I'll do another try run.

This full try run was green:  https://tbpl.mozilla.org/?tree=Try&rev=55ede52dbc6f.  I'll re-land tomorrow.

Comment 9

[Nicholas Nethercote [inactive]]

Fingers crossed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc9c169f28c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8dc9c169f28c