This is an enhancement which will aid in the diagnosis of memory management problems in Spidermonkey. Add code which logs memory management operations like malloc, free, arena pool creation, etc. The code should require a preprocessor macro to be defined, to be active: JS_ARENA_MEM_LOGGING.

Comment on attachment 311812 [details] [diff] [review] v1 - proposed patch >-#define COUNT(pool,what) (pool)->stats.what++ >+# define COUNT(pool,what) (pool)->stats.what++ > #else >-#define COUNT(pool,what) /* nothing */ >+# define COUNT(pool,what) /* nothing */ > #endif ((void)0) instead of /* nothing */ here, please. >+#ifdef JS_ARENA_MEM_LOGGING >+# define LOG_POOL_CTOR(pool, name, size, mask) \ >+ printf("mm poolCtor %p \"%s\" %u %u\n", \ >+ (pool), (name), (size), (mask)); \ >+ fflush(stdout) >+# define LOG_POOL_DTOR(pool, size, mask) \ >+ printf("mm poolDtor %p %u %u\n", \ >+ (pool), (size), (mask)); \ >+ fflush(stdout) >+# define LOG_POOL_ASSOC(pool, arena) \ >+ printf("mm poolAssoc %p %p\n", \ >+ (pool), (arena)); \ >+ fflush(stdout); >+#else >+# define LOG_POOL_CTOR(pool, name, size, mask) >+# define LOG_POOL_DTOR(pool, size, mask) >+# define LOG_POOL_ASSOC(pool, arena) >+#endif >+ Don't indent #define as "# define" at the top level. Look at other #if #define #endif patterns in headers for an example. Looks like all your printfs here can reside happily on one line? >+#ifdef JS_ARENA_MEM_LOGGING >+# include <stdio.h> >+#endif Again, no indentation for the #include here. >-#ifdef JS_ARENAMETER >+#if defined JS_ARENAMETER || defined JS_ARENA_MEM_LOGGING Oddly, we're not very consistent about whether we should do #if defined(JS_ARENAMETER) || defined(JS_ARENA_MEM_LOGGING) instead, but we do seem to favor having the parenthesis, based on my quick grepping. >-#ifdef JS_ARENAMETER >+#if defined JS_ARENAMETER || defined JS_ARENA_MEM_LOGGING > # define JS_NEW_PRINTER(cx, name, fun, indent, pretty) \ > js_NewPrinter(cx, name, fun, indent, pretty) > #else > # define JS_NEW_PRINTER(cx, name, fun, indent, pretty) \ > js_NewPrinter(cx, fun, indent, pretty) > #endif Why is this change necessary?

