libpref is mallochappy and allocates lots of little bytes.. for example, it allocates a PrefNode for each pref: typedef struct { PrefValue defaultPref; PrefValue userPref; PRUint8 flags; } PrefNode; (PrefValue is a union 4 bytes long) Thats 9 bytes, probably rounded up to either 12 or 16 bytes depending on the allocator. it also makes a copy of the pref name, which varies in size, but is probably 20-50 bytes long. If the pref is a string, it stores the string value, which also varies in length, but is probably 10-80 bytes long. Finally, it makes a PLHashEntry, which is probably 16 bytes. My plan is to either use one big arena, or use one arena for the strings, and nsFixedSizeAllocator for a combined PrefNode / PLHashEntry. Wish me luck.

First step is making prefapi compilable as C++. I needed to add a bunch of casts.

I needed to make some minor changes to make sure extern's matched up, etc. Platform issues: - Win32: I did not need to change makefile.win because the windows build system automatically picks up which file to compile (i.e. prefapi.cpp, when it appears) - Unix: had to modify Makefile.in - Mac: had to modify projects Looking for reviews.

I'd like to get these patches in 0.9.7 so I can start the 'real' work for 0.9.7. I'm looking for reviewers, can I get r=dp or r=bnesse and then sr=dveditz? see my earlier comments about the platform changes (specifically, why I changed Makefile.in but not makefile.win)

sr=dveditz

Comment on attachment 59758 [details] [diff] [review] add casts so that prefapi.c can be renamed to prefapi.cpp Boy there's a lot of crap in this file that should go away... r=bnesse.

Comment on attachment 59759 [details] [diff] [review] rest of patch required to switch the module over r=bnesse.

thanks guys - the first patch and the prefapi_private_data.h file have been checked in. The rest will wait for a carpool or some sort of cross-platform coordination tomorrow.

how much footprint we shave off? :-)

This checkin was just converting the file from C to C++ as a prelude to any actual improvement.

right... and after that, I'm switching over to PLDHash (I'm sure brendan & shaver will be happy) with the default allocators. That ought to reduce the NUMBER of allocations by about 25% and the TOTAL SIZE of THOSE allocations by about 22%... Basically I'll be combining the hashentry (16 bytes) and the PrefNode (9 bytes) into a unified structure that is only 17 bytes. I've got that much in my tree right now (but requires some of this C++ juggling first) Then I'll switch over to nsFixedSizedAllocator for these 17-byte objects. After that, we will have lots of variable length strings, and I'm not sure what the best course of action there is. All the strings aren't handed to us right away. I'm thinking that I'll use an arena or two for them.

Awesome. Please reduce the inital size of the hash table from 2048 to something like 256. That should suffice.

there are 733 default prefs in the default mozilla build. I'm going to set it to 1024 for now. I realize that yes, it will grow as necessary, but each growth requires an allocate-and-copy on each growth, so I don't see the benefit of making it significantly smaller than the number of prefs. besides, if I set it to 256, it will likely double twice to 1024 anyway, so we might as well start there. Also, mail creates a number of dynamic prefs which don't exist in the default prefs (i.e. a whole set of about 20+ for each mail account) and the commercial build may have more as well. These pref objects are going to be about 20 bytes, so making it 1024 vs. 733 only wastes about 5k of one-time space.

ok, I'm not sure how I pulled this off, but I feel intimately familiar with pldhash now :) Anyway, this patch converts us over to PLDHash and we'll be much less malloc-happy. After I land _this_, then I'll start using nsFixedSizedAllocator to allocate the PrefEntry structs.

adding brendan and shaver for PLDHash-related reviews. c'mon, you know you wanted someone to give libpref a spanking.

Comment on attachment 59905 [details] [diff] [review] switch prefs over to pldhash Remove, don't comment out: -#include "plhash.h" +/*#include "plhash.h"*/ +#include "pldhash.h" You might rather use PLDHashEntryHdr as the public base class and declare your own char *key; member. Then use NS_STATIC_CAST to downcast from PLDHashEntryHdr* to your Entry*. More commenting out whinage from me -- I trust you to clean up later, but why wait? -PLHashTable* gHashTable = NULL; +PRBool gHashTableInitialized = PR_FALSE; +//PLHashTable* gHashTable = NULL; +PLDHashTable gHashTable = { 0 }; PRBool gEnumeratingHashTable = PR_FALSE; Hmm, I thought & in front of a function name was redundant in C and meant something else in C++? Maybe it's just redundant in both. + &matchPrefEntry, + PL_DHashMoveEntryStub, + &clearPrefEntry, It's fair to use test whether .ops is null to decide when to initialize a static PLDHashTable, so you can get rid of gHashTableInitialized. Just take care to clear .ops on Init failure and after any Finish call. /be

