427858 - PLArena code enhancements, including memory management logging

Status: RESOLVED WONTFIX

(NSPR :: NSPR, enhancement, P4)

Description

[Robin Bate Boerop]

This bug is for enhancements to PLArena code.  I propose to add logging code which will help diagnose memory management problems.

Comment 1

[Robin Bate Boerop]

Attachment: v1

This patch to PLArena code:
(1) gives an http link to the cited paper
(2) adds logging code to help diagnose memory problems
(3) deletes unused code
(4) clarifies the deprecated status of PL_CompactArenaPool
(5) uses PR_DELETE in PL_ARENA_DESTROY instead of 'free', so that PLArena code won't necessarily be broken w.r.t the "zone allocator" code

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

I read through this patch because of the claim in comment 1 concerning 
the "zone allocator".  I have some observations and questions.

> (1) gives an http link to the cited paper
> (3) deletes unused code
> (4) clarifies the deprecated status of PL_CompactArenaPool

The above 3 things all seem good (thanks for the link to the paper).

> (5) uses PR_DELETE in PL_ARENA_DESTROY instead of 'free', so that PLArena 
> code won't necessarily be broken w.r.t the "zone allocator" code

<http://mxr.mozilla.org/mozilla/search?string=P%5BLR%5D_ARENA_DESTROY&regexp=on&case=on&tree=mozilla> 
tells us that the macro PL_ARENA_DESTROY is dead code.  
How about deleting it instead of fixing it?

> (2) adds logging code to help diagnose memory problems

several thoughts about that:

1) the new macros use stdio for logging, logging to stdout.  That won't work
very satisfactorily, or at all, in lots of environments, including those:
- that have no stdio and no stdout, like WinCE, on which NSPR now builds and
works
- that close stdout right away (e.g. Mozilla browsers on Windows)
- those that write other output to stdout

This probably should use NSPR's own built-in logging features, which do not
depend on stdio, and work even on WinCE and in programs that write to stdout.

2) who will use this, and how?
- How does PL_ARENA_MEM_LOGGING ever get defined?  
  By what means would a developer ever enable it?  
- Is there any script or other tool available which post-processes the log 
output?  

Comment 3

[Robin Bate Boerop]

(In reply to comment #2)
> > (5) uses PR_DELETE in PL_ARENA_DESTROY instead of 'free', so that PLArena 
> > code won't necessarily be broken w.r.t the "zone allocator" code
> <http://mxr.mozilla.org/mozilla/search?string=P%5BLR%5D_ARENA_DESTROY&regexp=on&case=on&tree=mozilla> 
> tells us that the macro PL_ARENA_DESTROY is dead code.  
> How about deleting it instead of fixing it?

My understanding is that NSPR must continue to support that macro because it may be used by users of NSPR outside of mozilla.  We can't know who is using that macro.  Am I mistaken?

> > (2) adds logging code to help diagnose memory problems
> several thoughts about that:
> 1) the new macros use stdio for logging, logging to stdout.  That won't work
> very satisfactorily, or at all, in lots of environments, including those:
> - that have no stdio and no stdout, like WinCE, on which NSPR now builds and
> works
> - that close stdout right away (e.g. Mozilla browsers on Windows)
> - those that write other output to stdout
> This probably should use NSPR's own built-in logging features, which do not
> depend on stdio, and work even on WinCE and in programs that write to stdout.

That seems like a good idea.  I'll have to check to make sure that the logging code does not recursively call the arena code....

> 2) who will use this, and how?
> - How does PL_ARENA_MEM_LOGGING ever get defined?  
>   By what means would a developer ever enable it?  
> - Is there any script or other tool available which post-processes the log 
> output?  

The PL_ARENA_MEM_LOGGING is intended to be defined by the hacker interested in the NSPR memory behaviour.  He/she would normally either hack in a #define line, or define the macro is the build environment.  I compile NSPR as part of my mozilla/Firefox code from my obj-dir with a line like:
   make XCFLAGS="-DPL_ARENA_MEM_LOGGING" -C nsprpub

I have a script to process the output, but I am not providing it as part of this bug.  I intend for developers to write their own scripts to process the output, since it is straightforward to do.  The hard part is to ensure that all of the useful memory management activity is logged and that none is missed.  That's what the most recent patch does - it encodes that knowledge.

I'll look into using NSPR's logging facility....

Comment 4

[Nicholas Nethercote [inactive]]

I don't think we need this.

Comment 5

[Wan-Teh Chang]

Attachment: Code cleanup changes from patch v1

I salvaged the code cleanup changes from patch v1 (attachment 314481 [details] [diff] [review]).

Checked in on the NSPR trunk (NSPR 4.8.9).

Checking in plarenas.h;
/cvsroot/mozilla/nsprpub/lib/ds/plarenas.h,v  <--  plarenas.h
new revision: 3.8; previous revision: 3.7
done