+++ This bug was initially created as a clone of Bug #400902 +++ Currently the maximum size of the GC free list for doubles is limited to 8 entries. This limit comes from the fact that currently code treats the double allocation bitmap as a sequence of bytes and delegate the constructing of the free list to the code outside the lock when a byte with unset buts is detected. Since there indications that lockiking is rather costly, it would be interesting to known how treating the bitmap as a sequence of uint16 affects the performance. This should on average decrease the amount of locks by 2.

Something that passes the test cases. Time to try test cases with less fear about "speedups" from buggy code.

(In reply to comment #1) > Something that passes the test cases. Time to try test cases with less fear > about "speedups" from buggy code. I meant time to "try benchmarks"

This shows 1% total gain over unpatched build. To decrease the noise from crash alignment changes I compiled the shell for all runs using BUILD_OPT=1 INTERP_XCFLAGS="-falign-functions=4096" One can still see the noise from alignment changes from regexp benchmarks, but at least the bit manipulations benchmark that never leaves the interpreter shows 0 changes.

here the results with the same setup as in the previous comments but which increases the limit to 32. The total gain now is 1.7% compared with 1% with the limit set to 16. Based on this I think the right way to deal with this is just use jsuword as a the base type for double allocation type and simplify the source complexity using the bit manipulation macros.

Asking for 1.9 blocking due to potentially noticeable performance gains.

The new patch uses jsbitmap as the basic type for double occupation bitmap. I will test it/benchmark it before asking for a review.

Does this impact memory use?

Just some nits, in case you do end up liking that patch: you mix "uword" and "jsuword" in your comments. - for (lastcell = cell + JS_BITS_PER_BYTE - 1; cell != lastcell; ++cell) + for (lastcell = cell + JS_BITS_PER_WORD - 1; + cell != lastcell; + ++cell) { cell->link = cell + 1; + } You probably wrapped this before ending back up with the same line length. It's much easier to read in the old style. Can we add JS_BITMAPMASK or JS_BITMASK64 for readability?

(In reply to comment #7) > Does this impact memory use? There is no extra memory overhead with mostly single-threaded embedding like firefox. The unit of double allocation is 4K and the patch just changes how things are organized inside that unit. With heavily multitasking one can observe the overhead when different thraeds are under utilized the free lists effectively wasting the space. But doubles have that property that when a script starts to allocate them, then it allocates a lot of them. So even in that case I suspect the overhead should be low.

(In reply to comment #8) > Can we add JS_BITMAPMASK or JS_BITMASK64 for readability? I am not sure about this. The patch have one place with (jsbitmap) 1 << shift and one place with ((jsbitmap) 1 << shift) - 1.

(In reply to comment #8) > Just some nits, in case you do end up liking that patch: > > you mix "uword" and "jsuword" in your comments. Could you point where it happens?

The new version addresses the nits from the comment 8.

(In reply to comment #8) jag: was blocking1.9 flag change from + into "?" was intentional?

Comment on attachment 309003 [details] [diff] [review] v2b Looks good to me, jag is the man who reviewed the basis patch. BTW, looks like this depends on the patch for bug 421274. Ok by me, as you should have that landed before this one. /be

Here is a version of the patch without changes from the bug 421274.

No, the change from blocking + to ? was accidental. Dunno how that happened. No warning on my end.

Comment on attachment 309042 [details] [diff] [review] v2c >Index: js/src/jsgc.c >=================================================================== >+ * Check that ARENA_INFO_OFFSET and sizeof(jsdouble) divides sizeof(jsuword). Nit: extra space >@@ -1968,75 +1983,75 @@ RefillDoubleFreeList(JSContext *cx) > doubleFlags = DOUBLE_ARENA_BITMAP(a); > break; > } > doubleFlags = > DOUBLE_ARENA_BITMAP(((JSGCArenaInfo *) doubleFlags)->prev); > } > > /* >- * When doubleFlags points the last bitmap's byte in the arena, its >+ * When doubleFlags points the last bitmap's word in the arena, its Nit: "points to" > * high bits corresponds to non-existing cells. ClearDoubleArenaFlags >+ * sets such bits to 1. Thus even for this last word its bit is unset >+ * iff the corresponding cell exists and free. I think I like the old wording better (note though s/is/are/ etc.): "Thus even for this last word its bits are unset only when the corresponding cells exist and are free." > do { > --bit; > --cell; >- if (!(JS_BIT(bit) & usedBits)) { >+ if (!(((jsbitmap) 1 << bit) & usedBits)) { I should've mentioned it last time around but I find this easier to read if you visually make |usedBits| the pattern to test and |((jsbitmap) 1 << bit)| the mask with which you iterate over the pattern, by swapping their order like this: >+ if (!(usedBits & ((jsbitmap) 1 << bit))) { I think JS_BIT64(bit) or JS_BIT_L(bit) or something would be easier on the eyes: >+ if (!(usedBits & JS_BIT64(bit))) { But it's not a requirement for r+ of this patch.

The new version addresses the nist from the comment 17: --- /home/igor/m/ff/mozilla/quilt.bM2955/js/src/jsgc.c 2008-03-13 16:16:43.000000000 +0100 +++ js/src/jsgc.c 2008-03-13 16:10:16.000000000 +0100 @@ -489,5 +489,3 @@ JS_STATIC_ASSERT(1 <= js_gcArenasPerChun -/* - * Check that ARENA_INFO_OFFSET and sizeof(jsdouble) divides sizeof(jsuword). - */ +/* ARENA_INFO_OFFSET and sizeof(jsdouble) must divide sizeof(jsuword). */ JS_STATIC_ASSERT(ARENA_INFO_OFFSET % sizeof(jsuword) == 0); @@ -1990,6 +1988,6 @@ RefillDoubleFreeList(JSContext *cx) /* - * When doubleFlags points the last bitmap's word in the arena, its + * When doubleFlags points to the last bitmap's word in the arena, its * high bits corresponds to non-existing cells. ClearDoubleArenaFlags - * sets such bits to 1. Thus even for this last word its bit is unset - * iff the corresponding cell exists and free. + * sets such bits to 1. Thus even for this last word its bits are + * unset only when the corresponding cells exist and free. */ @@ -2039,3 +2037,3 @@ RefillDoubleFreeList(JSContext *cx) --cell; - if (!(((jsbitmap) 1 << bit) & usedBits)) { + if (!(usedBits & ((jsbitmap) 1 << bit))) { JS_ASSERT(index + bit < DOUBLES_PER_ARENA);

I checked in the patch from the comment 18 to the trunk: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205438722&maxdate=1205438823&who=igor%25mir2.org