See bug 377787 comment 4. Maybe we should use a bit to keep track of whether it's even worth looking in the binding manager's hashtable when traversing a generic element? After all, usually it's not.

Haven't profiled this properly, but nsBindingManager::Traverse seems to get called quite often even when manager doesn't have anything pointing to aContent. I got pretty easily something like 15000 extra calls during cc which with the patch become faster because of the early return. All the bit handling is in nsBindingManager to keep things easier to understand and maintain.

Note that with sicking's patch to short-circuit traversal of currently-displayed documents this may not be as much of a problem anymore...

GetBinding is called very often and quite often mBindingTable.IsInitialized() is true, so testing the flag should safe some time. At least it saves some 150000 calls to mBindingTable.GetWeak when loading ~25 pages from BBC to different tabs and closing the browser.

After Bug 378987 nsBindingManager::Traverse gets called a lot less. It is called now mainly during shutdown, but also occasionally when running for example Tdhtml. The patch should however speed up GetBinding, so I'd like to try it (and maybe back it out if nothing gets faster).

Comment on attachment 263760 [details] [diff] [review] check the bit also in GetBinding >Index: content/xbl/src/nsBindingManager.cpp >=================================================================== > nsXBLBinding* > nsBindingManager::GetBinding(nsIContent* aContent) > { >- if (mBindingTable.IsInitialized()) >+ if (aContent && aContent->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR) && Do we really need to check for null aContent? > void > nsBindingManager::Traverse(nsIContent *aContent, > nsCycleCollectionTraversalCallback &cb) > { >+ if (!aContent->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) { >+ return; >+ } >+ > nsXBLBinding *binding = GetBinding(aContent); > if (binding) { Maybe we should just return if GetBinding returns null? I'm ok with trying this, but if it doesn't help I'd rather back it out and free the flag for something else.

(In reply to comment #5) > > Do we really need to check for null aContent? I think I saw some crashes without it. I'll double check. > Maybe we should just return if GetBinding returns null? > That wouldn't work, binding manager has also other references than just content->binding

I think this dropped tp2 and tdhtml a bit on bl-bldlnx01/argo-vm and bl-bldxp01/fx-win32-tbox (as I expected). The change isn't big, so might be within the noise. Leaving the patch in for now.