After Bug 198533 there are now almost 700 NodeInfos alive after bringing up the first browser window. An arena for NodeInfos might speed up things a bit or at least it should prevent some fragmentation.

Something like this?

Why remove the single-slot nodeinfo cache?

This keeps the single-slot cache.

Comment on attachment 171568 [details] [diff] [review] v2 nsFixedSizeAllocator.h mentions that you should #include NEW_H, not sure how important it is, but it couldn't hurt. >Index: nsNodeInfo.cpp >+static const PRInt32 kNodeInfoPoolInitialSize = >+ (NS_SIZE_IN_HEAP(sizeof(nsNodeInfo))) * 1024; 1024 seems a little excessive in embedded environments where we don't use chrome. I would go for something like 32 or 64 rather. I'm guessing this won't make a big difference since once we've grown to a good size we're not going to realloc a lot, no? r=me with that

Comment on attachment 171639 [details] [diff] [review] v2 + comments Though I'm a bit hesitant to let this go in without any performance numbers.

Couldn't we just try this, and if it is decreasing performance, remove it.

checking in...

Doesn't look like this gave any real perf gain. However looking at the code for the fixedsize allocator I'm thinking that we could maybe get rid of the singleslot cache since the allocator is very fast at returning deallocated memory. With that this patch would actually be a (small) net codesimplification.

Ever since this patch went in I'm seeing a reproducible crash. Steps to reproduce problem: 1. Start Mozilla (suite) via the Profile Manager Note that starting with -P "name" does not crash. This happens when nsNodeInfo::LastRelease() NS_RELEASEs the last reference to mOwnerManager; when that's the last node manager that calls nsNodeInfo::ClearCache which frees the node info.

There is a good chance this caused bug 280456 and bug 280341

Would a fix be to simply remove the singleslot cache? If so I think we should.

This was backed out for now.

I can't reproduce my crash from Bug 280341 anymore with: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b) Gecko/20050131 Mnenhy/0.7.0.10002 {Build ID: 2005013109} first with backed out patch. Seems backing out has fixed the crasher allmost for me, thanks for that.

If someone is interested in landing this again I think we should just remove the singleslot cache. The nsFixedSizeAllocator is very fast at reclaiming memory from a deallocation so the singleslot cache probably has very little gain. And it might have been the source of the crasher.

Well, it's not very clear what gain the arena is giving us either. The single-slot cache addressed a specific issue, and it did improve some testcases IIRC.

True. OTOH, i think this patch along with a removed cache would actually amount to less code. I don't really care either way.

I'd like to retry this. The change to nsLayoutStatics.cpp is needed, since ClearCache shouldn't be called by anyone else than nsNodeInfoManager. Otherwise (almost) same as earlier patch. Btw, this still has the mIDAttributeAtom = nsnull; fix ;)

Comment on attachment 227054 [details] [diff] [review] removing single slot cache I think you need to remove the call to nsNodeInfo::ClearCache from nsLayoutStatics as that might get torn down before the last nodes are destroyed. > nsNodeInfo::ClearCache() > { > // Clear our cache. >- delete sCachedNodeInfo; >- sCachedNodeInfo = nsnull; >+ if (sNodeInfoPool) { >+ delete sNodeInfoPool; >+ sNodeInfoPool = nsnull; >+ } > } Nit: you don't really need to check if sNodeInfoPool is non-null here as |delete| is nullsafe. Up to you. > nsNodeInfo::LastRelease() ... >+ if (mOwnerManager) { >+ mOwnerManager->RemoveNodeInfo(this); >+ mOwnerManager = nsnull; > } > >- // There's space in the cache for one instance. Put >- // this instance in the cache instead of deleting it. >- sCachedNodeInfo = this; >+ NS_IF_RELEASE(mInner.mName); >+ NS_IF_RELEASE(mInner.mPrefix); >+ mIDAttributeAtom = nsnull; It might be cleaner to instead create a proper dtor that cleans up all members as usual, and then call the dtor manually here. (we should have don't that even with the single-slot cache). r/sr=me with that

dhtml jumped a bit, but then came down again. There were some network problems just when I checked in the patch.