Deleting this hashtable and recreating it shows up in profiles of bug 1346723. Perhaps we should consider not doing this extra work given how quickly selections can be changed by the user and script. This can cause us to use a bit more memory but this hashtable should be very small in most cases. The patch is simple, please feel free to r- if you disagree with the change.

Comment on attachment 8891458 [details] [diff] [review] Do not delete the common ancestor ranges hashtable in nsRange::UnregisterCommonAncestor() because chances are we need to recreate it shortly after This obviously wouldn't work, we need to remove the entry as well.

Comment on attachment 8891598 [details] [diff] [review] Do not delete the common ancestor ranges hashtable in nsRange::UnregisterCommonAncestor() because chances are we need to recreate it shortly after I guess we can try this, but I wonder about the memory use. I think I'd prefer to limit this to native anonymous content, for now.

Pushed by eakhgari@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ddc59dc7b92d Do not delete the common ancestor ranges hashtable in nsRange::UnregisterCommonAncestor() because chances are we need to recreate it shortly after; r=smaug

Comment on attachment 8896796 [details] [diff] [review] Do not delete the common ancestor ranges hashtable in nsRange::UnregisterCommonAncestor() because chances are we need to recreate it shortly after >+ bool isNativeAnon = aNode->IsInNativeAnonymousSubtree(); >+ bool removed = false; >+ > if (ranges->Count() == 1) { > aNode->ClearCommonAncestorForRangeInSelection(); >- aNode->GetCommonAncestorRangesPtr().reset(); >+ if (!isNativeAnon) { >+ // For nodes which are in native anonymous subtrees, we optimize away the >+ // cost of deallocating the hashtable here because we may need to create >+ // it again shortly afterward. We don't do this for all nodes in order >+ // to avoid the additional memory usage unconditionally. >+ aNode->GetCommonAncestorRangesPtr().reset(); I'm confused at here. The comment says, "For nodes which are *in* native anonymous subtrees", but the block is used when IsInNativeAnonymousSubtree() returns *false*. Do I misunderstand something??

(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #8) > Comment on attachment 8896796 [details] [diff] [review] > Do not delete the common ancestor ranges hashtable in > nsRange::UnregisterCommonAncestor() because chances are we need to recreate > it shortly after > > >+ bool isNativeAnon = aNode->IsInNativeAnonymousSubtree(); > >+ bool removed = false; > >+ > > if (ranges->Count() == 1) { > > aNode->ClearCommonAncestorForRangeInSelection(); > >- aNode->GetCommonAncestorRangesPtr().reset(); > >+ if (!isNativeAnon) { > >+ // For nodes which are in native anonymous subtrees, we optimize away the > >+ // cost of deallocating the hashtable here because we may need to create > >+ // it again shortly afterward. We don't do this for all nodes in order > >+ // to avoid the additional memory usage unconditionally. > >+ aNode->GetCommonAncestorRangesPtr().reset(); > > I'm confused at here. The comment says, "For nodes which are *in* native > anonymous subtrees", but the block is used when IsInNativeAnonymousSubtree() > returns *false*. Do I misunderstand something?? This is a double negative. If the node is not in a native anonymous subtree, we delete the hashtable, otherwise (that is, if the node is inside a native anonymous subtree) we leave the hashtable alone in the hope that it will be reused later.