415967 - Use malloc_size() to increase arena sizes

Status: RESOLVED INVALID

(Core :: JavaScript Engine, defect, P2)

Description

[Robin Bate Boerop]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.8.1.12pre) Gecko/20080118 Camino/1.6b2 (like Firefox/2.0.0.12pre)
Build Identifier: 

malloc_size() will tell how much memory an allocation really uses.  When allocating arenas in jsarena.*, we use malloc().  These allocations are buffers, and bigger is better.  So, we should use malloc_size() to get the real size of the allocations, and then use all of it.  This is a way to reduce the allocation overhead (aka internal fragmentation).

This is a memory use optimization, not a logic error bug.

Reproducible: Always

Comment 1

[Robin Bate Boerop]

I have been writing code which measures memory use by JSArena code.  Some of my measurements show the value of using malloc_size() and malloc_good_size() for allocating JSArenas.

'MemStats' performs analysis of a log of a run of Firefox.  Here is some of what it says after just starting and then stopping the browser:

MemStats by Robin Bate Boerop.
Allocations summary:
  requested (malloc) total: 0
  real total: 0
  total requests: 6486
  total reallocations: 5
  total outstanding allocations: 0
  messages: []
Most outstanding allocations: 86
High water mark for requested memory: 332818
High water mark for real memory: 374560
Overhead (internal fragmentation) on high water mark: 41742 (11.1%)

This last number, the overhead, shows overhead at the 'high water mark' (HWM).  The HWM is the point at which the run consumed the most memory (by the allocations under scrutiny - the JSArena code).

What is it?  Well, each time malloc() allocates some memory for a caller, it may actually allocate more memory than is asked for.  It rounds up to some quantum size, typically, or rounds up to the nearest page size, in the case of large allocations.  I call this the 'real' memory use.  Its number is shown above.  The difference between the memory size that the caller asked for (the requested size) and the real size is the overhead.  That is,
   overhead = real - requested.

As mentioned in #c0, the arena allocations can be made bigger.  See my patch for how.  After using malloc_good_size() in the patch, we get a different output from MemStats:

MemStats by Robin Bate Boerop.
Allocations summary:
  requested (malloc) total: 0
  real total: 0
  total requests: 5797
  total reallocations: 5
  total outstanding allocations: 0
  messages: []
Most outstanding allocations: 66
High water mark for requested memory: 315168
High water mark for real memory: 315168
Overhead (internal fragmentation) on high water mark: 0 (0.0%)

The overhead has gone to 0.  The maximum number of outstanding allocations has dropped from 86 to 66.  The real memory used has dropped, also!

This is a significant improvement.  So, I also ran the MemStats program after a SunSpider run.  Here is the result before my patch:

MemStats by Robin Bate Boerop.
Allocations summary:
  requested (malloc) total: 0
  real total: 0
  total requests: 772441
  total reallocations: 302
  total outstanding allocations: 0
  messages: []
Most outstanding allocations: 808
High water mark for requested memory: 4576632
High water mark for real memory: 4609312
Overhead (internal fragmentation) on high water mark: 32680 (0.7%)

Here is the result after my patch:

MemStats by Robin Bate Boerop.
Allocations summary:
  requested (malloc) total: 0
  real total: 0
  total requests: 749952
  total reallocations: 262
  total outstanding allocations: 0
  messages: []
Most outstanding allocations: 545
High water mark for requested memory: 4544311
High water mark for real memory: 4548384
Overhead (internal fragmentation) on high water mark: 4073 (0.1%)

The % overhead is negligible on SunSpider, but maximum number of outstanding allocations at the HWM has dropped from 808 to 545.  The HWM for real memory use has dropped by 60K (just a little).  The number of calls to malloc has dropped by 3%.

Overall, the results are only positive.

I'll post the patch as soon as I figure out how.  It's not final.

Comment 2

[Robin Bate Boerop]

Attachment: draft1

Don't try to apply this patch.  It is for human reading only.

Comment 3

[Brian Crowder]

Comment on attachment 304676 [details] [diff] [review]
draft1

Nice results.  This use of const is (unfortunately) bogus in spidermonkey code, I think, as it isn't portable to ancient C compilers.  Someone else can correct me if I'm wrong.  You also cannot declare extra, hdrsz, and gross inline (again for portability reasons), they must be declared at the top of their containing block, if not earlier.  Spidermonkey style tends to prefer declarations at the top of the routine, in order of usage (essentially back where they were).

+            JS_ASSERT( sizeof(gross) == sizeof(size_t) );
This can and should be a static assertion, instead.  You should promote anything that can be calculated at compile-time to be a static assertion.  More in a bit...

Comment 4

[Brian Crowder]

Comment on attachment 304676 [details] [diff] [review]
draft1

+            JS_ASSERT( sizeof(gross) == sizeof(size_t) );
Spidermonkey style prefers this spacing style, for function and macro arguments:
+            JS_ASSERT(sizeof(gross) == sizeof(size_t));

It might be nice to do something like brendan's arenasize patch from bug 408921, but instead calculate a "good" arenasize there, rather than doing it here in each JS_ArenaAllocate.  Obviously, oversized allocations foil you; so perhaps this is better overall.

I do prefer this upsizing to the downsizing used in that patch, though.

+                if (gross > *pool->quotap)  /* use 'gross' for now, until we  */
+                                            /* decide to "bill" for goodGross */
Do this comment as a one-liner, complete sentence (caps and punctuation) before the line(s) it documents.

Comment 5

[Brian Crowder]

This patch would obsolete the patch from bug 408921; not sure if I've got the dependency/blocking relationship right, but the link should be noted.  We should get either this or that bug in for P4.  I like this fix better as it does not mandate touching all clients of JSArena code (though we may still want to consider it).

Robin:  If it is possible to calculate a good-size earlier (ie, in the arena init), I think there are wins to be had there, since we'd be less likely to find ourselves creating oversized allocations, and might end up with arenas with room leftover for usage by smaller-chunking neighbors.

Comment 6

[Brian Crowder]

Robin:  You or I should also open and track a bug to do this precise change for PLArenas, also targetted for this same milestone.

Comment 7

[Brian Crowder]

