Ok, now that bug 195262 has actually landed, I have converted over some consumers to use static atoms. This reduces both dynamic and static footprint in each module. It should also help startup performance because we have a tight loop to register the lists of static atoms.

and away we go!

Comment on attachment 118468 [details] [diff] [review] Convert most of layout over to static atoms dbaron, mind taking a look at this since you wrote the original nsAtomListUtils that I'm now removing? The patch is mostly code removal... the other changes are: * use gperf-generated files to test for member-ness of a few sets of atoms. I'm checking in .gperf files for now because gperf isn't installed by default on most platforms. * use of "wordlist" from the gperf files rather than declaring my own CSSPseudoClasses_info and so forth * the comments you see at the top of the header files are used by the gperf-generating script that I landed in the other bug thanks!

Why do you want to use gperf here?

I only use it in 3 places... stuff like IsCSSPseudoElement() - where we are testing for membership of a set.

To be a little more specific: some of the member test functions are currently unused, and none of them is a performance hotspot. (The unused ones are necessary to fix known bugs in the CSS parser, but those bugs haven't been fixed yet.) Even if they were performance problems, do you know that using gperf will actually speed things up? Iteration over a list of predetermined length is O(1) in all factors determined at runtime. In other words, why not just do what you did for nsMathMLAtoms, etc., for all the lists?

oh, I guess that's true... I guess I kind of assumed it was worth the performance gain, but who knows... I'll do this without gperf until we identify it as a problem...

ok, this converts over everything in content/shared/ and layout/mathml, which covers all the cases I know about.. In addition, I've started to walk around in content/ and found there are LOTS of modules that keep their own two or three or more references to little atoms that really could go into nsXBLAtoms:: for use as static atoms. I started with XBL. I've started to actually convert the XBL consumers over to use the nsXBLAtoms:: but at least this patch gets a bunch of the stuff XBL is using over to using static atoms. I'll do a diff of content/xbl/src seperately. With this patch, I see 978 atoms (the 41 came from the XBL stuff I was just mentioning) converted over to static atoms..wow!

Comment on attachment 118605 [details] [diff] [review] convert most of layout dbaron, do you mind reviewing? I took your advice and dropped gperf for now.. see my previous comments for the XBL_ATOM stuff

here's a sample of what I'm doing in XBL (this is only about half of the XBL stuff) I think there's stuff like this all over the content/ directory... Don't bother reviewing this one, but in case anyone is curious...

Comment on attachment 118607 [details] [diff] [review] get rid of private atoms in xbl This looks like it has been obsoleted by the patch on bug 199503.

Comment on attachment 118605 [details] [diff] [review] convert most of layout You've got too much stuff in your tree. :-) This has some changes from bug 199503 (some of the nsXBLAtomList.h changes). But it's also missing the definition of NS_ARRAY_LENGTH. (Moving it to some core location is probably a good thing. But where'd you put it? And don't forget to check it in when you land the patch...) Why not remove the |gRefCnt| and the |ReleaseAtoms| methods? (Will that pose a problem for accessibility's use of nsLayoutAtoms? I hope not...) Other than that, sr=dbaron.

I'm going to look into the accessiblity stuff - I think we can drop grefcnt/etc, as long as the AddRefAtoms() stuff is called from the module constructor there.. NS_ARRAY_LENGTH ended up in nsMemory.h with bug 195262 (the closest place with any relevant macros)

if you drop grefcnt you should probably rename AddRefAtoms() to InitAtoms() or some such

this one converts the known XPCOM atoms that are initialized at startup it also makes the hashtable inline, to save an extra allocation.

Comment on attachment 119301 [details] [diff] [review] convert nsDirectoryService requesting reviews from conrad and/or doug (using the superreview flag to request a review from either)

Comment on attachment 119301 [details] [diff] [review] convert nsDirectoryService > #elif defined (XP_BEOS) >- nsDirectoryService::sSystemDirectory = NS_NewAtom(NS_OS_SYSTEM_DIR); >- nsDirectoryService::sSettingsDirectory = NS_NewAtom(NS_BEOS_SETTINGS_DIR); >- nsDirectoryService::sHomeDirectory = NS_NewAtom(NS_BEOS_HOME_DIR); >- nsDirectoryService::sDesktopDirectory = NS_NewAtom(NS_BEOS_DESKTOP_DIR); >+ { NS_OS_SYSTEM_DIR, &nsDirectoryService::sSystemDirectory }, >+ { NS_BEOS_SETTINGS_DIR, &nsDirectoryService::sSettingsDirectory }, >+ { NS_BEOS_HOME_DIR, &nsDirectoryService::sHomeDirectory }, >+ { NS_BEOS_DESKTOP_DIR, &nsDirectoryService::sDesktopDirectory }, > #endif >+}; r=ccarlen Nit: I think the indentation of the old code made it easier to read. Can you add back some whitespace?

Comment on attachment 119301 [details] [diff] [review] convert nsDirectoryService I think that this patch my collide with what ssu is doing. Also, the mHashtable is no longer threadsafe. that is probably a bad thing. Also, i think that we increased the hashtable inital size since this table usually gets bloated up very quickly.

oops, I had forgotten to bring over those two parameters that we were using to make nshashtable threadsafe/etc... here's the update. (with indentation fixed too)

Comment on attachment 119323 [details] [diff] [review] convert nsDirectoryService 1.01 doug - I fixed the constructor to mHashtable, thanks for catching that.. other that that its the same as the previous patch. mind reviewing?

Comment on attachment 119323 [details] [diff] [review] convert nsDirectoryService 1.01 yeah. looks good.

so after the above two patches landed, I spent some time watching what atoms are still being created, and then tracked down which consumers were creating them. This patch handles about 60% of all dynamic atoms, and gets rid of a TON of churn due to atoms being created and destroyed, such as during the parsing of the chrome registry files. looking for reviews...

Comment on attachment 120009 [details] [diff] [review] convert more consumers Boris/Brian - do you mind reviewing this static atom conversion? its really straight forward, reduces bloat, and should give us a Ts improvement as well because I reduce atom churn signifigantly.

this converts most of editor over to static atoms

Comment on attachment 120092 [details] [diff] [review] convert editor Kathy/Kin, can I get a review on this? its a footprint/performance improvement for editor!

Comment on attachment 120092 [details] [diff] [review] convert editor In your new file, fix this typo: + All entires r=brade

And here's widget's conversion..

Comment on attachment 120104 [details] [diff] [review] convert widget this is an easy one.. trying to spread the review lovin' on these reviews...

Looking at the nsGenericFactory.cpp, we seem to return false for CanUnload for modules by default, and unloading doesn't work at all, right? But just to make sure, modules using static atoms do have to return false, so if we ever make unloading work, we need to pay attention to the static atoms, right?

right, we are years away from any module being able to return TRUE to canUnload... this certainly doesn't help the situation, but it doesn't make it any worse either (and it will just be some extra work to "unregister" the static atoms - but not worth taking the footprint hit until we're trying to tackle THAT problem)

Comment on attachment 120104 [details] [diff] [review] convert widget looks ok to me r=pink

Comment on attachment 120009 [details] [diff] [review] convert more consumers alec mentioned to me that there's another part to the chrome registry patch where scPrefix is actually used. sr=bryner for the whole thing.

The past patch broke Cocoa builds (Camino tbox is red). Here's a fix.

Cocoa toolkit patch checked in.

Comment on attachment 120009 [details] [diff] [review] convert more consumers lets try conrad - this is a lot of service provider stuff like I did in xpcom.. and bryner is the other reviewer so the nsChromeRegistry stuff is "covered" as far as module owner stuff...

ok, enough work has been done here to justify closing this. At some point we need to file bugs against: - xmlextra's schema loader - mailnews (or maybe that was already filed) - inspector - IME? anyway, see http://lxr.mozilla.org/seamonkey/search?string=NS_NewAtom%5C%28%5C%22 for possible candidates.