(In reply to comment #2) > (From update of attachment 311812 [details] [diff] [review]) > >-#define COUNT(pool,what) (pool)->stats.what++ > >+# define COUNT(pool,what) (pool)->stats.what++ > > #else > >-#define COUNT(pool,what) /* nothing */ > >+# define COUNT(pool,what) /* nothing */ > > #endif > ((void)0) instead of /* nothing */ here, please. Why? I'm not creating code, here, just adding indentation to make it consistent with my nearby new code. The "nothing" comment was already there. > Don't indent #define as "# define" at the top level. Look at other > #if > #define > #endif > patterns in headers for an example. I did. I randomly chose jsgc.c, and also looked elsewhere, to guide my style. jsgc.c definitely does it as I did. Please let me know which source file to look at to find "correct" Spidermonkey style, and I'll follow it. > Looks like all your printfs here can reside happily on one line? Some can, many cannot. I chose the multi-line style to help readers match the arguments following the string with the %-escapes in the string, and for consistency between the macros, some of which have printf arguments which do not fit on one line. Please let me know if you want the shorter printf's on one line where possible. > >-#ifdef JS_ARENAMETER > >+#if defined JS_ARENAMETER || defined JS_ARENA_MEM_LOGGING > > # define JS_NEW_PRINTER(cx, name, fun, indent, pretty) \ > > js_NewPrinter(cx, name, fun, indent, pretty) > > #else > > # define JS_NEW_PRINTER(cx, name, fun, indent, pretty) \ > > js_NewPrinter(cx, fun, indent, pretty) > > #endif > > Why is this change necessary? To make the "name" argument available for the JS_ARENA_MEM_LOGGING code. Otherwise, it was available only when JS_ARENAMETER was defined.

(In reply to comment #3) > > Don't indent #define as "# define" at the top level. Look at other > > #if > > #define > > #endif > > patterns in headers for an example. > I did. I randomly chose jsgc.c, and also looked elsewhere, to guide my style. > jsgc.c definitely does it as I did. Please let me know which source file to > look at to find "correct" Spidermonkey style, and I'll follow it. Sorry to cause so many style-related delays. I really am trying to get my style to match. :)

> > ((void)0) instead of /* nothing */ here, please. > > Why? I'm not creating code, here, just adding indentation to make it > consistent with my nearby new code. The "nothing" comment was already there. Yeah, it's too bad we're not consistent on this. No big deal if you choose not to do so here. > > Don't indent #define as "# define" at the top level. Look at other > > patterns in headers for an example. > > I did. I randomly chose jsgc.c, and also looked elsewhere, to guide my style. > jsgc.c definitely does it as I did. Please let me know which source file to > look at to find "correct" Spidermonkey style, and I'll follow it. Again, looks like we're not terribly consistent on this. No big deal. > > Looks like all your printfs here can reside happily on one line? > > Some can, many cannot. +#ifdef JS_ARENA_MEM_LOGGING +# define LOG_POOL_CTOR(pool, name, size, mask) \ + printf("mm poolCtor %p \"%s\" %u %u\n", (pool), (name), (size), (mask)); \ + fflush(stdout) +# define LOG_POOL_DTOR(pool, size, mask) \ + printf("mm poolDtor %p %u %u\n", (pool), (size), (mask)); \ + fflush(stdout) +# define LOG_POOL_ASSOC(pool, arena) \ + printf("mm poolAssoc %p %p\n", (pool), (arena)); \ + fflush(stdout); +#else Looks like they all can. > > Why is this change necessary? > > To make the "name" argument available for the JS_ARENA_MEM_LOGGING code. > Otherwise, it was available only when JS_ARENAMETER was defined. > Where does your logging code use JS_NEW_PRINTER?

(In reply to comment #5) > > > Looks like all your printfs here can reside happily on one line? > > Some can, many cannot. > +#ifdef JS_ARENA_MEM_LOGGING > +# define LOG_POOL_CTOR(pool, name, size, mask) > \ > + printf("mm poolCtor %p \"%s\" %u %u\n", (pool), (name), (size), (mask)); > \ > + fflush(stdout) > +# define LOG_POOL_DTOR(pool, size, mask) > \ > + printf("mm poolDtor %p %u %u\n", (pool), (size), (mask)); > \ > + fflush(stdout) > +# define LOG_POOL_ASSOC(pool, arena) > \ > + printf("mm poolAssoc %p %p\n", (pool), (arena)); > \ > + fflush(stdout); > +#else > Looks like they all can. Sorry, you're right. I thought you were talking about the ones in the .h file, not the .c file. I'll condense. > > > Why is this change necessary? > > To make the "name" argument available for the JS_ARENA_MEM_LOGGING code. > > Otherwise, it was available only when JS_ARENAMETER was defined. > Where does your logging code use JS_NEW_PRINTER? JS_NEW_PRINTER calls JS_INIT_ARENA_POOL with the 'name' argument passed to it.

It would be nice to fix pre-existing /* nothing */ macro bodies to be ((void)0) assuming expression bodies are the right thing, since that avoids warnings about empty statements from some compilers when the macro is the unbraced consequent of an if (condition). /be

This patch adds types to the macros (with ((void) 0) where appropriate), and makes the indentation consistent throughout jsarena.(c|h).

(In reply to comment #8) > Created an attachment (id=311854) [details] The patch also changes jsutil.h and jsutil.c so that one can compile with JS_ARENAMETER defined when DEBUG is not.

Comment on attachment 311854 [details] [diff] [review] v2 - after comments from Brian Crowder and Brendan Eich >-#define COUNT(pool,what) (pool)->stats.what++ >+# define COUNT(pool, what) ((void) ((pool)->stats.what++)) New cast here seems unnecessary? >-#define JS_COUNT_ARENA(pool,op) ((pool)->stats.narenas op) >+# define JS_COUNT_ARENA(pool, op) ((void) ((pool)->stats.narenas op)) Weird cast again? >-double >-JS_MeanAndStdDev(uint32 num, double sum, double sqsum, double *sigma) Can you put this function back where it was to minimize the patch-size? Do you have a compelling reason for moving it? So, I'm very sorry if I gave the impression that I wanted you to fix lots more indentation in the preprocessor sections (and elsewhere?) of these files. I'm not sure if it's a good idea or not -- we struggle to balance between tidiness and minimal patches for a variety of reasons (which is why we tend to be so Byzantine about style in initial reviews). Mainly, big sweeping indentation fixes make CVS archaeology at least a _little_ harder. Consistency is good, but aim first for consistency in the code you're adding and we can worry about consistency everywhere else when those pieces of code get touched.

(In reply to comment #10) > (From update of attachment 311854 [details] [diff] [review]) > >-#define COUNT(pool,what) (pool)->stats.what++ > >+# define COUNT(pool, what) ((void) ((pool)->stats.what++)) > New cast here seems unnecessary? Brendan asked that I change the "/* nothing */" to "(void) 0". The new cast is to make sure that the type of the macro is the same regardless of whether JS_ARENAMETER is defined. (The type of the above macro was previously "typeof what", for example, not "void".) > >-#define JS_COUNT_ARENA(pool,op) ((pool)->stats.narenas op) > >+# define JS_COUNT_ARENA(pool, op) ((void) ((pool)->stats.narenas op)) > Weird cast again? Same reason as above. > >-double > >-JS_MeanAndStdDev(uint32 num, double sum, double sqsum, double *sigma) > Can you put this function back where it was to minimize the patch-size? Do you > have a compelling reason for moving it? The reason for moving it is that if JS_ARENAMETER is defined when JS_BASIC_STATS is not, the above function is called but never defined. So, there's a compilation warning and then a link error. I encounter this problem whenever I try to use JS_ARENAMETER. Either JS_ARENAMATER should imply JS_BASIC_STATS, or the above change should be made. I chose to add the change to this bug because it has the same purpose - "make Robin's analyses easier". > So, I'm very sorry if I gave the impression that I wanted you to fix lots more > indentation in the preprocessor sections (and elsewhere?) of these files. I'm > not sure if it's a good idea or not -- we struggle to balance between tidiness > and minimal patches for a variety of reasons (which is why we tend to be so > Byzantine about style in initial reviews). Mainly, big sweeping indentation > fixes make CVS archaeology at least a _little_ harder. Consistency is good, > but aim first for consistency in the code you're adding and we can worry about > consistency everywhere else when those pieces of code get touched. Some of my changes are merely changes in spacing, but others (like void-casting) have substance. I understand there there is a tradeoff between code style consistency and CVS archeology. In other bugs, either you or Brendan (or both) suggested that I change spacing of code that was _near_ code that I had changed, to remain consistent with it. So, I have trouble deciding what to change and what to leave alone. Please consider my above comments, let me know precisely what changes to make. Meanwhile, I'll submit another patch which preserves more CVS archeology.

(In reply to comment #11) > (In reply to comment #10) > Brendan asked that I change the "/* nothing */" to "(void) 0". Yeah, that's enough to avoid useless warnings. > The new cast is > to make sure that the type of the macro is the same regardless of whether > JS_ARENAMETER is defined. (The type of the above macro was previously "typeof > what", for example, not "void".) This is good for purity but not important to avoid warnings, and a slight ding to readability -- the cast to throw away the value is not important to the macro's meaning. I may be guilty over enlarging past patches, but given the end-game state of the release it's probably time to dial it down. /be

Changes made according to my interpretation of Brendan's most recent comment.

Comment on attachment 312073 [details] [diff] [review] v4 - no type consistency on macros, fewer spacing changes >+#ifdef JS_ARENA_MEM_LOGGING >+# define LOG_POOL_CTOR(pool, name, size, mask) \ >+ printf("mm poolCtor %p \"%s\" %u %u\n", (pool), (name), (size), (mask)); \ >+ ((void) fflush(stdout)) Use JS_BEGIN_MACRO and JS_END_MACRO on these, and don't do the cast for fflush. >+# define JS_ARENA_MEM_LOG_DISASSOC(pool, arena) \ >+ printf("mm poolDisassoc %p %p\n", (pool), (arena)); \ >+ ((void) fflush(stdout)) Same here. Sorry I didn't notice these before; otherwise looks good. One more spin.

review ping

Comment on attachment 313027 [details] [diff] [review] v5 - Use JS_BEGIN/END_MACRO Let's not try to push this for 1.9, though. We'll hit in in the next release.

I have not tested this patch as much as v5, but it is not likely to be incorrect because the memory analysis tool that I wrote would find unpaired mallocs/frees. It didn't; the memory analysis tool ran smoothly. I have only tested on Mac OS (defining -DMALLOC_GOOD_SIZE=malloc_good_size and -DJS_ARENA_MEM_LOGGING=1). This patch should be compiled on other platforms to test the logic for defining the MALLOC_GOOD_SIZE macro.

Comments for v6 apply to this patch, also. (I made a small change to format specifiers in this patch.)

These bugs are all part of a search I made for js bugs that are getting lost in transit: http://tinyurl.com/jsDeadEndBugs They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.

Nochum, this bug was not review+'d.

I apologize, the search isn't perfect I guess.

This patch has probably rotted, and I am probably no longer a good reviewer for it, sorry for letting it languish. Absent action here, this bug should be closed in a week or so.

This bug is at the leaf of a dependency tree for more important bugs. I will update the patch for it, and for the dependent bugs, if someone will guarantee me a review. Reviewer suggestions, anyone? Brian?

Look in the IRC channel #jsapi

I'm making an executive decision: we don't need this.

Comment on attachment 339803 [details] [diff] [review] v7 - Tiny change to v6 Clearing review flag since this was wontfix'd.