(In reply to comment #5)
> should get either this or that bug in for P4.

Of course, I meant "b4" here.

Comment 8

[Robin Bate Boerop]

Attachment: First serious attempt at a patch for this bug.

Uses malloc_usable_size() if the CPP macro HAVE_MALLOC_USABLE_SIZE is defined.  If it's not, then it uses malloc_size() if the CPP macro HAVE_MALLOC_SIZE is defined.

The jemalloc code should provide a malloc_usable_size() on all platforms.  malloc_usable_size() is available on Linux in any case.  malloc_size() is available on Mac OS X.

Comment 9

[Mike Schroepfer]

Robin is this ready for review or are you still working on the patch?

Comment 10

[Brian Crowder]

As I discussed with Robin earlier, this needs a follow-up that either discerns the available malloc queries in config/*.mk (for JS-shell in Makefile.ref) or inherits them from the global makefile; there is no way to guess on Win32, we must be told by the Firefox build system that we have jemalloc malloc querying abilities.

cc:ing Ted for thoughts on the build features.

Robin:  in the "details" for your patch, set me for review, and set Igor for additional review, if you're ready to get feedback on this patch.

Comment 11

[Brian Crowder]

Comment on attachment 305092 [details] [diff] [review]
First serious attempt at a patch for this bug.

+    /* The jemalloc code will always provide malloc_usable_size().
+     */
Prevailing style would make this a one-line comment.

+    #define MALLOC_USABLE_SIZE(p) malloc_usable_size(p)
Prevailing style indents macros thusly:
+# define MALLOC_USABLE_SIZE(p) malloc_usable_size(p)
(only one space for macro indentation, and it occurs after the #)

+/* Allocates the requested amount of memory, or more.  The amount actually
+ * available is pointed to by pRealSize.
+ */

Multiple lines like this:
+/*
+ * Allocates the requested amount of memory, or more.  The amount actually
+ * available is pointed to by pRealSize.
+ */

+void * flexibleMalloc( size_t requestedSize, size_t * pRealSize )
Should be:

+static void *
+flexibleMalloc(size_t requestedSize, size_t *pRealSize)
(multiple spacing changes), also made this static to permit possible inlining.  Also, match the StudlyCaps style used everywhere for static functions (cap first letter of each word, including the first)

+    void * returnPtr = malloc( requestedSize );
No extra space after * or padding for () in function calls

+    } else {
+        *pRealSize = 0;
+    }
Not necessary, returning NULL here is an OOM-case and you'll never care about the size of the NULL data if the rest of the OOM handling is right.

+    jsuword extra, hdrsz, gross, finalSize;
Maybe just "final" to agree with prevailing naming style.

+            JS_STATIC_ASSERT( sizeof( size_t ) == sizeof( finalSize ) );
Again, no extra padding around (), should be:
+            JS_STATIC_ASSERT(sizeof(size_t) == sizeof(finalSize));
(I'll spare you from any more complaining about this, please fix everywhere)
Also, you should move these static assertions outside of the function, and provide a comment as to why they are critical.

+            b->limit = (jsuword)b + finalSize;
While you're here, fix this pre-existing cast to be:
+            b->limit = (jsuword) b + finalSize;


We need a flexible "realloc" in JS_ArenaGrow, no?

Did you see my feedback from before?  I still wonder if we don't want this logic or something like it to happen when arenasizes are originally set.  Not sure if it's possible without doing a malloc there, though, which probably wouldn't be worth it.

Have you run this patch with the js/tests suite and MochiTests yet?

Comment 12

[(not currently active) Ted Mielczarek]

configure defines MOZ_MEMORY both as a CPP macro and as a makefile variable when we're building with jemalloc.  In addition, there's a define per-platform:
http://lxr.mozilla.org/mozilla/source/configure.in#6066

Comment 13

[Robin Bate Boerop]

Attachment: Second patch, implements Brian's suggested changes

Another patch is on the way; it will handle the realloc case.  Note however, that my measurements and testing showed that reallocations account for only a very small portion of the memory consumed.  So, the realloc case is only for completeness.  This patch should show as much of a performance improvement as we can expect in this bug.

Brian, this patch tries to address all of your stylistic concerns.  Thanks for teaching me the style of mozilla/js!

I will mark the next patch for review.

Comment 14

[Robin Bate Boerop]

(In reply to comment #4)
> It might be nice to do something like brendan's arenasize patch from bug
> 408921, but instead calculate a "good" arenasize there, rather than doing it
> here in each JS_ArenaAllocate.  Obviously, oversized allocations foil you; so
> perhaps this is better overall.
> 
> I do prefer this upsizing to the downsizing used in that patch, though.

The upsizing effected by malloc_usable_size() is orthogonal to the downsizing done in the patches for 408921, as strange as that may sound.  In other words, we should do both.  This bug arranges for us to waste less space, the other bug arranges for us to use an amount of space closer to what we thought we were using.  (By "what we thought we were using", I mean that the arenas are brought closer to the size given by the creator of the arena pool.)

It is difficult (inefficient) to calculate the "good" arenasize earlier, because the premise of this bug's patch is to use the malloc_usable_size function.  It takes a pointer to an already-allocated memory region.  So, we can't easily know the size before actually having allocated the memory.

We could allocate the memory, obtain the "good" size, and then free the memory, but that is inefficient.  It would not lead to beautiful code.  Alternately, we could call malloc_good_size(), my original idea, but, in fact, that does the same hack under the covers.

I think that calling malloc_usable_size very "close" to the actual allocation will also lead to code which is less fragile.  In addition, the oversize allocations would screw that up, as you mention, Brendan.  So, I agree with you that this is the right way to use malloc_usable_size() in the JSArena code.

Comment 15

[Robin Bate Boerop]

(In reply to comment #6)
> Robin:  You or I should also open and track a bug to do this precise change for
> PLArenas, also targetted for this same milestone.

Done.  It is bug 419131.  The PLArenas are in NSPR.  My understanding of that library is that it does not follow the same release schedule as the rest, and that the current mozilla source tree uses a fixed CVS snapshot of libnspr, and not the most current one.  So, if I make the changes, they may not make it into beta4.  Perhaps someone can verify?

Comment 16

[Robin Bate Boerop]

(In reply to comment #9)
> Robin is this ready for review or are you still working on the patch?

The most recent patch contains stuff which should be reviewed and considered, but it is not my final version.  More to come, today.  I will mark my next patch for review by Brian Crowder, at his request.

Comment 17

[Robin Bate Boerop]

(In reply to comment #5)
> Robin:  If it is possible to calculate a good-size earlier (ie, in the arena
> init), I think there are wins to be had there, since we'd be less likely to
> find ourselves creating oversized allocations, and might end up with arenas
> with room leftover for usage by smaller-chunking neighbors.

As I mentioned in another reply, it is better to calculate the "good" size closer to the malloc call.  There is still reason to fix bug 408921, however.  I think that your other concerns are best discussed in the context of that bug.  I'll take a look at that after I finish the patch for this bug.

Comment 18

[Brian Crowder]

Comment on attachment 305203 [details] [diff] [review]
Second patch, implements Brian's suggested changes

+#else
+ /* Do not define MALLOC_USABLE_SIZE. */
+#endif
Don't need the empty else-case.

+    if (returnPtr) {
+        /* Calling malloc_usable_size(0) is illegal. */

The comment isn't necessary; also, reword this as:

if (!returnPtr)
    return NULL;

Your ifdefs here should line up against column 0 always, and the code should be indented as it would normally be within the block.

+            JS_STATIC_ASSERT(sizeof(size_t) == sizeof(gross));
+            JS_STATIC_ASSERT(sizeof(size_t) == sizeof(final));
Hoist these out of the function, as I suggested in last review.

+            JS_STATIC_ASSERT(sizeof(jsuword) == sizeof(b));
This one, too.

File a separate bug on the use of jsuword here, rather than size_t or ptrdiff_t, as appropriate.

No realloc love still?  Maybe better as another bug, I'd hate to hold this one up for it.

Status on js/tests?  Mochitests?  Have you thrown this patch at the TryServer?

Comment 19

[Robin Bate Boerop]

Brian, thanks for your comments.  Some of them are style-related.  I hope that you can point me to a js-component style document that I can read, to avoid future style conflicts.  I would like to learn the reasons behind the style choices.  That is important, because most of the choices I've seen made in js are exactly the opposite from those made in other contexts.  :)  Further, some of the style choices have runtime consequences.

(In reply to comment #18)
> (From update of attachment 305203 [details] [diff] [review])
> +#else
> + /* Do not define MALLOC_USABLE_SIZE. */
> +#endif
> Don't need the empty else-case.

Agreed.  Fixed.

> +    if (returnPtr) {
> +        /* Calling malloc_usable_size(0) is illegal. */
> 
> The comment isn't necessary; also, reword this as:

The comment documents a poorly-documented (or undocumented) function.  One would think that malloc_usable_size(0) is quite legal (and would return 0).  If that were true, then the if-block surrounding the comment would be unnecessary and a poor coding choice.  I'm trying to future-proof the code against someone making exactly that change.  I'm also adding documentation about a poorly documented function.  So, I would like to keep the comment.

> if (!returnPtr)
>     return NULL;

This rewording introduces code path, and does not take advantage of the fact that the value "NULL" is already available in the 'returnPtr' variable.  However, it does match other code in js.  Is the style-matching the reason for the change?

> Your ifdefs here should line up against column 0 always, and the code should be
> indented as it would normally be within the block.

Though this is a common coding style, moving the preprocessor directives leftward reduces readability.  Are you suggesting this to help keep a common style among all js code, or because some ancient preprocessors don't handle leading whitespace properly?

> +            JS_STATIC_ASSERT(sizeof(size_t) == sizeof(gross));
> +            JS_STATIC_ASSERT(sizeof(size_t) == sizeof(final));
> Hoist these out of the function, as I suggested in last review.
> 
> +            JS_STATIC_ASSERT(sizeof(jsuword) == sizeof(b));
> This one, too.

Hoisted.

> No realloc love still?  Maybe better as another bug, I'd hate to hold this one
> up for it.

Yeah.  Realloc is harder because it seems that callers think they know how big the allocation is.  As of now, they don't, because FlexibleMalloc makes it bigger.  So, the logic changes are more tricky.

> Status on js/tests?  Mochitests?  Have you thrown this patch at the TryServer?

Mochitests are under way.  No to the other two questions.  I need help with the TryServer, as I am a TryServer virgin.  :-[

Comment 20

[Brian Crowder]

(In reply to comment #19)
> Brian, thanks for your comments.  Some of them are style-related.  I hope that
> you can point me to a js-component style document that I can read, to avoid
> future style conflicts.

http://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style <--- this is an old doc and doesn't really describe even a fraction of the style nuances which are enforced in Spidermonkey, unfortunately.


> I would like to learn the reasons behind the style
> choices.  That is important, because most of the choices I've seen made in js
> are exactly the opposite from those made in other contexts.  :)

Other contexts within spidermonkey?


> Further, some of the style choices have runtime consequences.

Which ones?


> > +    if (returnPtr) {
> > +        /* Calling malloc_usable_size(0) is illegal. */
> > 
> > The comment isn't necessary; also, reword this as:
> 
> The comment documents a poorly-documented (or undocumented) function.  One
> would think that malloc_usable_size(0) is quite legal (and would return 0).  If
> that were true, then the if-block surrounding the comment would be unnecessary
> and a poor coding choice.  I'm trying to future-proof the code against someone
> making exactly that change.  I'm also adding documentation about a poorly
> documented function.  So, I would like to keep the comment.

Anyone else modifying this code, in the absence of the comment would surely notice the NULL check preceding the function-calls, but I'm not hugely opposed to the comment.

> > if (!returnPtr)
> >     return NULL;
> 
> This rewording introduces code path, and does not take advantage of the fact
> that the value "NULL" is already available in the 'returnPtr' variable. 
> However, it does match other code in js.  Is the style-matching the reason for
> the change?

Yes, style matching and the observation that the more conditional nesting exhibited by code, the less maintainable it is.  return early and often.  There is no new code path introduced here that does not already exist, the only difference is that the OOM case is handled immediately and is not a concern for any following code.

> > Your ifdefs here should line up against column 0 always, and the code should be
> > indented as it would normally be within the block.
> 
> Though this is a common coding style, moving the preprocessor directives
> leftward reduces readability.  Are you suggesting this to help keep a common
> style among all js code, or because some ancient preprocessors don't handle
> leading whitespace properly?

This readability case is a matter of opinion, so please follow the house style.

> > Status on js/tests?  Mochitests?  Have you thrown this patch at the TryServer?
> 
> Mochitests are under way.  No to the other two questions.  I need help with the
> TryServer, as I am a TryServer virgin.  :-[

http://wiki.mozilla.org/Build:TryServer will show you the ropes for the TryServer.  It's super easy.

Comment 21

[Robin Bate Boerop]

(In reply to comment #20)
> http://wiki.mozilla.org/Build:TryServer will show you the ropes for the
> TryServer.  It's super easy.

I have neither an LDAP account nor a CVS account.  My understand is that I'm not allowed to have an LDAP account.  So, I haven't used the TryServer.

MochiTest gave the same quantity of errors with my change as without (497).  I didn't run it with -chrome.  Not sure if that matters.

Comment 22

[Robin Bate Boerop]

Attachment: Reviewable patch, no realloc changes

The realloc changes are left out of this patch.  I will continue to test them.  I'll continue to work on another patch, and on executing js/tests for this patch.

Comment 23

[Robin Bate Boerop]

My patch (attachment 305340 [details] [diff] [review]) reproducibly gives 4 errors from js/tests that an unmodified build does not.  (171 errors vs. 167 errors).  I ran

perl -I$HOME/.cpan/build/Getopt-Mixed-1.10/lib/ jsDriver.pl -e smdebug -L slow-n.tests -L spidermonkey-n.tests

The new failures are:

e4x/Regress/regress-394941.js
js1_5/Array/regress-108440.js
js1_5/extensions/regress-44009.js
js1_5/Regress/regress-290575.js

I am investigating.  However, I have no prior experience with js/tests, and certainly not with these 4 tests in particular.  So, the investigation may take a while.  If anyone has any ideas, I'd appreciate hearing them.

Comment 24

[Robin Bate Boerop]

None of the new failures are false alarms.

The first of the 4 is an out-of-memory error.  The patch may have changed the memory consumption behaviour enough to produce this.  Will investigate later.

The last 3 of the 4 are assertion failures in the realloc code, for which I will have to produce a patch.  I will handle this, first.

Comment 25

[Igor Bukanov]

Comment on attachment 305340 [details] [diff] [review]
Reviewable patch, no realloc changes


> #include "jsutil.h" /* Added by JSIFY */
> 
>+#if defined(MOZ_MEMORY)
>+  size_t malloc_usable_size(void *);
>+# define HAVE_MALLOC_USABLE_SIZE
>+#elif defined(XP_UNIX) && !defined(XP_MACOSX)
>+# include <malloc.h>  /* for malloc_usable_size */
>+# define HAVE_MALLOC_USABLE_SIZE
>+#elif defined(XP_MACOSX)
>+# include <malloc/malloc.h>  /* for malloc_size */
>+# define HAVE_MALLOC_SIZE
>+#endif

Check for MOZ_MEMORY after all other options are considered. It avoids redeclaring malloc_usable_size from a system header.

>-                b = (JSArena *) malloc(gross);
>+                b = (JSArena *) FlexibleMalloc(gross, &final);
>                 if (!b)
>                     return NULL;
>+                /*
>+                 * Let's only "bill" the quota for the gross.  The additional
>+                 * (final - gross) bytes are free.  TODO: Reconsider this.
>+                 */

It is better to do the accounting properly and account for all all allocated size.

Comment 26

[Robin Bate Boerop]

(In reply to comment #25)
> Check for MOZ_MEMORY after all other options are considered. It avoids
> redeclaring malloc_usable_size from a system header.

Done in my immediately upcoming patch.

> >+                /*
> >+                 * Let's only "bill" the quota for the gross.  The additional
> >+                 * (final - gross) bytes are free.  TODO: Reconsider this.
> >+                 */
> 
> It is better to do the accounting properly and account for all all allocated
> size.

Yes.  I have opened bug 419399 for this.

For now, we can say that even though we are "going over the quota" by not accounting for the extra memory consumed, we were actually already doing that, except that the memory was previously being wasted (as overhead from malloc) instead of used for the arena.

Comment 27

[Robin Bate Boerop]

Attachment: Final patch, I hope

This patch will be unpleasant to review.  :)

Understanding the changes requires knowledge of the intricacies of the jsarena code.  To help with understanding, I have added many useful assertions to the code, as well as some key comments.

The essence of the patch is to disallow arenas larger than the pool->arenasize to use the extra memory from malloc_usable_size.  This is necessary in order to maintain a current invariant in the jsarena code, which is that allocations larger than the arenasize each get their own dedicated arena.  So, the patch takes special care not to let allocations larger than the arenasize sneak into an arena which was "enlarged" by malloc_usable_size.

This patch fixes the errors encountered in the prior patch.

I have compiled this only on a Mac.

I tested with
   jsDriver -e smdebug -L slow-n.tests -L spidermonkey-n.tests
which ran 2443 tests and produced the same 167 errors as on an unmodified build.

Comment 28

[Brian Crowder]

Comment on attachment 305454 [details] [diff] [review]
Final patch, I hope


First:  No tabs!  Kill them all!


+ * Allocates the requested amount of memory, or more.  The amount actually
Should be "Allocate"?


+                 * (real - gross) bytes are free.  TODO: Reconsider this.
Instead of "TODO:" use "FIXME: bugNNNNNN" instead.


if (real > arenaSize) {
    JS_ASSERT(gross <= JS_UPTRDIFF(a->limit, a));
    /*
     * Don't create arenas larger than the gross size when the
     * arena is an oversized (dedicated) allocation.  Oversized
     * allocations are always one-allocation-per-arena, and we
     * don't want other allocations sneaking in there.
     */
    a->limit = (jsuword) a + gross;
} else {
    a->limit = (jsuword) a + real;
}
Would be better if you could clean this up as a ternary expression and reword the JS_ASSERT().


+        JS_ASSERT(a->base == (jsuword) p);
     } else {
         ap = &pool->first.next;
         while ((a = *ap) != pool->current)
             ap = &a->next;
+        JS_ASSERT(a->base == (jsuword) p);
     }
 
-    JS_ASSERT(a->base == (jsuword)p);
Put this assertion back where it was and you don't need two copies of it.


+    JS_ASSERT(aoff >= size + incr);
+    /* 
One more vertical space before long block-comments like this.


+            _nb > (pool)->arenasize)                                         \
Did you arrive at this independently, or is it from the patch in bug 408921?

Let's go one more spin, incorporating Igor's feedback as well (should be coming soon).

Comment 29

[Igor Bukanov]

Comment on attachment 305454 [details] [diff] [review]
Final patch, I hope

>Index: jsarena.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsarena.c,v
>retrieving revision 3.38
>diff -u -8 -p -r3.38 jsarena.c
>--- jsarena.c	18 Feb 2008 21:14:15 -0000	3.38
>+++ jsarena.c	25 Feb 2008 06:55:51 -0000
>@@ -17,16 +17,17 @@
>  * March 31, 1998.
>  *
>  * The Initial Developer of the Original Code is
>  * Netscape Communications Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 1998
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>+ *    Robin Bate Boerop <[e-mail]>

Please do not add this. The contributors lines are never modified nowday and stays empty. CVS commit messages is used for attribution instead. It will be very visible when one runs cvs blame.  

> /*
>  * Lifetime-based fast allocation, inspired by much prior art, including
>  * "Fast Allocation and Deallocation of Memory Based on Object Lifetimes"
>  * David R. Hanson, Software -- Practice and Experience, Vol. 20(1).
>+ * <http://storage.webhop.net/documents/fastalloc.pdf>
>  */

Nice to have a reference!

>+#if defined(XP_UNIX) && !defined(XP_MACOSX)
>+# include <malloc.h>  /* for malloc_usable_size */
>+# define HAVE_MALLOC_USABLE_SIZE
>+#elif defined(XP_MACOSX) && !defined(MOZ_MEMORY)
>+# include <malloc/malloc.h>  /* for malloc_size */
>+# define HAVE_MALLOC_SIZE
>+#elif defined(MOZ_MEMORY)
>+  size_t malloc_usable_size(void *);
>+# define HAVE_MALLOC_USABLE_SIZE
>+#endif

Use the code like that defines HAVE_MALLOC_USABLE_SIZE and ensures a declaration of malloc_usable_size when HAVE_MALLOC_USABLE_SIZE is true.

#ifndef HAVE_MALLOC_USABLE_SIZE
# if defined(XP_UNIX) && !defined(XP_MACOSX)
#  include <malloc.h>  /* for malloc_usable_size */
#  define HAVE_MALLOC_USABLE_SIZE
# elif defined(XP_MACOSX) && !defined(MOZ_MEMORY)
/*
 * malloc_size() is equivalent to malloc_usable_size().  Darwin has
 * malloc_size(), but not malloc_usable_size().
 */
#  include <malloc/malloc.h>  /* for malloc_size */
#  define HAVE_MALLOC_USABLE_SIZE 1
#  define malloc_usable_size(p)    malloc_size(p)  
# elif defined(MOZ_MEMORY)
   size_t malloc_usable_size(void *);
#  define HAVE_MALLOC_USABLE_SIZE 1
# endif
#endif

This way without changing the sources one can disable HAVE_MALLOC_USABLE_SIZE when compiling using -DHAVE_MALLOC_USABLE_SIZE=0. This is very useful for benchmarking.

>+
>+#if defined(HAVE_MALLOC_USABLE_SIZE)
>+# define MALLOC_USABLE_SIZE(p) malloc_usable_size(p)
>+#elif defined(HAVE_MALLOC_SIZE)
>+ /*
>+  * malloc_size() is equivalent to malloc_usable_size().  Darwin has
>+  * malloc_size(), but not malloc_usable_size().
>+  */
>+# define MALLOC_USABLE_SIZE(p) malloc_size(p)
>+#endif

With the above changes this should be removed. 

>+
>+/*
>+ * Allocates the requested amount of memory, or more.  The amount actually
>+ * available is pointed to by pRealSize, but only if the allocation succeeded.
>+ * So, if FlexibleMalloc returns 0, then *pRealSize is invalid.
>+ */
>+static void *
>+FlexibleMalloc(size_t requestedSize, size_t *pRealSize)
>+{
>+    void *returnPtr = malloc(requestedSize);
>+    if (!returnPtr)
>+        return NULL;
>+#ifdef MALLOC_USABLE_SIZE
>+    *pRealSize = MALLOC_USABLE_SIZE(returnPtr);
>+#else
>+    *pRealSize = requestedSize;
>+#endif

This becomes:

#if HAVE_MALLOC_USABLE_SIZE
    *pRealSize = malloc_usable_size(returnPtr);
#else
    *pRealSize = requestedSize;
#endif

>+    /* Don't wrap! */
>+    JS_ASSERT(aoff > size);
>+    JS_ASSERT(aoff > incr);
>+    JS_ASSERT(aoff >= size + incr);
>+    /* 
>+     * All reallocations must make the arena larger than it was.  No
>+     * arena can ever be smaller than pool->arenasize.  So, this
>+     * reallocation will necessarily produce an oversized arena.
>+     */

Nit: comment requires a blank line before unless they follow { or other construction that increases the indent.

> #define JS_ARENA_ALLOCATE_COMMON(p, type, pool, nb, guard)                    \
>     JS_BEGIN_MACRO                                                            \
>         JSArena *_a = (pool)->current;                                        \
>         size_t _nb = JS_ARENA_ALIGN(pool, nb);                                \
>         jsuword _p = _a->avail;                                               \
>-        if ((guard) || _p > _a->limit - _nb)                                  \
>+        if ((guard) || _p > _a->limit - _nb ||                                \
>+            _nb > (pool)->arenasize)                                          \
>             _p = (jsuword)JS_ArenaAllocate(pool, _nb);                        \
>         else                                                                  \
>             _a->avail = _p + _nb;                                             \

Why is the extra _nb > (pool)->arenasize check necessary? The oversized allocations will be tight and would not have any room left to add anything. Thus _p > _a->limit - _nb should be sufficient I think.

Comment 30

[Brian Crowder]

This passes my launch at the TryServer.  I'll launch another test of this with the new patch that incorporates my feedback and Igor's

Comment 31

[Brian Crowder]

Robin:  Please also run "make check" from the top-level of your objdir.  I ran into a crash in this patch while doing so.

Comment 32

[Robin Bate Boerop]

(In reply to comment #28)
> (From update of attachment 305454 [details] [diff] [review])
> First:  No tabs!  Kill them all!
Yikes!  I hate tabs.  I can't find any in my code, however.  Sorry if they
slipped in somehow.  My new code definitely has none.

> + * Allocates the requested amount of memory, or more.  The amount actually
> Should be "Allocate"?
Fixed.

> Instead of "TODO:" use "FIXME: bugNNNNNN" instead.
Okay.

> Would be better if you could clean this up as a ternary expression and reword
> the JS_ASSERT().
Done.

> +        JS_ASSERT(a->base == (jsuword) p);
>      } else {
>          ap = &pool->first.next;
>          while ((a = *ap) != pool->current)
>              ap = &a->next;
> +        JS_ASSERT(a->base == (jsuword) p);
>      }
> 
> -    JS_ASSERT(a->base == (jsuword)p);
> Put this assertion back where it was and you don't need two copies of it.

The two copies each serve a purpose.  When this assertion fires, one is
especially interested in which of the two cases it was.  The two statements
will give that, in the form of line information.  I have found the two
assertions useful, and I expect others might, too.

> +    JS_ASSERT(aoff >= size + incr);
> +    /* 
> One more vertical space before long block-comments like this.
Okay.

> +            _nb > (pool)->arenasize)                                         \
> Did you arrive at this independently, or is it from the patch in bug 408921?

I have not used any code from the patch in bug 408921.  That line is necessary
to ensure that allocations larger than an arenasize only ever each occupy their
own arena.  This logic is necessary for my patch.  Sadly, it adds code path and
a memory reference to the common-case allocation (where the code path does not
leave the macro by calling a function).  I don't think that there's any way
around it.

Comment 33

[Robin Bate Boerop]

(In reply to comment #29)
> > #define JS_ARENA_ALLOCATE_COMMON(p, type, pool, nb, guard)                    \
> >     JS_BEGIN_MACRO                                                            \
> >         JSArena *_a = (pool)->current;                                        \
> >         size_t _nb = JS_ARENA_ALIGN(pool, nb);                                \
> >         jsuword _p = _a->avail;                                               \
> >-        if ((guard) || _p > _a->limit - _nb)                                  \
> >+        if ((guard) || _p > _a->limit - _nb ||                                \
> >+            _nb > (pool)->arenasize)                                          \
> >             _p = (jsuword)JS_ArenaAllocate(pool, _nb);                        \
> >         else                                                                  \
> >             _a->avail = _p + _nb;                                             \
> 
> Why is the extra _nb > (pool)->arenasize check necessary? The oversized
> allocations will be tight and would not have any room left to add anything.
> Thus _p > _a->limit - _nb should be sufficient I think.

The extra check is necessary now that it is possible for an arena to be able to fit an allocation > arenasize in it.  This change is necessary to ensure that no SINGLE allocation larger than arenasize will sneak into an arena that isn't dedicated to that allocation.  In other words, there is an invariant that all allocations larger than arenasize are in their own arenas.

Igor, I really appreciate your other recommendations.  They are better than what I had.  I will make the changes.

Comment 34

[Robin Bate Boerop]

(In reply to comment #31)
> Robin:  Please also run "make check" from the top-level of your objdir.  I ran
> into a crash in this patch while doing so.

Yes, sorry.  That was due to my improperly placed assertion in JS_ARENA_GROW_CAST; it is fixed in my upcoming patch.

However, 'make check' revealed some other problems of a substantial nature.  I had to make more changes, and to complete some of the quota handling which I was previously going to leave for another bug.

My prior patch definitely should not be checked in, even with the JS_ARENA_GROW_CAST assertion moved.  I am testing my next patch, now.

Comment 35

[Jeff Walden [:Waldo]]

(In reply to comment #29)
> (From update of attachment 305454 [details] [diff] [review])
> >Index: jsarena.c
> >  * Contributor(s):
> >+ *    Robin Bate Boerop <[e-mail]>
> 
> Please do not add this. The contributors lines are never modified nowday and
> stays empty. CVS commit messages is used for attribution instead. It will be
> very visible when one runs cvs blame.  

This is wrong.  The only reason they're empty in SpiderMonkey is that the people who've hacked on it simply haven't added their names.  In general, this only gets filled in when someone explicitly adds himself when he makes a patch to the file and chooses to add himself; there are no restrictions on when you may or may not add yourself.  (I've heard some people say this should be updated for *any* change to the file, but generally people update only for "substantial"-sized changes, where "substantial" is pretty much entirely in the eye of the patch author.)  If you want to add your name here, go ahead.

Comment 36

[Robin Bate Boerop]

(In reply to comment #34)
> (In reply to comment #31)
> However, 'make check' revealed some other problems of a substantial nature.  I
> had to make more changes, and to complete some of the quota handling which I
> was previously going to leave for another bug.

I've discovered the source of my problems.  Some callers of JS_ARENA_GROW_CAST are 'lying' about the original size of allocation that they are resizing.  They are passing in a valid pointer, but not a valid size.  It seems that they are treating a series of adjacently-allocated memory as the same allocation.  So, the logic which tries to determine whether the allocation is oversized or not (if (size > pool->arenasize)...) is no longer sufficient.

This is quite rare.  'make check' passes with my changes, as do all but 6 tests run by jsDriver (that weren't already failing).  Of the 6 situations, 5 are SEGVs and 1 is an assertion failure.

This bug is exposed/created by my change because it is now possible for several contiguous allocations in an arena to have a size sum greater than arenasize even when the arena is not "oversized".  (Oversized = dedicated to a single allocation larger than arenasize.)

I have a fix in mind.  Working on it....

Comment 37

[Robin Bate Boerop]

Attachment: Assertions patch, no behaviour changes

This patch is one which isolates some of the changes in my upcoming patch.  It is only the assertions and other changes which do NOT change the behaviour of the code.

I am providing this so that my upcoming patch is easier to understand.  This patch would also be very useful for anyone else who is making changes to jsarena code, as it documents behaviour and functionality that is not clearly (or at all) documented elsewhere.  Also, the patch "proves" what it says.  :)

Comment 38

[Robin Bate Boerop]

Attachment: JS_ARENA_BACK_PTR_MAGIC patch, an understanding aid only

This patch is to help reviewers understand my changes in the upcoming patch.

This patch introduces a structure that sits behind oversized arenas' bases.  The intention is to be able to tell if an arena is oversized without trusting the sizes provided by callers to JS_ArenaRealloc and friends.

I have added assertions to help with understanding.  Some of them cannot be in the final, upcoming patch, because they will not all hold when the malloc_usable_size code in introduced.

Notice also that I have fixed a bug in JS_ARENAMETER code in JS_F...ArenaPool.

Comment 39

[Robin Bate Boerop]

Attachment: malloc_usable_size stuff added, and understanding aid only

This patch is only to help reviewers understand my upcoming patch.  This patch is to be applied (conceptually) only after the subsequent 2 review-aid patches.  This one adds in the malloc_usable_size logic.

Notice the quota logic is updated, and that some assertions from prior patches had to be commented out because they no longer hold.

Comment 40

[Robin Bate Boerop]

Attachment: check me in, please

Here are some highlights of this patch:

It works.  ;)

It fixes a problem with JS_ARENAMETER code; JS_FreeArenaPool and
JS_FinishArenaPool were not reporting deallocations correctly.

It introduces JSArenaBackPtr (give me a better name if you can think of one).
This structure holds a magic number, and serves to discriminate between
oversized allocations and allocations which are not oversized.  This was a
necessary addition, because the size being claimed by callers of the arena
growth functions were 'lying' about the size of the allocations that they
were resizing.

It makes changes to the handling of quotas.  The changes should mean that
while the quotas stay the same, and thus so does the use of 'arena memory',
the amount of real memory used should be reduced.  Of course this applies
only on platforms which support malloc_usable_size (i.e. not on Windows
until jemalloc is active).

It introduces valuable assertions to the jsarena code which will help to
make that code less brittle.  The assertions will help maintainers to
understand the code, also.  The assertions are all O(1).

The pool->current member is no longer updated on each loop iteration in
the loop in JS_ArenaAllocate, it is now only updated once at the end.

Comment 41

[Robin Bate Boerop]

Attachment: No, check ME in, please. :)

Comments that apply to attachment 306808 [details] [diff] [review] apply to this patch.

This one also handles the case of an arena which was originally not oversized (but whose malloc_usable_size was oversized) being resized to an arena which is oversized (but smaller than the malloc_usable_size of the original arena).

Comment 42

[Robin Bate Boerop]

I have measured the memory savings provided by this patch, on a 'start-stop' run of Firefox.  I just launch it and then quit it.  We save 7% 'real' memory, meaning that jsarena code consumes about 7% fewer pages than it did before the change.

If someone will point me to a more 'typical' test, then I will collect stats on that.

Comment 43

[Brian Crowder]

Robin:  Try running gmail for a bit, that's a popular and relatively JS-intensive test.

Comment 44

[Robin Bate Boerop]

(In reply to comment #43)
> Robin:  Try running gmail for a bit, that's a popular and relatively
> JS-intensive test.

Good idea.  My test involves some mouse-clicking and movement, which may affect the result.  I did my best to do the same Gmail actions identically on both test runs.  Here are the results.

Without my patch:
numMallocs:    16913
numReallocs:   220
numFrees:      16913
hwmRequested:  1149785
hwmReal:       1515296

With my patch:
numMallocs:    13809
numReallocs:   213
numFrees:      13809
hwmRequested:  1063043
hwmReal:       1136416
 
The patch improves:
- the number of mallocs by 18% (very significant)
- the high water mark of 'real' memory used by 25% (wow!)

That is WAY better than I expected.

(I also have various histograms.  I'll post if anyone wants them.)

Comment 45

[Brian Crowder]

Comment on attachment 306808 [details] [diff] [review]
check me in, please

>+/*
>+ * Invariants
>+ *
>+ * [Please add to this list.]
>+ */
Leave out the [Please add...]


>+JS_PUBLIC_API(int)
>+JS_ArenaIsOversized(void *pBase)
>+{
>+    return IS_OVERSIZED(pBase);
>+}
Let's just use IS_OVERSIZED directly, your patch doesn't introduce any other calls to this to function.  Even if it did, this would still be a private API, not public (JS_FRIEND_API, and js_ instead of JS_).  If you insist on keeping this as a function, make it static, or better: force inline it with JS_INLINE

>-    JS_ASSERT((nb & pool->mask) == 0);
>-    for (a = pool->current; nb > a->limit || a->avail > a->limit - nb;
>-         pool->current = a) {
>+    JS_ASSERT(nb == JS_ARENA_ALIGN(pool, nb));
>+    a = pool->current;

01234567890123456789012345678901234567890123456789012345678901234567890123456789
>+    while (nb > a->limit || a->avail > a->limit - nb || nb > pool->arenasize) {
This line is overlong (remember the 80-column limit).

>-                *pool->quotap -= gross;
>+                JS_ASSERT(gross <= real);
>+                *pool->quotap -= (real <= *pool->quotap) ? real : gross;
Not sure I understand the purpose of the quota-accounting difference here...

>+                /* Proof that you allocated more with malloc_usable_size: */
>+                /* printf("Recovered %d bytes.\n", (real - gross)); */
Cut these remarks, don't leave debug printfs laying around.  Maybe this is a candidate for JS_ARENAMETER stats.

>-        a = *ap;                                /* move to next arena */
>+        a = *ap;  /* move to next arena */
Please leave the comment aligned where it was, to preserve house-style (note comments 20-lines or so following this one), and minimize patch.

>+    JS_ASSERT(!extra || IS_OVERSIZED(p));
Whatever you choose to do with JS_ArenaIsOversized() above, at least make sure you use it or don't use it consistently everywhere.  I vote to just stick with IS_OVERSIZED()

> 
> JS_PUBLIC_API(void)
> JS_FreeArenaPool(JSArenaPool *pool)
> {
>-    FreeArenaList(pool, &pool->first);
>-    COUNT(pool, ndeallocs);
>+    JS_FinishArenaPool(pool);
> }
Let's just make the one consumer of this API use the other one.  (js_GetPrinterOutput)  Ditch the function in the src, and make the header contain a #define aliasing one to the other (for source compatibility).


>-JS_PUBLIC_API(void)
>-JS_ArenaFinish()
>-{
>-}
>-
>-JS_PUBLIC_API(void)
>-JS_ArenaShutDown(void)
>-{
>-}
>-
You can't whack these without breaking the API, so just leave them stubbed in.


>+#include "jsutil.h"  /* for JS_STATIC_ASSERT */
Can you include this from the .c file instead?  Static assertions in headers seem to be troublesome.

>+JS_STATIC_ASSERT(sizeof(size_t) == sizeof(jsuword));
>+JS_STATIC_ASSERT(sizeof(void *) == sizeof(jsuword));
Move back to the C file, as in previous patch?

> #define JS_ARENA_GROW_CAST(p, type, pool, size, incr)                         \
>     JS_BEGIN_MACRO                                                            \
>         JSArena *_a = (pool)->current;                                        \
>-        if (_a->avail == (jsuword)(p) + JS_ARENA_ALIGN(pool, size)) {         \
>+        void *_p = p;                                                         \
Why alias this?

Comment 46

[Brian Crowder]

Sorry for long delay on review.  We're close, though, can't wait to land this.

Comment 47

[Brendan Eich [:brendan]]

Nowhere in SpiderMonkey is anything like Hungarian notation (pBase) used. Please don't start now.

/be

Comment 48

[Robin Bate Boerop]

(In reply to comment #45)
> >+ * [Please add to this list.]
> >+ */
> Leave out the [Please add...]

Okay.

> >+JS_PUBLIC_API(int)
> >+JS_ArenaIsOversized(void *pBase)
> >+{
> >+    return IS_OVERSIZED(pBase);
> >+}
> Let's just use IS_OVERSIZED directly, your patch doesn't introduce any other
> calls to this to function.  Even if it did, this would still be a private API,
> not public (JS_FRIEND_API, and js_ instead of JS_).  If you insist on keeping
> this as a function, make it static, or better: force inline it with JS_INLINE

My patch consistently uses the IS_OVERSIZED macro in the .c file, but JS_ArenaIsOversized in the .h file.  The .h uses the function in macros which are called from other js*.c files.  If I were to try to use IS_OVERSIZED directly, then I would have to move many other macros from jsarena.c to jsarena.h, like the HEADER macros, and then the macros that they use, etc.  So, I propose to continue using IS_OVERSIZED internally in jsarena.c, but to used JS_ArenaIsOversized, a function, outside of jsarena.c.  JS_ArenaIsOversized must be callable by anyone who can call JS_ARENA_ALLOCATE.  Given that, please explain whether the JS_PUBLIC_API is appropriate or not.

> 01234567890123456789012345678901234567890123456789012345678901234567890123456789
> >+    while (nb > a->limit || a->avail > a->limit - nb || nb > pool->arenasize) {
> This line is overlong (remember the 80-column limit).

No, it's 79 characters long.  It appears longer because you started your numbering on the column used for the 'diff' marker, the '+', by accident.

> >-                *pool->quotap -= gross;
> >+                JS_ASSERT(gross <= real);
> >+                *pool->quotap -= (real <= *pool->quotap) ? real : gross;
> Not sure I understand the purpose of the quota-accounting difference here...

That's one of the meaty changes in this patch.  Here, it is possible for the caller to have asked for an arena of size n, but obtain an arena of size (n+d), where d is the overhead recovered from malloc.  (n+d) is the 'real' size, n is the 'requested' size.  What if the quota is (n+x) bytes, with x < d?  In that case, we don't want to deny the allocation because the caller has requested x < + x bytes, which does not exceed the quota.  On the other hand, if the quota allows for it, we want to bill for the (n + d) > (n + x) bytes in the arena.  The logic above solves the problem by allowing the quota to be exceeded, but only by an amount not exceeding d.

> >+                /* Proof that you allocated more with malloc_usable_size: */
> >+                /* printf("Recovered %d bytes.\n", (real - gross)); */
> Cut these remarks, don't leave debug printfs laying around.  Maybe this is a
> candidate for JS_ARENAMETER stats.
Oops!  Deleted.

> >-        a = *ap;                                /* move to next arena */
> >+        a = *ap;  /* move to next arena */
> Please leave the comment aligned where it was, to preserve house-style (note
> comments 20-lines or so following this one), and minimize patch.
Okay.

> >+    JS_ASSERT(!extra || IS_OVERSIZED(p));
> Whatever you choose to do with JS_ArenaIsOversized() above, at least make sure
> you use it or don't use it consistently everywhere.  I vote to just stick with
> IS_OVERSIZED()
I do use it consistently, as mentioned: I use the more efficient IS_OVERSIZED macro in jsarena.c, and the function outside of jsarena.c.

> > JS_PUBLIC_API(void)
> > JS_FreeArenaPool(JSArenaPool *pool)
> > {
> >-    FreeArenaList(pool, &pool->first);
> >-    COUNT(pool, ndeallocs);
> >+    JS_FinishArenaPool(pool);
> > }
> Let's just make the one consumer of this API use the other one. 
> (js_GetPrinterOutput)  Ditch the function in the src, and make the header
> contain a #define aliasing one to the other (for source compatibility).

Okay, but if source (and not binary) compatibility is all that is being maintained, then perhaps I should also define the following 2 functions to be empty macros?

> >-JS_PUBLIC_API(void)
> >-JS_ArenaFinish()
> >-{
> >-}
> >-
> >-JS_PUBLIC_API(void)
> >-JS_ArenaShutDown(void)
> >-{
> >-}
> >-
> You can't whack these without breaking the API, so just leave them stubbed in.
See my prior question.


> >+#include "jsutil.h"  /* for JS_STATIC_ASSERT */
> Can you include this from the .c file instead?  Static assertions in headers
> seem to be troublesome.
Done.

> > #define JS_ARENA_GROW_CAST(p, type, pool, size, incr)                         \
> >     JS_BEGIN_MACRO                                                            \
> >         JSArena *_a = (pool)->current;                                        \
> >-        if (_a->avail == (jsuword)(p) + JS_ARENA_ALIGN(pool, size)) {         \
> >+        void *_p = p;                                                         \
> Why alias this?

Because, previously, the "p" argument of the macro was of a type determined by the caller.  With the new assertions in that macro, the type of p becomes important when calling JS_ArenaIsOversized().  That function will give a compilation warning in the files that use the macro if p is not of the correct type.  So, I introduce _p, of the correct type, to suppress the warnings.

Comment 49

[Robin Bate Boerop]

(In reply to comment #47)
> Nowhere in SpiderMonkey is anything like Hungarian notation (pBase) used.
> Please don't start now.

Okay.  Fixed.

Comment 50

[Brian Crowder]

> My patch consistently uses the IS_OVERSIZED macro in the .c file, but
> JS_ArenaIsOversized in the .h file.  The .h uses the function in macros which
> are called from other js*.c files.  If I were to try to use IS_OVERSIZED
> directly, then I would have to move many other macros from jsarena.c to
> jsarena.h, like the HEADER macros, and then the macros that they use, etc.  So,
> I propose to continue using IS_OVERSIZED internally in jsarena.c, but to used
> JS_ArenaIsOversized, a function, outside of jsarena.c.  JS_ArenaIsOversized
> must be callable by anyone who can call JS_ARENA_ALLOCATE.  Given that, please
> explain whether the JS_PUBLIC_API is appropriate or not.

Rename IS_OVERSIZED to JS_ARENA_IS_OVERSIZED, and put it in the header.


> No, it's 79 characters long.  It appears longer because you started your
> numbering on the column used for the 'diff' marker, the '+', by accident.

Duh, sorry.  Need a better ruler.  :)


> > >+                *pool->quotap -= (real <= *pool->quotap) ? real : gross;
> > Not sure I understand the purpose of the quota-accounting difference here...
> 
> That's one of the meaty changes in this patch.  Here, it is possible for the
> caller to have asked for an arena of size n, but obtain an arena of size (n+d),
> where d is the overhead recovered from malloc.  (n+d) is the 'real' size, n is
> the 'requested' size.  What if the quota is (n+x) bytes, with x < d?  In that
> case, we don't want to deny the allocation because the caller has requested x <
> + x bytes, which does not exceed the quota.  On the other hand, if the quota
> allows for it, we want to bill for the (n + d) > (n + x) bytes in the arena. 
> The logic above solves the problem by allowing the quota to be exceeded, but
> only by an amount not exceeding d.
> 

I know you have a separate bug on this quota question.  I'm feeling a bit like we should answer that question there, and not here.  I'm loathe to charge the caller for an overage -we're- causing.  OTOH embedders may really mean it when they apply quota numbers.  I'm interested to hear Igor's thoughts on this quota quibble.


> > contain a #define aliasing one to the other (for source compatibility).
> 
> Okay, but if source (and not binary) compatibility is all that is being
> maintained, then perhaps I should also define the following 2 functions to be
> empty macros?

Sure, that's cool.

Comment 51

[Robin Bate Boerop]

(In reply to comment #50)
> > > >+                *pool->quotap -= (real <= *pool->quotap) ? real : gross;
> > > Not sure I understand the purpose of the quota-accounting difference here...
> > 
> > That's one of the meaty changes in this patch.  Here, it is possible for the
> > caller to have asked for an arena of size n, but obtain an arena of size (n+d),
> > where d is the overhead recovered from malloc.  (n+d) is the 'real' size, n is
> > the 'requested' size.  What if the quota is (n+x) bytes, with x < d?  In that
> > case, we don't want to deny the allocation because the caller has requested x <
> > + x bytes, which does not exceed the quota.  On the other hand, if the quota
> > allows for it, we want to bill for the (n + d) > (n + x) bytes in the arena. 
> > The logic above solves the problem by allowing the quota to be exceeded, but
> > only by an amount not exceeding d.
> I know you have a separate bug on this quota question.  I'm feeling a bit like
> we should answer that question there, and not here.  I'm loathe to charge the
> caller for an overage -we're- causing.  OTOH embedders may really mean it when
> they apply quota numbers.  I'm interested to hear Igor's thoughts on this quota
> quibble.

Yeah, there are some issues, here.  In the following, I'm talking about 'real' memory use:

Option 1: we charge only for the gross, as we used to do.  Then, we are consuming more memory that we are being charged for.  This is how it works before my patch.
Option 2: we charge for the real memory, but then callers get out-of-memory problems where they didn't before my patch, and get upset at me.  Indeed, if I try this, I get failures in the test suite.
Option 3: we do what I did, above, and then switch to Option 2 at a later date in another defect.
Option 4: something else.

Embedders may not like Option 1, as you mention.  Also, Option 1 would mean that some callers may not reach a quota limit when then used to.  Igor was against Option 1, which was in one of my original patches.  Some tests fail with Option 1, as with Option 2.

Option 3 works, but makes it unclear what the "quota" is counting.  (Mind you, so do the other two options, IMO.)

> > > contain a #define aliasing one to the other (for source compatibility).
> > 
> > Okay, but if source (and not binary) compatibility is all that is being
> > maintained, then perhaps I should also define the following 2 functions to be
> > empty macros?
> 
> Sure, that's cool.

Will do.

Comment 52

[Brian Crowder]

Comment on attachment 307072 [details] [diff] [review]
No, check ME in, please.  :)

Waiting for next patch.

Comment 53

[Igor Bukanov]

(In reply to comment #51)
> Yeah, there are some issues, here.  In the following, I'm talking about 'real'
> memory use:
> 
> Option 1: we charge only for the gross, as we used to do.  Then, we are
> consuming more memory that we are being charged for.  This is how it works
> before my patch.
> Option 2: we charge for the real memory, but then callers get out-of-memory
> problems where they didn't before my patch, and get upset at me.  Indeed, if I
> try this, I get failures in the test suite.
> Option 3: we do what I did, above, and then switch to Option 2 at a later date
> in another defect.
> Option 4: something else.

I look at this from a different angle. The size of the arena that we ask to allocate is a runtime parameter. Such parameter is not necessary fixed but rather may be (or even should be) tuned for the best runtime performance. The extra space after malloc is one of the options to tune the size.

Ideally the size of the arena should be optimized so malloc is tight. Obviously one can not do it in advance at the compile time. But malloc_size() allows to fix this after the allocation. Effectively this turns arena's size into a hint to allocate at least that size of memory.

From this point of view malloc_size() should account for the quota since in an ideal situation we will ask to allocate that amount of memory.  

Comment 54

[Brian Crowder]

Robin:  Can you pick the nits from the previous patch and send up a new one for review?  We'll deal with the quota issue in your other bug and live with option 3 for now (unless Igor strongly disagrees, which I wasn't totally clear on from his comment).

Comment 55

[Robin Bate Boerop]

Attachment: Adds JS_ARENA_RESET_OVERSIZED functionality

Here is the patch with nits from prior patches addressed.

This also introduces the important JS_ARENA_RESET_OVERSIZED macro, which is necessary.  It ensures that we erase the JS_ARENA_BACK_PTR_MAGIC flag before we free or realloc an arena.  That way, we are much less likely to allocate a non-oversized arena which happens to have JS_ARENA_BACK_PTR_MAGIC in exactly the right place by mere coincidence.

Comment 56

[Brian Crowder]

Comment on attachment 307296 [details] [diff] [review]
Adds JS_ARENA_RESET_OVERSIZED functionality

Just a thought:  check backpointer AND limit - base to determine oversizedness?

Full review in a sec

Comment 57

[Igor Bukanov]

(In reply to comment #54)
> Robin:  Can you pick the nits from the previous patch and send up a new one for
> review?  We'll deal with the quota issue in your other bug and live with option
> 3 for now (unless Igor strongly disagrees, which I wasn't totally clear on from
> his comment).

To be specific: I prefer to account in the quota for the total malloc_size(), not the size of the arena. This is based on the assumption that malloc_size() is the size that we should ask in the first place when allocating arenas.

But surely this can be fixed in a separated bug.

Comment 58

[Robin Bate Boerop]

(In reply to comment #56)
> (From update of attachment 307296 [details] [diff] [review])
> Just a thought:  check backpointer AND limit - base to determine oversizedness?

To check those values, we would have to trust (and follow) the header->arenaBackPtr member (to get the JSArena) which would be bad news if the the arenaBackPtr does not point to a valid JSArena.

Further, this (limit-base) value could be larger than arenasize even in a non-oversized arena.  That's why the JSArenaBackPtr was necessary to begin with.

Comment 59

[Brian Crowder]

Comment on attachment 307296 [details] [diff] [review]
Adds JS_ARENA_RESET_OVERSIZED functionality

>+#define IS_MAC (defined(DARWIN) || defined(XP_MACOSX))

Something about this makes my spidermonkey-style spidey-sense tingle...  not sure why.  I might feel better if you just used the longer expression.  But, I'm not attached, someone else may be (Brendan?).


>- * However, we may need to add more space to pad the JSArena ** back-pointer
>+ * However, we may need to add more space to pad the JSArenaBackPtr
>  * so that it lies just behind a->base, because a might not be aligned such
>  * that (jsuword)(a + 1) is on a pointer boundary.

This comment now needs a rewrap.


>+#define HEADER_SIZE(pool)        (sizeof(JSArenaBackPtr) \
>                                  + (((pool)->mask < POINTER_MASK)             \
>                                     ? POINTER_MASK - (pool)->mask             \
>                                     : 0))

Need to indent the lines following the HEADER_SIZE line to match the space you've added to it.  (ie., the '+' should be one space deeper, and so on).  Also, '\' should always be in column 79.


>+/*
>+ * Allocate the requested amount of memory, or more.  The amount actually
>+ * available is pointed to by pRealSize, but only if the allocation succeeded.
>+ * So, if FlexibleMalloc returns 0, then *pRealSize is invalid.
>+ */
>+static void *
>+FlexibleMalloc(size_t requestedSize, size_t *pRealSize)

Woops, more Hungarian!  realsize would be fine.


>-    JSArena **ap, *a, *b;
>-    jsuword extra, hdrsz, gross;
>+    JSArena **ap, *a;
>+    jsuword extra, hdrsz, gross, real;

Nice to see the puzzling 'b' go away here.


>+                /* Oversized arenas don't use the (real - gross) bytes. */
>+                a->limit = (jsuword)a + gross;
Need a ' ' between (jsuword) and 'a' here.
>+                a->limit = (jsuword)a + real;
And here.


>-        a = *ap;                                /* move to next arena */
>+        a = *ap;                                       /* move to next arena */

Again, just leave this where it was.


>+    pool->current = a;
>+    JS_ASSERT(a->avail <= a->limit - nb);
>     p = (void *)a->avail;
Fix the cast spacing for this pre-existing code, while you're in here
|(void *) a->avail|.


>+    JS_ASSERT((nb <= pool->arenasize && !JS_ARENA_IS_OVERSIZED(p))
>+              || (nb > (pool)->arenasize && JS_ARENA_IS_OVERSIZED(p)));

Don't over-parenthesize pool here.

>-    jsuword boff, aoff, extra, hdrsz, gross, growth;
>+    jsuword boff, aoff, extra, hdrsz, gross, quotaChange;

How about just "change"...  the introduction of studlyCaps here is wonky with the rest of the code.  Or quotachange, though that's verbose.


>+    JS_ARENA_RESET_OVERSIZED(a);

Maybe this should be JS_ARENA_SET_OVERSIZED()?


>+        if (gross > (a->limit - (jsuword) a))
>+            *pool->quotap -= quotaChange;
>+        else
>+            *pool->quotap += quotaChange;

Alternatively:
*pool->quotap += (gross > (a->limit - (jsuword) a)) ? -quotaChange : quotaChange
?

>-    JS_ASSERT(a->base <= a->avail && a->avail <= a->limit);
>+    JS_ASSERT(a->base <= a->avail);
>+    JS_ASSERT(a->avail <= a->limit);
Unsplit this back to the original form?  It will better match your own comments that way, and minimize the patch.


> #ifdef JS_ARENAMETER
>+
No extra vertical space here.


>Index: js/src/jsarena.h
>+struct JSArenaBackPtr {
>+    jsuword arenaBackPtrMagic;
>+    JSArena **arenaBackPtr;  /* the address of previous arena's 'next' member */
>+};

"magic" and "a" would probably be fine, succinct names for these, esp. since we already know we're in a JSArenaBackPtr struct, and "a" is common for JSArena *'s throughout the rest of the code.  I am not in love with the use of the magic number, though, I wish there were a better way.


>+#define JS_ARENA_RESET_OVERSIZED(arena)                                       \
>+    JS_BEGIN_MACRO                                                            \
>+        jsuword _arenaBase = (arena)->base;                                   \
Just _base ?


> #define JS_ARENA_DESTROY(pool, a, pnext)                                      \
>     JS_BEGIN_MACRO                                                            \
>         JS_COUNT_ARENA(pool,--);                                              \
>+        JS_ARENA_RESET_OVERSIZED(a);                                          \

I think you shouldn't bother with this on destroy, or maybe DEBUG-only.


>- * again unless JS_FinishArenaPool(pool) has been called.
>+ * Deprecated functions
Make this a full sentence instead of a fragment, even if it's just "The following functions are deprecated."


>+#define JS_ArenaFinish
>+#define JS_ArenaShutDown

These should be parameterized macros, no?  Will this build as a proper NOOP for people invoking them?  What about inside conditionals?  Gobble parameters and make sure your noop is a statement.  (void *)0 ?


Sorry I missed some of this stuff on the last go-'round, we're close.

Comment 60

[Robin Bate Boerop]

(In reply to comment #59)
> >- * However, we may need to add more space to pad the JSArena ** back-pointer
> >+ * However, we may need to add more space to pad the JSArenaBackPtr
> This comment now needs a rewrap.
Rewrapped.

> >+#define HEADER_SIZE(pool)        (sizeof(JSArenaBackPtr) \
> >                                  + (((pool)->mask < POINTER_MASK)             \
> >                                     ? POINTER_MASK - (pool)->mask             \
> >                                     : 0))
> Need to indent the lines following the HEADER_SIZE line to match the space
> you've added to it.  (ie., the '+' should be one space deeper, and so on). 
> Also, '\' should always be in column 79.
Done.  You mean column 78, right?  That's the column that everything started on.  If you really do mean 79, I'll adjust accordingly.  Gee, it's too bad that I'm having to learn all of the js* coding style on such a big patch.  I'm sorry to create patch review rejection that are style-based.

> >+static void *
> >+FlexibleMalloc(size_t requestedSize, size_t *pRealSize)
> Woops, more Hungarian!  realsize would be fine.
Updated.

> >+                /* Oversized arenas don't use the (real - gross) bytes. */
> >+                a->limit = (jsuword)a + gross;
> Need a ' ' between (jsuword) and 'a' here.
> >+                a->limit = (jsuword)a + real;
> And here.
Fixed.

> >-        a = *ap;                                /* move to next arena */
> >+        a = *ap;                                       /* move to next arena */ 
> Again, just leave this where it was.
You suggested "leaving it" aligned with the comments 20 or so lines later.  But, the two were mutually exclusive.  :(

> >+    pool->current = a;
> >+    JS_ASSERT(a->avail <= a->limit - nb);
> >     p = (void *)a->avail;
> Fix the cast spacing for this pre-existing code, while you're in here
> |(void *) a->avail|.
Fixed.

> >+    JS_ASSERT((nb <= pool->arenasize && !JS_ARENA_IS_OVERSIZED(p))
> >+              || (nb > (pool)->arenasize && JS_ARENA_IS_OVERSIZED(p)));
> Don't over-parenthesize pool here.
Oops.  Fixed.

> >-    jsuword boff, aoff, extra, hdrsz, gross, growth;
> >+    jsuword boff, aoff, extra, hdrsz, gross, quotaChange;
> How about just "change"...  the introduction of studlyCaps here is wonky with
> the rest of the code.  Or quotachange, though that's verbose.
Used quotachange.

> >+    JS_ARENA_RESET_OVERSIZED(a);
> Maybe this should be JS_ARENA_SET_OVERSIZED()?
No, we're not setting the arena to be oversized, we're setting it to be not-oversized.  Resetting the oversized-ness.

> >+        if (gross > (a->limit - (jsuword) a))
> >+            *pool->quotap -= quotaChange;
> >+        else
> >+            *pool->quotap += quotaChange;
> Alternatively:
> *pool->quotap += (gross > (a->limit - (jsuword) a)) ? -quotaChange :
> quotaChange
Would result in unary minus operation on an unsigned value.

> >-    JS_ASSERT(a->base <= a->avail && a->avail <= a->limit);
> >+    JS_ASSERT(a->base <= a->avail);
> >+    JS_ASSERT(a->avail <= a->limit);
> Unsplit this back to the original form?  It will better match your own comments
> that way, and minimize the patch.
Okay.

> > #ifdef JS_ARENAMETER
> >+
> No extra vertical space here.
Removed.

> >Index: js/src/jsarena.h
> >+struct JSArenaBackPtr {
> >+    jsuword arenaBackPtrMagic;
> >+    JSArena **arenaBackPtr;  /* the address of previous arena's 'next' member */
> >+};
> "magic" and "a" would probably be fine, succinct names for these, esp. since we
> already know we're in a JSArenaBackPtr struct, and "a" is common for JSArena
> *'s throughout the rest of the code.  I am not in love with the use of the
> magic number, though, I wish there were a better way.
Yeah, the magic number is not the best.  It does have the useful side effect of helping detect memory corruption, though, on DEBUG builds.

 
> >+#define JS_ARENA_RESET_OVERSIZED(arena)                                       \
> >+    JS_BEGIN_MACRO                                                            \
> >+        jsuword _arenaBase = (arena)->base;                                   \
> Just _base ?
To avoid camelcase, you mean?

> > #define JS_ARENA_DESTROY(pool, a, pnext)                                      \
> >     JS_BEGIN_MACRO                                                            \
> >         JS_COUNT_ARENA(pool,--);                                              \
> >+        JS_ARENA_RESET_OVERSIZED(a);                                          \
> I think you shouldn't bother with this on destroy, or maybe DEBUG-only.
It's necessary.  Otherwise, it is possible for a realloc'd arena to contain the magic number just before it's base, and thus appear to be an oversized arena when it's not.  Many hours of debugging taught me that.

> >- * again unless JS_FinishArenaPool(pool) has been called.
> >+ * Deprecated functions
> Make this a full sentence instead of a fragment, even if it's just "The
> following functions are deprecated."
Done.

> >+#define JS_ArenaFinish
> >+#define JS_ArenaShutDown
> These should be parameterized macros, no?  Will this build as a proper NOOP for
> people invoking them?  What about inside conditionals?  Gobble parameters and
> make sure your noop is a statement.  (void *)0 ?
Fixed.  Please check my solution carefully, as there are no uses of those functions to test on.

Comment 61

[Brian Crowder]

> I'm having to learn all of the js* coding style on such a big patch.  I'm sorry
> to create patch review rejection that are style-based.

I'm still learning it myself, on big patches and small.


> > >-        a = *ap;                                /* move to next arena */
> > >+        a = *ap;                                       /* move to next arena */ 
> > Again, just leave this where it was.
> You suggested "leaving it" aligned with the comments 20 or so lines later. 
> But, the two were mutually exclusive.  :(

huh, when I looked, it seemed like it matched cleanly...  this is good if it all lines up.


> > >+    JS_ARENA_RESET_OVERSIZED(a);
> > Maybe this should be JS_ARENA_SET_OVERSIZED()?
> No, we're not setting the arena to be oversized, we're setting it to be
> not-oversized.  Resetting the oversized-ness.

CLEAR, then?


> > *pool->quotap += (gross > (a->limit - (jsuword) a)) ? -quotaChange :
> > quotaChange
> Would result in unary minus operation on an unsigned value.

Fair enough.  Still, would be nicer as a ternary, if you can work it out.


> > >+        jsuword _arenaBase = (arena)->base;                                   \
> > Just _base ?
> To avoid camelcase, you mean?
And verbosity.


> 
> > > #define JS_ARENA_DESTROY(pool, a, pnext)                                      \
> > >     JS_BEGIN_MACRO                                                            \
> > >         JS_COUNT_ARENA(pool,--);                                              \
> > >+        JS_ARENA_RESET_OVERSIZED(a);                                          \
> > I think you shouldn't bother with this on destroy, or maybe DEBUG-only.
> It's necessary.  Otherwise, it is possible for a realloc'd arena to contain the
> magic number just before it's base, and thus appear to be an oversized arena
> when it's not.  Many hours of debugging taught me that.

Ugh!


> > people invoking them?  What about inside conditionals?  Gobble parameters and
> > make sure your noop is a statement.  (void *)0 ?
> Fixed.  Please check my solution carefully, as there are no uses of those
> functions to test on.

Patch coming right up, I assume?

Comment 62

[Brian Crowder]

> > Fixed.  Please check my solution carefully, as there are no uses of those
> > functions to test on.
> 
> Patch coming right up, I assume?
> 

For my sanity and yours, try these out in js.c or something (not to be included in the patch) before uploading the patch.  Make sure your solution works in conditionals.

Comment 63

[Robin Bate Boerop]

Attachment: Suggested changes made

With the suggested changes.

Comment 64

[Brian Crowder]

Comment on attachment 307342 [details] [diff] [review]
Suggested changes made

I'd like to see a patch (or your answer to) that deals with the CLEAR suggestion and arenaBase, and I'd also love to have Igor's eyes on this.  You have recently run all the test-suites at your disposal on this, right, Robin?

Comment 65

[Robin Bate Boerop]

(In reply to comment #64)
Calling it JS_ARENA_CLEAR_OVERSIZE instead of JS_ARENA_RESET_OVERSIZED implies that something is zeroed.  Clearing == zeroing.  But, I don't zero it.  So, the CLEAR name is not quite right, though the RESET name is not perfect either.  I'm up for other suggestions if you think it's worth holding up the patch for.

The 'arenaBase' to 'base' change is not one that I want to make.  Though, if camelCase is not appropriate for js style, I'd gladly change it.  Just calling it 'base' is not as meaning-rich to a first-time reader of the function.  They'll ask "base of what?".  'arenaBase' answers that question.  So would 'arena_base', or 'base_of_arena', or 'arenabase' or whatever.  I'm just trying to write more meaningful code.

As it is, there are too many variables with meaningless names in this code, like 'a', 'ap', and 'base'.  While it is easy for a knowledgeable reader to understand what's going on, a new reader of the code would prefer more meaningful variable names.  Also, better names make for better LXR indexability.  Suppose that one wants to know the 'limit' member of arenas are used, and what code changes that structure member.  Feeding 'limit' to LXR will yield too many false hits.  But, if the 'limit' member of arenas were called 'arenaLimit', then LXR would give useful results.  Better name choices would help avoid name collisions in macros, also.

That said, I wouldn't want to delay this patch over a naming or style issue, and will do whatever it takes to get it approved.

To test this, I ran 'make check'.  I also ran 'jsDriver' with -L slow-n.tests and -L spidermonkey-n.tests; I had 167 failures on that - the same as with an unmodified build.  I used Gmail.  I will run whatever further tests are suggested to me.  I'll try running without the -L options on jsDriver.

Comment 66

[Brian Crowder]

I'm satisfied with the patch and will land it pending 1.9 approval and Igor's review.

Comment 67

[Robin Bate Boerop]

(In reply to comment #62)
> > > Fixed.  Please check my solution carefully, as there are no uses of those
> > > functions to test on.
> For my sanity and yours, try these out in js.c or something (not to be included
> in the patch) before uploading the patch.  Make sure your solution works in
> conditionals.

Done, but the 3 functions in question all returned void, so wouldn't work in
conditionals in any case.

Comment 68

[Brian Crowder]

Sorry, by conditionals I meant this:

old code:

if (foo)
    JS_ArenaFinish();
bar();

If you don't properly define JS_ArenaFinish()'s macro, then this change will change the semantics of the program (causing the client to execute bar() if foo, instead of always).

Comment 69

[Brendan Eich [:brendan]]

(In reply to comment #65)
> (In reply to comment #64)
> Calling it JS_ARENA_CLEAR_OVERSIZE instead of JS_ARENA_RESET_OVERSIZED implies
> that something is zeroed.  Clearing == zeroing.  But, I don't zero it.  So, the
> CLEAR name is not quite right, though the RESET name is not perfect either. 
> I'm up for other suggestions if you think it's worth holding up the patch for.

RESET is better for the reason you give.

> The 'arenaBase' to 'base' change is not one that I want to make.  Though, if
> camelCase is not appropriate for js style, I'd gladly change it.

Verbose struct member and macro parameter names are not consistent with the style of in this code.

A lot of the incremental style nit-picking could be avoided by observing the prevailing style. That's good manners hacking on any code you're not rewriting and planning to own for a longer term.

The macro parameter name _arenaBase isn't "bad". It's just a little long, and however nice to first-time readers, it may become tedious to maintainers. But it's not a big deal.

> As it is, there are too many variables with meaningless names in this code,
> like 'a', 'ap', and 'base'.

Those names are used consistently and well-defined in context. For example, base in context of JSArena needs to prefix, and prepending arena would just make for tedium.

Some of this is "just so", like a typographic convention such as French spacing (two spaces after full stop, which I note you use, which I used to use for many years, but which I no longer use). Again, follow prevailing style when patching.

/be

Comment 70

[Brendan Eich [:brendan]]

> base in context of JSArena needs to prefix, and prepending arena would just

s/needs to/needs no/

/be

Comment 71

[Brendan Eich [:brendan]]

JS_ArenaFinish should not become a macro (I don't understand comment 68, however; definining it as ((void)0) will not result in dangling else bugs, warnings about empty statements, or any other problem I can see). It has been JS_PUBLIC_API for going on a decade as open source, longer before then. Embedders may use it and want to keep linking against it for whatever reason. It is part of a primitive contract we should uphold.

Same goes for other JS_PUBLIC_API(T) JS_Foo(...) entry points.

/be

Comment 72

[Brian Crowder]

(In reply to comment #71)
> JS_ArenaFinish should not become a macro (I don't understand comment 68,
> however; definining it as ((void)0) will not result in dangling else bugs,
> warnings about empty statements, or any other problem I can see). It has been
> JS_PUBLIC_API for going on a decade as open source, longer before then.
> Embedders may use it and want to keep linking against it for whatever reason.
> It is part of a primitive contract we should uphold.

Comment 68 was explaining why I wanted it to be ((void) 0) and not empty, as it had been previously.  As for whether these functions should be stubbed out as macros, I don't see why not?  They've not done anything in an age (they don't even take parameters in their current incarnation), and we break binary compatibility fairly frequently (though, I confess I cannot site a recent example).  These at least remain source-compatible, as they are.  Anyone actually using the arena code in the presence of this patch will need a recompile anyway, since the ARENA_ALLOC macros have changed significantly.

Comment 73

[Brendan Eich [:brendan]]

crowder: I'm not concerned about the change to ((void)0) and I'm glad you are in favor of that over an empty macro. But at this point, getting rid of an API stub is just borrowing trouble. Breaking binary compatibility happened going from 1.8 to 1.9, but not since AFAIK.

Why borrow trouble if the cost of the stub is tiny?

/be

Comment 74

[Brian Crowder]

I still think any JSArena consumer is going to have to rebuild no matter what, but I'm not attached enough to defend it further.  Robin, can you please leave these stubs in?  Thanks.

Comment 75

[Robin Bate Boerop]

Attachment: Removed API changes

This patch incorporates Brendan's request.  Regardless of whether the deprecated functions should be removed from the code or not, this patch is not the place to do it, since it obscures what the patch is trying to achieve.  This patch does not delete the deprecated functions, but still corrects the JS_ARENAMETER bug in JS_FinishArenaPool.

Comment 76

[Brendan Eich [:brendan]]

Robin, thanks for separating concerns. Brian, you may be right, obviously so if this patch changes something in jsarena.h that requires recompilin (I haven't read the whole patch yet), but it still seems best to separate the macro-stubbing.

/be

Comment 77

[Igor Bukanov]

Comment on attachment 307403 [details] [diff] [review]
Removed API changes

>Index: js/src/jsarena.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsarena.c,v
>retrieving revision 3.38
>diff -u -8 -p -r3.38 jsarena.c
>--- js/src/jsarena.c	18 Feb 2008 21:14:15 -0000	3.38
>+++ js/src/jsarena.c	5 Mar 2008 06:04:50 -0000
>@@ -17,16 +17,17 @@
>  * March 31, 1998.
>  *
>  * The Initial Developer of the Original Code is
>  * Netscape Communications Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 1998
>  * the Initial Developer. All Rights Reserved.
>  *
>  * Contributor(s):
>+ *    Robin Bate Boerop  <[e-mail]>
>  *

Sorry to raise this issue again, but Contributor(s): is not used in SpiderMonkey sources. The attribution is tacked via CVS commit messages and bugzilla.

Comment 78

[Igor Bukanov]

(In reply to comment #33)
> (In reply to comment #29)
> > Why is the extra _nb > (pool)->arenasize check necessary? The oversized
> > allocations will be tight and would not have any room left to add anything.
> > Thus _p > _a->limit - _nb should be sufficient I think.
> 
> The extra check is necessary now that it is possible for an arena to be able to
> fit an allocation > arenasize in it.  This change is necessary to ensure that
> no SINGLE allocation larger than arenasize will sneak into an arena that isn't
> dedicated to that allocation.  In other words, there is an invariant that all
> allocations larger than arenasize are in their own arenas.

But why oversized arenas have any room available in them? I thought when the arena is oversized, then arena->avail == arena->limit. 

Comment 79

[Stuart Parmenter]

igor: per section 3.5 of the MPL:  "If You created one or more Modification(s) You may add your name as a Contributor to the notice described in Exhibit A." -- it is a license thing, not a module thing.

Comment 80

[Robin Bate Boerop]

(In reply to comment #78)
> But why oversized arenas have any room available in them? I thought when the
> arena is oversized, then arena->avail == arena->limit. 

I would prefer that this were so, but it is not.  It is due to how the 'gross' and 'hdrsz' and other such values are calculated and rounded off.

I have opened bug 420476 to fix the problem.  (It IS a problem, because it may result in mallocations which are unnecessarily large.)  Perhaps someone would be so kind as to assign that bug to me?

Comment 81

[Igor Bukanov]

(In reply to comment #80)
> (In reply to comment #78)
> > But why oversized arenas have any room available in them? I thought when the
> > arena is oversized, then arena->avail == arena->limit. 
> 
> I would prefer that this were so, but it is not.  It is due to how the 'gross'
> and 'hdrsz' and other such values are calculated and rounded off.

Would simply setting arena->limit to arena->avail after the allocation is done for oversized arenas fix this?

Comment 82

[Robin Bate Boerop]

(In reply to comment #81)
> (In reply to comment #80)
> > (In reply to comment #78)
> > > But why oversized arenas have any room available in them? I thought when the
> > > arena is oversized, then arena->avail == arena->limit. 
> > 
> > I would prefer that this were so, but it is not.  It is due to how the 'gross'
> > and 'hdrsz' and other such values are calculated and rounded off.
> 
> Would simply setting arena->limit to arena->avail after the allocation is done
> for oversized arenas fix this?

It would mean that avail == limit, but it would not change the fact that the arena had (maybe) previously allocated slightly more than it should have.  Also, it would mess up the quota accounting.  When the arena is freed, the quota is augmented by (a->limit - a).  So, if that quantity is not the quantity by which the quota was decremented, then there would be a quota bug.  I hope to clarify and fix things in bug 420476, which I am actively working on.

Comment 83

[Robin Bate Boerop]

Should this bug be flagged wanted-next, to match bug 408921?

Comment 84

[Robin Bate Boerop]

(In reply to comment #20)
> > I would like to learn the reasons behind the style
> > choices.  That is important, because most of the choices I've seen made in js
> > are exactly the opposite from those made in other contexts.  :)
> Other contexts within spidermonkey?
No, other C development shops.

> > Further, some of the style choices have runtime consequences.
> Which ones?

One style choice which has a (negative) runtime consequence is the one which prefers
  if (unlikely_condition)
      return ERROR;
  happyPath();
  return SUCCESS;
to
  if (likely_condition) {
      happyPath();
      return SUCCESS;
  }
  return ERROR;

Here, the latter form results in other style problems, but it is faster because of how static branch prediction works on moderns processors.

However, as Brendan has pointed out elsewhere, profile-guided optimization (which Mozilla does on releases, I guess) cancels out the negative effects.  So, we should prefer the "better looking code" style.

Comment 85

[Brian Crowder]

Comment on attachment 307403 [details] [diff] [review]
Removed API changes

fwiw, this patch doesn't seem to improve sunspider noticeably, but it doesn't hurt either.  On some runs, I saw as much as a 2% improvement, never much worse than 1% or so degradation.

Comment 86

[Robin Bate Boerop]

If anyone is planning on trying out the patch: remember that it will have no performance effect on Windows.  This is because Windows has no malloc_usable_size().  The patch will only have an effect on Mac and on UNIXes with malloc_usable_size defined.  Later, the patch will have an effect on Windows when the jemalloc code is activated.

Comment 87

[Stuart Parmenter]

uh, jemalloc has been enabled on windows for at least a month...

Comment 88

[Robin Bate Boerop]

(In reply to comment #87)
> uh, jemalloc has been enabled on windows for at least a month...
Stuart: thanks.  I saw bug 419470 (Figure out why building jemalloc breaks with PGO enabled), where Ted said "We're just going to disable this for the time being, but we should figure out why it breaks."  I realize now that he meant PGO is to be disabled, not jemalloc.  Duh.  Sorry.

With jemalloc enabled, this bug should save slightly more memory than it does on  Mac.  (This is due to jemalloc having larger differences between quantum sizes for mallocations.)

Comment 89

[Stuart Parmenter]

also, ted was talking about on linux with PGO

Comment 90

[Brian Crowder]

Review-ping.

Comment 91

[Brendan Eich [:brendan]]

Comment on attachment 307403 [details] [diff] [review]
Removed API changes

First, thanks for cleaning up this old code. It's not often one has 13-year old code hang around and be hacked sporadically under duress, and finally get some needed rejuvenation.

Second, most of my comments are nit-picky, but the patch makes many style changes as well as substantial ones, and style matters.

Finally, I hope this comes in time -- long day, tired now, will plow through. One more patch and I'll r+.

/be

----- Comments start here -----

I like your suggestion from bug 419131 comment 16:

#ifndef USE_MALLOC_USABLE_SIZE
# if defined(MOZ_MEMORY)
size_t malloc_usable_size(void *);
#  define USE_MALLOC_USABLE_SIZE 1
# endif
#endif

But do extern the malloc_usable_size prototype for C++ compilation support and local style, and put JS_BEGIN/END_EXTERN_C around it for C++ too.

>  * lies just behind a->base, because a might not be aligned such that

Suggest "because the arena might not ..." instead.

> #define JS_ASSERT_PTR_ALIGNED(p) JS_ASSERT(0 == ((jsuword) p & POINTER_MASK))

Lose the JS_ prefix for a local macro, and parenthesize (p) in the macro definition.

HEADER and BackPtr are conflicting tropes -- suggest Header for the name-part instead of BackPtr: JSArenaHeader.

Strongly prefer magic and prevp or backp for the member names, for consistent brevity without ambiguity in context of HEADER macrology (e.g. PTR_TO_HEADER(...)->magic).

The macro definitions starting with

#define POINTER_MASK  ...

did not change as much as the diff suggests -- could you minimize change based on losing the JS_ prefix and reindenting? Just trying to keep the patch small at this stage of the release, or smaller at any rate ;-).

>  * So, if FlexibleMalloc returns 0, then *realsize is invalid.

s/0/null/

Total style nit: most of the code declares without initializing, instead of

    void *ptr = malloc(requestedsize);
    if (!ptr)
        ...

etc. But that's to avoid losing initializers in a forest of declarations, not a problem here. Up to you.

Name nit: wantsize (to match realsize in length/density) vs. requestedsize, which is a bit too vowel-y.

One blank line before whole- and multi-line comments:

                /* Oversized arenas don't use the (real - gross) bytes. */

This:

    JS_ASSERT((nb <= pool->arenasize && !JS_ARENA_IS_OVERSIZED(p))
              || (nb > pool->arenasize && JS_ARENA_IS_OVERSIZED(p)));

wants to be this (if it didn't, the || would still want to go at the end of the first line):

    JS_ASSERT((nb <= pool->arenasize) ^ JS_ARENA_IS_OVERSIZED(p));

Same comment applies to the longer assertion at the end of JS_ArenaGrow.

Nit: one blank line before one-or-more-line comments such as:

    /* Don't wrap! */

No need to parenthesize - against > in:

        if (gross > (a->limit - (jsuword) a))

and it seems like you could set quotachange's sign (two's complement interpretation of its unsigned bits) to avoid the repeated test of the above condition, and just use += to shrink and grow.

The JS_ArenaRelease JS_ASSERT((jsuword) mark == a->avail || !JS_ARENA_IS_OVERSIZED(a->base)) seems better written as JS_ASSERT_IF(JS_ARENA_IS_OVERSIZED(a->base), (jsuword) mark == a->avail).

JS_ARENA_IS_OVERSIZED is named as if its parameter is a JSArena *, but it takes arenaBase of course. Simplifying call sites that have to load a->base or b->base by factoring out JS_ARENA_BASE_IS_OVERSIZED would help.

Comment 92

[Brendan Eich [:brendan]]

> b->base by factoring out JS_ARENA_BASE_IS_OVERSIZED would help.

or maybe JS_ARENA_IS_OVERSIZED_BASE. Neither name is ideal, but JS_ARENA_IS_... is less right unless it takes arena, not arenaBase, per the proposed factoring.

/be

Comment 93

[Robin Bate Boerop]

(In reply to comment #91)
> I like your suggestion from bug 419131 comment 16:
> #ifndef USE_MALLOC_USABLE_SIZE
> # if defined(MOZ_MEMORY)
> size_t malloc_usable_size(void *);
> #  define USE_MALLOC_USABLE_SIZE 1
> # endif
> #endif
> But do extern the malloc_usable_size prototype for C++ compilation support and
> local style, and put JS_BEGIN/END_EXTERN_C around it for C++ too.

I've done something different than the above, mostly because I want to be able to use malloc_size() instead of malloc_usable_size() on Mac OS X, which does not provide the former.  Now, one should issue 'make' on Mac OS X with "-DMALLOC_USABLE_SIZE=malloc_size".  See my upcoming patch (I'm running tests, now.)  Please see my upcoming patch.

As to the rest of your suggested changes, I have implemented almost exactly what you suggested, with some exceptions:
 
> HEADER and BackPtr are conflicting tropes -- suggest Header for the name-part
> instead of BackPtr: JSArenaHeader.

Used JsArenaOversizeHeader.

> This:
> 
>     JS_ASSERT((nb <= pool->arenasize && !JS_ARENA_IS_OVERSIZED(p))
>               || (nb > pool->arenasize && JS_ARENA_IS_OVERSIZED(p)));
> 
> wants to be this (if it didn't, the || would still want to go at the end of the
> first line):
> 
>     JS_ASSERT((nb <= pool->arenasize) ^ JS_ARENA_IS_OVERSIZED(p));

Wow!  I've never used XOR in an assertion before.  Weird.  I will implement this change because it tightens up the code and is correct.  It does presents a problem to code fragility, though.

^ is bitwise, whereas || and && are logical.  This means that if JS_ARENA_IS_OVERSIZED were changed so that it returns something other than 1 when true, then the assertion logic would be incorrect.  (For example, if (nb <= arenasize) were true (1) and JS_ARENA_IS_OVERSIZED were also not-false (but equal to, say 2) then the xor expression would be true, where the logical expression would have been false.  I wonder if we should create a "logical xor" for just this sort of scenario....

> and it seems like you could set quotachange's sign (two's complement
> interpretation of its unsigned bits) to avoid the repeated test of the above
> condition, and just use += to shrink and grow.

We can't change the sign of 'quotachange' because it is compared to an unsigned value (*quotap), and would return the wrong result if 'quotachange' were negative.

Comment 94

[Robin Bate Boerop]

Attachment: Post Brendan-review patch

With this patch, malloc_usable_size will be used if MOZ_MEMORY is defined.  To enable the effects on a build in which MOZ_MEMORY is not defined, one must select a function for MALLOC_USABLE_SIZE.  So, on Linux:
   make XCFLAGS="-DMALLOC_USABLE_SIZE=malloc_usable_size"  (untested)
and on Mac OS X:
   make XCFLAGS="-DMALLOC_USABLE_SIZE=malloc_size"  (tested)

Comment 95

[Mike Beltzner [:beltzner, not reading bugmail]]

TM --> mozilla1.9, this won't block beta 5. If you disagree, please reset the TM to beta5 and explain why it needs to block beta.

Comment 96

[Mike Schroepfer]

Until we are clear on the benefit/problem this can't block - please re-nom if we have a good solution here.

Comment 97

[Robin Bate Boerop]

(In reply to comment #96)
> Until we are clear on the benefit/problem this can't block - please re-nom if
> we have a good solution here.

I agree about blocking.

This intention of this bug was to reduce the footprint primarily, and only secondarily to improve performance.  I will comment on the benefits and problems of the most recent patch's solution, soon.

Comment 98

[Robin Bate Boerop]

Attachment: With and without malloc_usable_size activated on an optimized build

The attached file is the comparison of two typical runs of SunSpider.  One run is minutely faster (.9%) than the other.

These data were from optimized shell SunSpider runs.

The slow run was for code which included the most recent patch, but with malloc_size deactivated.  The fast run was the same code, but with malloc_size activated.

The use of malloc_size always yields slightly better SunSpider results, in my tests.  It is important to note that this bug was primarily for the reduction of footprint, not primarily speed.  The footprint results are much more impressive (but not for SunSpider - for typical everyday browser use).

Now, it remains to compare a completely unmodified build against a build which includes the most recent patch.  We can expect that, in an optimized build, SunSpider might slow down, even with malloc_size activated.  This is because the patch introduces an "oversize check" on each allocation.  Since arena allocations are blazingly fast in the typical case, adding code path in that case may cause a slowdown.  I am testing this, now.

Comment 99

[Robin Bate Boerop]

This note is a reminder about why we're doing this.  The patch reduces the footprint of typical browser use.  That's what users want.

[The HWM is the point at which the run consumed the most memory (by the
allocations under scrutiny - the JSArena code).]

In the start-and-stop the browser test before malloc_size/malloc_usable_size:
total malloc requests: 6486
Most outstanding allocations: 86
High water mark for requested memory: 332818
High water mark for real memory: 374560

After malloc_size/malloc_usable_size:
total malloc requests: 5797
Most outstanding allocations: 66
High water mark for requested memory: 315168
High water mark for real memory: 315168

This means that malloc_size/malloc_usable_size reduces the real memory use by 19% in this test.  There were slightly *better* results w.r.t. footprint (posted in other bugs) for the "informal Gmail use" test.

Comment 100

[Mike Schroepfer]

(In reply to comment #99)
> This note is a reminder about why we're doing this.  The patch reduces the
> footprint of typical browser use.  That's what users want.
> 
> [The HWM is the point at which the run consumed the most memory (by the
> allocations under scrutiny - the JSArena code).]
> 
> In the start-and-stop the browser test before malloc_size/malloc_usable_size:
> total malloc requests: 6486
> Most outstanding allocations: 86
> High water mark for requested memory: 332818
> High water mark for real memory: 374560
> 
> After malloc_size/malloc_usable_size:
> total malloc requests: 5797
> Most outstanding allocations: 66
> High water mark for requested memory: 315168
> High water mark for real memory: 315168
> 
> This means that malloc_size/malloc_usable_size reduces the real memory use by
> 19% in this test.  There were slightly *better* results w.r.t. footprint
> (posted in other bugs) for the "informal Gmail use" test.
> 

If you get me an optimized build for vista I can run it through our normal memory test to try and verify these results.  Whatever build Brian Crowder handed me earlier did very poorly on that test.

Comment 101

[Robin Bate Boerop]

(In reply to comment #100)
> If you get me an optimized build for vista I can run it through our normal
> memory test to try and verify these results.  Whatever build Brian Crowder
> handed me earlier did very poorly on that test.

Brian probably gave you a build from bug 423815, which is expected to do very poorly w.r.t. memory footprint.

I have no way to get you an optimized build for Vista - I have no access to any Vista machine that I know of.

Comment 102

[Robin Bate Boerop]

(In reply to comment #98)
> Now, it remains to compare a completely unmodified build against a build which
> includes the most recent patch.  We can expect that, in an optimized build,
> SunSpider might slow down, even with malloc_size activated.  This is because
> the patch introduces an "oversize check" on each allocation.  Since arena
> allocations are blazingly fast in the typical case, adding code path in that
> case may cause a slowdown.  I am testing this, now.

Sadly, this is the case.  SunSpider, on an optimized build, is slowed by more than 1% by this patch, even with malloc_size activated.  This is unacceptable.

I am very interested in what Brian and Brendan would guess is the reason for the slowdown.

I suspect it is the additional check of pool->arenasize in JS_ARENA_ALLOCATE_COMMON.  I will try to remove that check, and see if it affects results.

Comment 103

[Brendan Eich [:brendan]]

Comment on attachment 310127 [details] [diff] [review]
Post Brendan-review patch

Sorry, I've been too busy -- crowder is the guy to do this.

/be

Comment 104

[Brian Crowder]

I'm happy with this as long as we don't regress on sunspider, and have already r+d a patch.  I'll give it another once-over in a bit.  Robin:  is this provably improved by the landing of 412866?

Comment 105

[Brendan Eich [:brendan]]

Uber-nit on the patch: SpiderMonkey code generally avoids double blank lines, just cuz one is enough and uniformity is good. FYI only.

/be

Comment 106

[Brian Crowder]

Robin:  What is your reply to comment 104?

Comment 107

[Robin Bate Boerop]

(In reply to comment #104)
> Robin:  is this provably improved by the landing of 412866?

I am confused by the question: has 412866 landed?

I have not checked, though I will check, whether the patch in 412866 will improve the performance degradation found here.

Comment 108

[Brian Crowder]

No it hasn't landed yet; but I remember some assertion that that patch helped this case.  If I'm wrong, let me know.

Comment 109

[Robin Bate Boerop]

(In reply to comment #102)
> Sadly, this is the case.  SunSpider, on an optimized build, is slowed by more
> than 1% by this patch, even with malloc_size activated.  This is unacceptable.

The reason for the slowdown is fixed by the patch for 412866.  When I applied the most recent patch for this bug on top of the most recent patch for bug 412866, I obtained SunSpider speeds equivalent to the patch for 412866 by itself.

In other words, once 412866 lands, this patch will not produce a slowdown on SunSpider.

Comment 110

[Robin Bate Boerop]

Attachment: Double-spaces removed, tested after 412866

This patch is identical to the prior one, but with double spaces removed, and with changes to help with CVS history.

Comment 111

[Robin Bate Boerop]

More performance data regarding SunSpider and the most recent patch:

(This refers to builds which include the patch for bug 412866.)

The extra code path in this patch slows SunSpider when malloc_usable_size is not available.  The slowdown is ~2%, and is significant.

However, when malloc_usable_size is available, there is no slowdown.  (For me, there is an insignificant speedup.)

So, when malloc_usable_size is available:
slowdown (due to code path) == speedup (due to malloc_usable_size)

However, the effect on footprint is excellent when malloc_usable_size is available, as discussed above.

This should be considered when deciding whether to approve the patch.  The  malloc_usable_size function is always available when MOZ_MEMORY (jemalloc, etc.) is available.

Comment 112

[Brian Crowder]

-        if ((guard) || _p > _a->limit - _nb || _nb > (pool)->arenasize)       \
+        if ((guard) || _p > _a->limit - _nb)                                  \

Why did you back off on this change?  (sorry for years-late review)

Comment 113

[Robin Bate Boerop]

(In reply to comment #112)
> -        if ((guard) || _p > _a->limit - _nb || _nb > (pool)->arenasize)
> +        if ((guard) || _p > _a->limit - _nb)
> Why did you back off on this change?  (sorry for years-late review)

Super good question.  The top version arranges for allocations that are larger than the pool's arenasize to result in "oversize" allocations, always.  The bottom version allows allocations larger than the pool's arenasize to sometimes be in non-oversize arenas.

To be clear, the bottom version is how the code is presently, without this patch.  (Brian, you must be diff'ing between patches, here?)  The top version is in a prior patch for this bug.

My logic is this: code inside of JS_ARENA_ALLOCATE_COMMON is on the very-performance-sensitive path.  Even a little extra code path is likely to have a performance penalty, here.  So, we want to avoid adding code to that macro.

The top line, above, added code which resulted in a memory reference to the pool structure.  From a performance perspective, removing that reference is good.  So, I did.

But, is it correct?  Well, it results in seriously different behaviour.  That's what should be noted.

*** The bottom version allows allocations which would previously have been in their own arenas to go into an arena with other allocations. ***

Previously, this could never happen because the arena could not be larger than the arenasize.  Now, however, the arena could be larger than the arenasize because of the malloc_usable_size stuff.  Now:
   arenasize <= malloc_usable_size(arena)

For example, if the arenasize is 1100, then we could now have an arena of size 2048.  So, an allocation of size 1200 would fit into the 2048-byte arena if there were only, say, 200 bytes already allocated in that arena when the 1200-byte allocation arrives.  Without this patch, the 1200 would never fit into an arena when the arenasize was 1100.

Previously (top version), I was trying to maintain the old behaviour.  But, then I thought, who cares?  I get to save a memory reference on the fast path.

The new behaviour only happens when the allocation size is "close" to the arenasize.  When the allocation size is *really* large, then there's no chance of it fitting into an arena with other allocations, and it will get its own arena, just as without the patch.

With my change, I considered the original reason for giving large allocations their own arenas.

Please let me know if I'm not being clear.  This change is syntactically small, but semantically large.

Comment 114

[Robin Bate Boerop]

I know that the discussion above is long, but the summary is this:  the patch is correct, but needs a final review.  It reduces memory use of the JavaScript engine in all cases, sometimes significantly (>15%).  When malloc_usable_size() is available, there is no slowdown.  malloc_usable_size() is provided by jemalloc, and therefore available whenever that library is used.  malloc_usable_size() is also available on some platforms, regardless of jemalloc's availability.

The patch may have bitrotted, but I will be very available to help answer questions about it between now and December.

Comment 115

[mmc]

Shame this has stalled...

Comment 116

[Brian Crowder]

I think the issues addressed here deserve a fresh analysis and a new bug, if any.

Comment 117

[Paul Biggar]

This bug is a disaster - so much work, such a benefit, and then lost :(

Robin, are you still around? I'm sorry to ask you to do more work on this when you've done so much already, but it would be wonderful to get the benefits.

igor: do you think the patch, or some updated variant thereof, is still useful, or has the memory system changed substantially?

Comment 118

[Igor Bukanov]

(In reply to comment #117)
> igor: do you think the patch, or some updated variant thereof, is still useful,
> or has the memory system changed substantially?

We have been using mmap-allocated memory for stack and other cases. So this patch is less useful then it was before.

Comment 119

[Robin Bate Boerop]

(In reply to comment #117)
> This bug is a disaster - so much work, such a benefit, and then lost :(

The root cause of the loss was lack of review. Reviewing the patches would have required lots of thinking and poking into some tricky code. Difficult.

> Robin, are you still around? I'm sorry to ask you to do more work on this when
> you've done so much already, but it would be wonderful to get the benefits.

I'm still around, but have little spare time to work on this. However, my employer (Collabora) is an open source software consultancy which could be contracted to do this and other similar work.

From Igor's comment, it sounds like the value of the patch is less than it once was. So, I am not motivated to do further work on this or other similar bugs without a contract.

Comment 120

[Nicholas Nethercote [inactive]]

(In reply to comment #117)
> This bug is a disaster - so much work, such a benefit, and then lost :(

I agree, I think it's a great idea.  I'm surprised the patch is so big, though.  I would have expected a patch of only a few lines.  The only function that calls malloc (well, OffTheBooks::malloc_ now) is JS_ArenaAllocate.  Maybe I'm missing some subtleties, but all the oversized arena stuff looks like overkill.

> We have been using mmap-allocated memory for stack and other cases. So this
> patch is less useful then it was before.

Are you saying that arenas are used less than they used to be?  The cx->tempPool, which is used to hold tokens while parsing, can still get pretty big, eg. 10s of MBs.  So I think this is still worthwhile.

Comment 121

[Ryan VanderMeulen [:RyanVM]]

njn, would it be worthwhile to file a new bug for comment #120?

Comment 122

[Robin Bate Boerop]

(In reply to comment #120)
> > This bug is a disaster - so much work, such a benefit, and then lost :(
> I agree, I think it's a great idea.  I'm surprised the patch is so big,
> though.  I would have expected a patch of only a few lines.  The only
> function that calls malloc (well, OffTheBooks::malloc_ now) is
> JS_ArenaAllocate.  Maybe I'm missing some subtleties, but all the oversized
> arena stuff looks like overkill.

The oversized arena stuff was definitely requisite in the patch. However, the code being patched probably looks different now than then (2008). So, a new patch might indeed be smaller.

It would be unwise to think that picking and choosing a strict subset of this patch is feasible. It would be very time consuming and very difficult to get right.

The patch is big in part because it uses assertions to document non-obvious assumptions that are made in the code. They are valuable. They represent many person-days of reverse engineering the code.

Further, the assertions were all code-reviewed by Brian Crowder and Brendan Eich. There is lots of brainpower encoded in this patch!  :)

Comment 123

[Nicholas Nethercote [inactive]]

Bug 675150 will ensure that arena chunks are powers-of-two, and so there should be no need for this because there should be no rounding up by the heap allocator (jemalloc or otherwise).

Comment 124

[Justin Dolske [:Dolske]]

Comment on attachment 312975 [details] [diff] [review]
Double-spaces removed, tested after 412866

Clearing review flag since this bug was closed as invalid. Please reopen or file a new bug if this patch was still somehow relevant.