oops, sorry 'bout those comments - I had meant to take at least those out :) Anyway, I addressed everything you mentioned - I like the NS_STATIC_CAST thing - switching over actually found one place where I was casting away const-ness.

This patch makes it so that we use nsCAutoString and friends instead of PR_smprintf and other malloc-happy functions, such as str_escape.

Comment on attachment 59914 [details] [diff] [review] switch prefs over to pldhash, v2 >-PR_STATIC_CALLBACK(PRIntn) pref_enumChild(PLHashEntry *he, int i, void *arg) >+PR_STATIC_CALLBACK(PLDHashOperator) >+ pref_enumChild(PLDHashTable *table, PLDHashEntryHdr *heh, >+ PRUint32 i, void *arg) > { >+ PLDHashEntryStub *he = (PLDHashEntryStub*)heh; Why not NS_STATIC_CAST(PrefHashEntry*, heh) here, and he->key without the (char*) cast below? > EnumerateData *d = (EnumerateData *) arg; NS_REINTERPRET_CAST for style points. >+typedef struct : PLDHashEntryHdr > { >+ char *key; >+ PrefValue defaultPref; >+ PrefValue userPref; >+ PRUint8 flags; > >+} PrefHashEntry; Now that these files are written in C++, no need to typedef an anonymous struct, just move the PrefHashEntry typename to after the struct keyword. (Get rid of the empty line after the last member, while you're at it.) >+void PR_CALLBACK >+clearPrefEntry(PLDHashTable *table, PLDHashEntryHdr *entry) > { >+ PrefHashEntry *pref = NS_STATIC_CAST(PrefHashEntry *, entry); > if (pref) > { > if (pref->flags & PREF_STRING) No need to null-check pref (or entry) here. >+PRBool PR_CALLBACK >+matchPrefEntry(PLDHashTable*, const PLDHashEntryHdr* entry, >+ const void* key) >+{ >+ const PrefHashEntry *prefEntry = >+ NS_STATIC_CAST(const PrefHashEntry*,entry); >+ >+ if (prefEntry->key == key) return PR_TRUE; >+ >+ if (!prefEntry->key || !key) return PR_FALSE; >+ >+ return (strcmp((const char*)prefEntry->key, (const char*)key) == 0); Ick, no need for (const char*) cast in front of prefEntry->key. Mebbe declare a const char* otherKey = NS_REINTERPRET_CAST(const char*, key) local to match? >+PLDHashTable gHashTable = { 0 }; s/0/nsnull/ > if (!gMochaTaskState) > { >+ Extra spaces at end of empty line, and this added empty line seems useless to me (it's unconventional after an opening brace, esp. a brace on its own line, to add yet another vertical separator). Nit-pick worth fixing? >@@ -738,18 +740,18 @@ > if (tmp_str) > { > prefEntry = PR_smprintf("user_pref(\"%s\", \"%s\");", >- (char*)he->key, tmp_str); >+ (char*)pref->key, tmp_str); No need for (char*) before pref->key -- check elsewhere. >+PR_STATIC_CALLBACK(PLDHashOperator) >+pref_ClearUserPref(PLDHashTable *table, PLDHashEntryHdr *he, PRUint32, >+ void *arg) > { >- PrefNode *pref = (PrefNode *) he->value; >+ PrefHashEntry *pref = NS_STATIC_CAST(PrefHashEntry*, he); > PR_ASSERT(pref); > > if (pref && No need for null checks here. >+static inline PrefHashEntry* pref_HashTableLookup(const void *key) > { >- return gEnumeratingHashTable ? >- PL_HashTableLookupConst(gHashTable, key) : >- PL_HashTableLookup(gHashTable, key); Get rid of gEnumeratingHashTable? >+ PrefHashEntry* result = >+ NS_STATIC_CAST(PrefHashEntry*,PL_DHashTableOperate(&gHashTable, key, PL_DHASH_LOOKUP)); Space after that first comma? Mebbe put the PL_DHashTableOperate call on its own line for clarity. >@@ -1483,13 +1481,13 @@ > (JSContext *cx, JSObject *obj, unsigned int argc, jsval *argv, jsval *rval) > { > /*void* value = NULL;*/ >- PrefNode* pref; >+ PrefHashEntry* pref; > /*PRBool prefExists = PR_TRUE;*/ > > if (argc >= 1 && JSVAL_IS_STRING(argv[0])) > { > const char *key = JS_GetStringBytes(JSVAL_TO_STRING(argv[0])); >- pref = (PrefNode*) pref_HashTableLookup(key); >+ pref = NS_STATIC_CAST(PrefHashEntry*, pref_HashTableLookup(key)); No need to cast here, eh? /be

all comments addressed again - and was proactive about avoiding unnecessary casts I had forgotten, when I switched to making PrefHashEntry inherit from PLDHashEntryHdr, to look in nsPrefBranch.cpp for uses of PLDHashEntryStub.. that's why there were all those unnecessary casts in order to share the structure between the files, I moved it into prefapi.h. hows this one look?

Comment on attachment 60189 [details] [diff] [review] switch prefs over to pldhash, v3 >@@ -817,13 +820,16 @@ > return NS_OK; > } > >-PR_STATIC_CALLBACK(PRIntn) pref_enumChild(PLHashEntry *he, int i, void *arg) >+PR_STATIC_CALLBACK(PLDHashOperator) >+ pref_enumChild(PLDHashTable *table, PLDHashEntryHdr *heh, >+ PRUint32 i, void *arg) > { >- EnumerateData *d = (EnumerateData *) arg; >+ PrefHashEntry *he = NS_STATIC_CAST(PrefHashEntry*,heh); >+ EnumerateData *d = NS_REINTERPRET_CAST(EnumerateData *, arg); > if (PL_strncmp((char *)he->key, d->parent, PL_strlen(d->parent)) == 0) { No need for (char *) above? > d->pref_list->AppendElement((void *)he->key); NS_REINTERPRET_CAST for consistency? >@@ -1070,19 +1035,20 @@ > #endif /* XP_MAC */ > > /* Delete a branch. Used for deleting mime types */ >-int PR_CALLBACK >-pref_DeleteItem(PLHashEntry *he, int i, void *arg) >+PLDHashOperator PR_CALLBACK >+pref_DeleteItem(PLDHashTable *table, PLDHashEntryHdr *heh, PRUint32 i, void *arg) > { >+ PrefHashEntry* he = NS_STATIC_CAST(PrefHashEntry*,heh); > const char *to_delete = (const char *) arg; > int len = PL_strlen(to_delete); > > /* note if we're deleting "ldap" then we want to delete "ldap.xxx" > and "ldap" (if such a leaf node exists) but not "ldap_1.xxx" */ >- if (to_delete && (PL_strncmp((char*)he->key, to_delete, (PRUint32) len) == 0 || >- (len-1 == (int)PL_strlen((char*)he->key) && PL_strncmp((char*)he->key, to_delete, (PRUint32)(len-1)) == 0))) >- return HT_ENUMERATE_REMOVE; >+ if (to_delete && (PL_strncmp(he->key, to_delete, (PRUint32) len) == 0 || >+ (len-1 == (int)PL_strlen(he->key) && PL_strncmp(he->key, to_delete, (PRUint32)(len-1)) == 0))) >+ return PL_DHASH_REMOVE; > else No else after return non-sequiturs. >- return HT_ENUMERATE_NEXT; >+ return PL_DHASH_NEXT; > } > > PrefResult Ok, finally a non-nit! Sorry for all the fluff, here's some meat: > PrefResult pref_HashPref(const char *key, PrefValue value, PrefType type, PrefAction action) > { >- PrefNode* pref; >+ PrefHashEntry* pref; > PrefResult result = PREF_OK; > >- if (!gHashTable && !pref_useDefaultPrefFile()) >+ if (!gHashTable.ops) > return PREF_NOT_INITIALIZED; > >- pref = (PrefNode*) pref_HashTableLookup(key); >+ pref = pref_HashTableLookup(key); > if (!pref) > { >- pref = (PrefNode*) calloc(sizeof(PrefNode), 1); >+ pref = NS_STATIC_CAST(PrefHashEntry*, PL_DHashTableOperate(&gHashTable, key, PL_DHASH_ADD)); You shouldn't have to hash twice (once for the Lookup, and again for the PL_DHASH_ADD). Just call PL_DHashTableOperate with PL_DHASH_ADD, and if it succeeds (doesn't fail with OOM), test pref->key or another initialized by default to zero member to tell whether you are making a new entry, of hitting an existing one. Fix the above and sr=brendan@mozilla.org /be

bnesse, do you mind reviewing?

Comment on attachment 60184 [details] [diff] [review] use ns*AutoStrings to write prefs to disk You were going somewhere with this comment I trust? :) + // figure out if we're getting our How about killing PREF_AboutConfig() and pref_printDebugInfo() while your at it? Less to clean up later. And finally, could you kill the appropriate function declarations in prefapi.h too? With that. r=bnesse.

way ahead of you - I actually did kill thos functions in attachment 60189 [details] [diff] [review] :)

Comment on attachment 60189 [details] [diff] [review] switch prefs over to pldhash, v3 @@ -817,13 +820,16 @@ return NS_OK; } -PR_STATIC_CALLBACK(PRIntn) pref_enumChild(PLHashEntry *he, int i, void *arg) +PR_STATIC_CALLBACK(PLDHashOperator) + pref_enumChild(PLDHashTable *table, PLDHashEntryHdr *heh, + PRUint32 i, void *arg) { - EnumerateData *d = (EnumerateData *) arg; + PrefHashEntry *he = NS_STATIC_CAST(PrefHashEntry*,heh); Space after the comma please. @@ -515,7 +497,7 @@ /* Free up gSavedLine to avoid MLK. */ if (gSavedLine) free(gSavedLine); - gSavedLine = (char*)malloc(i+1); + gSavedLine = (char *)malloc(i+1); How about some whitespace around that "+"? @@ -1297,6 +1263,8 @@ switch (type & PREF_VALUETYPE_MASK) { case PREF_STRING: + // printf("\tSetting old string %s to new string %s\n", + // oldValue->stringVal, newValue.stringVal); Did Brendan already pick on you for this? @@ -1366,6 +1337,8 @@ pref_SetValue(&pref->defaultPref, value, type); if (!PREF_HAS_USER_VALUE(pref)) result = PREF_VALUECHANGED; + // if ((type & PREF_VALUETYPE_MASK) == PREF_STRING) + // printf("\tNow value is set to %s\n", pref->defaultPref); } And this? r=bnesse.

awesome, thanks guys. I've addressed everyone's comments here and landed the pldhash stuff. adding dveditz for sr= of nsautostring stuff.

here's an updated patch to reflect the patch that just landed.

Comment on attachment 60270 [details] [diff] [review] use nsCAutoStrings, v2 sr=dveditz

*sigh* - I caused a smoketest blocker, but this is a fix on top of attachment 60270 [details] [diff] [review] thanks to bnesse for noticing what was going on!

Comment on attachment 60362 [details] [diff] [review] only quote strings, not every type The second prefValue = '\"' (the one right after the str_escape call) should of course be += (I bugged alecf over irc and he said he fixed this in his tree). With that, sr=brendan@mozilla.org. /be

Comment on attachment 60362 [details] [diff] [review] only quote strings, not every type sigh. r=bnesse.

this patch is a MASSIVE cleanup to prefs - I went through with a moderately fine toothed comb to try to rip out all unused functions, and declare static the ones that only are used by prefapi.cpp This should make future work much easier. can I get an r=bnesse, sr=..? (Someone who wants to see prefs cleaned up further...:))

You did a fine job of breaking macpref.cpp (which uses the BinaryPref Stuff) and I get this neat error in prefapi.cpp... Error : inconsistent linkage: 'extern' object redeclared as 'static' prefapi.cpp line 204 static PRBool pref_VerifyLockFile(char* buf, long buflen);

ack! macpref.cpp! we should kill that. Why would it need the binary pref stuff? do you mind fixing macpref.cpp and attaching a patch to this bug? (thanks for the platform testing though!)

by the way, you can clean up that linkage error by just removing the pref_VerifyLockFile declaration from prefapi_private_data.h

It uses it in some path stuff. I'm not sure if those functions are used or not... I'll have to search a bit. I think the reason I haven't already killed macprefs is because they are...

well, as long as the library compiles without linking errors, then we know it's ok - the only exported function is the XPCOM NSGetFactory, which means nobody is (or better be) using PREF_GetPathPref() and all the other ones in macprefs.cp

Comment on attachment 60873 [details] [diff] [review] massive prefs cleanup It appears that, with your changes to prefapi.cpp, both macpref.cp and MacPrefUtils.cpp can be removed from the build. Please make sure you have emoved the the pref_VerifyLockFile declaration from prefapi_private_data.h. r=bnesse.

Comment on attachment 61307 [details] [diff] [review] Mac build changes cool. r=alecf

Comment on attachment 60873 [details] [diff] [review] massive prefs cleanup sr=sfraser

Comment on attachment 61307 [details] [diff] [review] Mac build changes sr=sfraser

thanks guys.. cleanup has been checked in

Ok, last patch and then I mark this fixed. The last place where prefs was malloc-happy was where we were PL_strdup'ing every pref name, even though all prefs names had the same lifetime, and were all allocated at the same time. So I made a little PrefNameBuffer class which allocates space for the strings in 8k chunks. All the pref names are allocated in this buffer. I chose 8k because the total bytes allocated to pref names in a basic run with about 750 prefs is 22k... with this patch, we allocate 3 8k buffers. bnesse and brendan, do you mind reviewing? Brendan, I'm especially interested to hear if you think I'm taking the right approach, and if so if I should try to align the allocated strings at all for speed in strcmp, etc (and if so, if there's an easy way to tell the "optimal" alignment) Also, I thought more about the 256 entry vs. 1024 entry issue. The one thing I did not take into account is that embedded apps should (eventually) have far fewer prefs - i.e. more in the 200-300 range... so maybe we should start with 256 entries? It looks like the tradeoff is more speed of mozilla-the-browser vs. memory-used-by-embedded-mozilla.

prefNameBuffer: I wish we convert pref names into an atom and store them only. Enumerate would be an issue ugh. I bet you will see no perf issue with 4k buffers. They are the page size. When instrumenting malloc cost, we find 4k buffer allocs to be ultra fast.

dp: they are stored as atoms, essentially.. its just that consumers don't use them... but that's ok because the only overhead cost is hashing - the only time we do any enumerate-and-compare loops is when calling pref-changed callbacks..

+// making PrefNameBuffer exactly 8k for nice allocation +#define PREFNAME_BUFFER_LEN (8196 - (sizeof(PrefNameBuffer*) + sizeof(char*))) Shouldn't that be 8192? +PrefNameBuffer::Alloc(PRInt32 len) +{ + NS_ASSERTION(this == gRoot, "Can only allocate on the root!\n"); + NS_ASSERTION(len < PREFNAME_BUFFER_LEN, "Can only allocate short strings\ n"); + // check for space in the current buffer + if ((mNextFree + len) > PREFNAME_BUFFER_LEN-1) { How about some white space for the "-" sign? +PrefNameBuffer::FreeBuffer() +{ + printf("\t***Freeing a PrefNameBuffer\n"); Hmm, that should be fun in a release build. ;) +PrefNameBuffer::CopyString(const char *str) +{ + if (!gRoot) gRoot = new PrefNameBuffer(nsnull); + + // ack, no memory + if (!gRoot) return nsnull; I'd like to see something more like: + if (!gRoot) { + gRoot = new PrefNameBuffer(nsnull); + // ack, no memory + if (!gRoot) + return nsnull; + } easier to breakpoint, and less compare/branch tests in the common case One concern I have with this is that it will bloat in the case where PREF_ClearAllUserPrefs() is called (when a "profile-do-change" message is received for example.) There will be a similar, albeit smaller, bloat if a pref or a pref branch is removed. As a side note to this... pref_ClearUserPref() returns PL_DHASH_REMOVE when it resets a user pref. Doesn't that cause the entry to be deleted? That will delete the default information (if there is any) too... And an unrelated note... I noticed that after the hashtable cleanup we now have the function pref_HashTableEnumerateEntries() which basically calls PL_DHashTableEnumerate()... not terribly efficient.

ok, I'll clean up whitespace (and good catch on 8196, I realized it after I attached a patch... really!) and I like that gRoot branch cleanup Now, I think we're ok as far as leaking/etc. We will bloat minorly because we never free up pref name strings until shutdown, but personally I think its worth possibly bloating a few k when the profile switches, than making lots of little allocations. .. eventually we might want to blow everything away and re-read the default prefs whenever a profile changes. oh, and I cleaned up pref_HashTableEnumerateEntries while I was there.

Comment on attachment 61597 [details] [diff] [review] use a real 8k buffer, with cleanups I don't think you want to check in that makefile change... I did some testing and, at least on the Mac, we never bloated, even during profile migration, because it does currently blow away everthing and reload. I really believe this is the right thing to do anyway... I don't see any real gain from trying to only clear the user prefs.

Er... that should have ended with r=bnesse.

brendan, can you take a look at the latest patch? its got r=bnesse, bug again I want to make sure I'm taking the right approach here.

Comment on attachment 61597 [details] [diff] [review] use a real 8k buffer, with cleanups >- PL_strfree((char*)pref->key); >+ // pref->key is cleared by the global PrefNameBuffer >+ pref->key = nsnull; > memset(entry, 0, table->entrySize); The comment confuses me -- does it mean that pref->key points to memory owned by the global PrefNameBuffer and freed all at once (at shutdown)? >+char* >+PrefNameBuffer::Alloc(PRInt32 len) >+{ >+ NS_ASSERTION(this == gRoot, "Can only allocate on the root!\n"); >+ NS_ASSERTION(len < PREFNAME_BUFFER_LEN, "Can only allocate short strings\n"); >+ // check for space in the current buffer >+ if ((mNextFree + len) > PREFNAME_BUFFER_LEN - 1) { >+ // allocate and update the root >+ gRoot = new PrefNameBuffer(this); >+ return gRoot->Alloc(len); >+ } >+ >+ // ok, we have space. >+ char *result = &mBuf[mNextFree]; >+ mNextFree += len; >+ >+ return result; >+} Sure, may as well align allocations. Any word-copy-optimized memcpy will have code (cf. Duff's Device) to unroll a byte-loop to align the copy, if necessary. Bypassing that code wins in cycles, at the cost of ~2 bytes per string, average (assuming uniformly distributed string lengths -- not sure that holds!). >+const char * >+PrefNameBuffer::StrDup(const char *str) >+{ >+ if (!gRoot) { >+ gRoot = new PrefNameBuffer(nsnull); >+ >+ // ack, no memory >+ if (!gRoot) >+ return nsnull; >+ } >+ >+ // extra byte for null-termination >+ PRInt32 len = PL_strlen(str) + 1; >+ >+ char *buf = gRoot->Alloc(len); >+ PL_strcpy(buf, str); Use memcpy, you have the length in bytes to pass, and it's fastest in general (inlined by gcc for constant len, possibly for short non-constant len). I do not want to overhype it, so I'll say that another reason to use it is to get with the standard C/C++ program to the extent that we can -- PL_strcpy is IIRC a wrapper added only to work around some HPUX bug in 1996! We should use strlen these days, I claim, unless you have the number of bytes precomputed, as this code does, in which case memcpy is indicated. >+ return buf; >+} >+ >+void >+PrefNameBuffer::FreeAllBuffers() >+{ >+ if (!gRoot) return; >+ gRoot->FreeBuffer(); How deep are we gonna recurse here and in similar cases? Worth unwinding the tail recursion (transforming to tail as needed)? >+ gRoot = nsnull; >+} Looks good otherwise (I must need to review other patches in this bug, still). What's the effect on startup time? /be

this patch addresses brendan's comments.. brendan/brian, care to review? (Brendan, all the other patches listed here are checked in, so I won't need your review on those) I wasn't sure of the best way to align the "next free" pointer, so I welcome any suggestions.

Comment on attachment 63405 [details] [diff] [review] same thing, and align on 4 byte boundaries +void +PrefNameBuffer::FreeBuffer() +{ + if (mNext) + mNext->FreeBuffer(); + delete this; +} Isnt tail recursion supposed to be better ? something like: PrefNameBuffer *next = mNext; delete this; next->FreeBuffer(); BTW, is delete this from a method ok ?

ooh... I didn't understand what brendan meant, then promptly forgot about it. now that I've seen it written out, I like it. I'll use dp's code.

Comment on attachment 63405 [details] [diff] [review] same thing, and align on 4 byte boundaries >+ PRInt32 mNextFree; Use PRUint32, or cast when using as %-with-power-of-two constant operand (see below). >+ // check for space in the current buffer >+ if ((mNextFree + len) > PREFNAME_BUFFER_LEN - 1) { Why - 1? If mNextFree + len <= PREFNAME_BUFFER_LEN, then len <= PREFNAME_BUFFER_LEN - mNextFree and the allocation fits in the current buffer. >+ // now align the next free allocation >+ if (mNextFree % PR_ALIGN_OF_WORD) >+ mNextFree += (PR_ALIGN_OF_WORD - (mNextFree % PR_ALIGN_OF_WORD)); Given a signed integral type for mNextFree, % requires a sign test if it is strength-reduced to &, as it should be here. So use PRUint32 for the member's type, or at least cast here. >+ >+ return result; >+} >+ >+void >+PrefNameBuffer::FreeBuffer() >+{ >+ if (mNext) >+ mNext->FreeBuffer(); >+ delete this; >+} dp, by "(transforming to tail as needed)" I meant what you did. But that was parenthetical to my main point, which was to manually eliminate recursion, replacing it with iteration. No C/C++ compiler known to me will do that for you. So: void PrefNameBuffer::FreeBuffer() { PrefNameBuffer* curr = this; PrefNameBuffer* next; do { next = curr->mNext; delete curr; } while ((curr = next) != nsnull); } >+ // extra byte for null-termination >+ PRInt32 len = PL_strlen(str) + 1; Ack! My sermon on the obsolescence of PL_str* didn't electrify you enough :-). Scour all PL_str*, I say. Just use strlen, etc. /be

cool, thanks for the further reviews. will attach a (hoepfully final) patch later

Gilding the lily here on alignment fu. >+ // now align the next free allocation >+ if (mNextFree % PR_ALIGN_OF_WORD) >+ mNextFree += (PR_ALIGN_OF_WORD - (mNextFree % PR_ALIGN_OF_WORD)); Besides being careful to avoid signed modulus overhead, you might explicitly eliminate the common subexpression (mNextFree % PR_ALIGN_OF_WORD) with a local variable. But see next point for a better way to avoid repetition or c.s.e. An if is required only if mNextFree has a pointer type and you don't want ugly casts. If the alignment modulus is a power of two (it had better be), and if you use an integral-type (word-sized) cursor instead of a pointer-type cursor, as you do here (PRUint32 in my suggested revision), then you can align without the if-test and without any signed-modulus hassles, via : mNextFree = (mNextFree + WORD_ALIGN_MASK) & ~WORD_ALIGN_MASK; where WORD_ALIGN_MASK is (PR_ALIGN_OF_WORD - 1). If you're worried about the return of 36-bits-per-word computers :-), then you can raise a portability flag with: #define WORD_ALIGN_MASK (PR_ALIGN_OF_WORD - 1) #if (PR_ALIGN_OF_WORD & WORD_ALIGN_MASK) != 0 # error "PR_ALIGN_OF_WORD is not a power of two!" #endif (Proof that (n & (n - 1)) == 0 iff n is a power of two is left to the reader.) So in summary: * use PRUint32 for mNextFree; * avoid %, use &~ as sketched above; * insist that the modulus is a power of two when deriving its mask, just in case something's fubared (not in case of 36-bit machine comebacks). /be

The diffs for nsPrefBranch.cpp, nsPrefService.cpp, and prefapi_private_data.h are missing... I assume they are still part of your patch? And, as long as you're cleaning up... +// making PrefNameBuffer exactly 8k for nice allocation +#define PREFNAME_BUFFER_LEN (8192 - (sizeof(PrefNameBuffer*) + sizeof(char*))) [snip] + // member variables + class PrefNameBuffer* mNext; + PRInt32 mNextFree; + char mBuf[PREFNAME_BUFFER_LEN]; Seems to me that the define should read... +#define PREFNAME_BUFFER_LEN (8192 - (sizeof(PrefNameBuffer*) + sizeof(PRInt32))) or ...sizeof(PRUint32) based on brendans comments.;)

excellent, thanks for the alignment help - I had finally figured out how to do a non-branching align just before you posted that :) Anyway, all commments addressed again. one more try, hopefully I got it right this time :)

Comment on attachment 63577 [details] [diff] [review] align on word boundary, take 2 Looks good, but now I'm wondering something that I should have asked earlier: why not just use a PLArenaPool? It has slightly higher memory overhead (because it has no constant PREFNAME_BUFFER_LEN analogue, it stores a limit cursor in each arena), but using it instead of writing this code should save code space (and development time :-/). /be

oh man, wish you had said that earlier.. :) I guess I still don't understand when it's better to use arenas vs. your own mem management. Oh well.. the code is pretty tiny for this use... I'll land as is and we can clean up if it looks like we need it.

