Closed Bug 424287 Opened 12 years ago Closed 9 years ago

Add logging to memory management code in jsarena.(h|c)

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED WONTFIX

People

(Reporter: moz, Assigned: moz)

References

Details

Attachments

(1 file, 6 obsolete files)

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.
Priority: -- → P3
Attached patch v1 - proposed patch (obsolete) — Splinter Review
Attachment #311812 - Flags: review?(crowder)
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?
Attachment #311812 - Flags: review?(crowder) → review-
(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).
Attachment #311812 - Attachment is obsolete: true
Attachment #311854 - Flags: review?(crowder)
(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.
Attachment #311854 - Flags: review?(crowder) → review-
(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.

Attachment #311854 - Attachment is obsolete: true
Attachment #311964 - Flags: review?(crowder)
(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.
Attachment #311964 - Attachment is obsolete: true
Attachment #312073 - Flags: review?(crowder)
Attachment #311964 - Flags: review?(crowder)
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.
Attachment #312073 - Flags: review?(crowder) → review-
Attached patch v5 - Use JS_BEGIN/END_MACRO (obsolete) — Splinter Review
Attachment #312073 - Attachment is obsolete: true
Attachment #313027 - Flags: review?(crowder)
Blocks: 420476
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.
Attachment #313027 - Flags: review?(crowder) → review+
Attached patch v6 - unbitrotted v5 (obsolete) — Splinter Review
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.
Attachment #313027 - Attachment is obsolete: true
Attachment #339795 - Flags: review?(crowder)
Comments for v6 apply to this patch, also.  (I made a small change to format specifiers in this patch.)
Attachment #339795 - Attachment is obsolete: true
Attachment #339803 - Flags: review?(crowder)
Attachment #339795 - Flags: review?(crowder)
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.
Attachment #339803 - Flags: review?(crowderbt) → review?
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.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Comment on attachment 339803 [details] [diff] [review]
v7 - Tiny change to v6

Clearing review flag since this was wontfix'd.
Attachment #339803 - Flags: review?
You need to log in before you can comment on or make changes to this bug.