Comment on attachment 63577 [details] [diff] [review] align on word boundary, take 2 >Index: prefapi.cpp >=================================================================== >RCS file: /cvsroot/mozilla/modules/libpref/src/prefapi.cpp,v >retrieving revision 3.112 >diff -u -r3.112 prefapi.cpp >--- prefapi.cpp 21 Dec 2001 22:29:25 -0000 3.112 >+++ prefapi.cpp 4 Jan 2002 22:10:37 -0000 >@@ -176,8 +178,97 @@ > nsnull, > }; > >+class PrefNameBuffer; > >-/*----------------------------------------------------------------------------------------*/ >+// making PrefNameBuffer exactly 8k for nice allocation >+#define PREFNAME_BUFFER_LEN (8192 - (sizeof(PrefNameBuffer*) + sizeof(char*))) >+#define WORD_ALIGN_MASK (PR_ALIGN_OF_WORD - 1) >+ >+// sanity checking >+#if (PR_ALIGN_OF_WORD & WORD_ALIGN_MASK) != 0 >+#error "PR_ALIGN_OF_WORD must be a power of 2!" >+#endif Chris Seawood just reported that the use of PR_ALIGN_OF_WORD breaks the Seamonkey-Ports Tinderboxes because this macro is only defined on Mac, Win32, and Linux (bug 118286). PR_ALIGN_OF_WORD is an undocumented macro that we keep around for backward compatibility. It should not be used in new code. I have two suggested fixes. 1. Be conservative and align on 8-byte boundaries. That is, simply define WORD_ALIGN_MASK as 0x7. 2. Replace PR_ALIGN_OF_WORD by sizeof(void *). In either case, the sanity checking will need to be deleted. In fact, since you only use PrefNameBuffer::Alloc to allocate character strings, I don't understand why you need to align at all.

OK, I saw that you want to align the character strings to improve the performance of strcmp. :-)

The patch busted darwin... prefapi.cpp c++ -o prefapi.o -c -DOSTYPE=\"Darwin5.2\" -DOSARCH=\"Darwin\" -DOJI -DB_ONE_M -I../../../dist/include/xpcom -I../../../dist/include/string -I../../../ dist/include/js -I../../../dist/include/xpconnect -I../../../dist/include/caps -I../../../ dist/include/intl -I../../../dist/include/necko -I../../../dist/include/pref -I../../../dist/ include -I/Users/josh/Code/Mozilla_Project/mozilla/dist/include/nspr - fPIC -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wbad- function-cast -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor- privacy -Wno-long-long -fpascal-strings -traditional-cpp -fno-common -I/ Developer/Headers/FlatCarbon -F/System/Library/Frameworks -pipe - DNDEBUG -DTRIMMED -O -DMOZILLA_CLIENT -include ../../../config- defs.h -Wp,-MD,.deps/prefapi.pp prefapi.cpp prefapi.cpp: In method `char * PrefNameBuffer::Alloc(int)': prefapi.cpp:231: `PR_ALIGN_OF_WORD' undeclared (first use this function) prefapi.cpp:231: (Each undeclared identifier is reported only once prefapi.cpp:231: for each function it appears in.) nsPrefBranch.cpp: In method `nsresult nsPrefBranch::GetChildList(const char *, PRUint32 *, char ***)': nsPrefBranch.cpp:525: warning: unused variable `nsresult rv' nsPrefBranch.cpp: In method `nsresult nsPrefBranch::AddObserver(const char *, nsIObserver *, int)': nsPrefBranch.cpp:584: warning: unused variable `nsresult rv' nsPrefBranch.cpp: In method `nsresult nsPrefBranch::RemoveObserver(const char *, nsIObserver *)': nsPrefBranch.cpp:625: warning: `struct PrefCallbackData * pCallback' might be used uninitialized in this function ../../../dist/include/string/nsBufferHandle.h: At top level: prefapi.cpp:165: warning: `char * gLockFileName' defined but not used prefapi.cpp:166: warning: `char * gLockVendor' defined but not used prefapi.cpp:305: warning: `PRBool pref_VerifyLockFile(char *, long int)' defined but not used prefapi.cpp:1254: warning: `enum PrefResult PREF_CreateChildList(const char *, char **)' defined but not used make[4]: *** [prefapi.o] Error 1 make[4]: *** Waiting for unfinished jobs.... make[4]: Leaving directory `/Users/josh/Code/Mozilla_Project/mozilla/ modules/libpref/src' make[3]: *** [libs] Error 2 make[3]: Leaving directory `/Users/josh/Code/Mozilla_Project/mozilla/ modules/libpref' make[2]: *** [tier_9] Error 2 make[2]: Leaving directory `/Users/josh/Code/Mozilla_Project/mozilla' make[1]: *** [default] Error 2 make[1]: Leaving directory `/Users/josh/Code/Mozilla_Project/mozilla' make: *** [build] Error 2 make: Leaving directory `/Users/josh/Code/Mozilla_Project/mozilla'

I had already checked in that patch... I fixed the bustage by defining my own ALIGN_OF_WORD as sizeof(void*) on platforms where it isn't already defined. That completes this bug. For further malloc issues, file new bugs.

Alec Flett wrote: > I fixed the bustage by defining my own ALIGN_OF_WORD > as sizeof(void*) on platforms where it isn't already defined. I reiterate my recommendation that PR_ALIGN_OF_WORD should not be used in new code. You should just use sizeof(void*) on all platforms.