Boris Zbarsky [:bzbarsky] Reporter
	Description • 18 years ago

The patch for bug 375270 broke GC_MARK_DEBUG builds.  I assume that that functionality has a replacement; is there a reason the GC_MARK_DEBUG code in nsXPConnect.cpp wasn't switched to it?  Not to mention the JS debugger and xpcshell, though those are not really needed for leak debugging?

See http://lxr.mozilla.org/seamonkey/search?string=GC_MARK_DEBUG for remaining consumers...  Some of these were just outputting extra data in GC_MARK_DEBUG mode to allow correlation of GC dumps with refcount logs, so we may just want a different define for them, but some of them use js_DumpGCHeap.

	Igor Bukanov Assignee
	Comment 2 • 18 years ago

There are 2 types of remaining GC_MARK_DEBUG users.

The first group is jsd and xpcshell that use it to define dumping functionality with GC_MARK_DEBUG defined. It is simple to fix them to define something like dumpHeap provided by jsshell now.

The second group is xpconnect code that tries in GC_MARK_DEBUG mode to generate better output for the price of decreased performance. It is less trivial to fix that without performance loss in a generic DEBUG fix but with the patch from bug 340212 I can replace AddNamedRoot/LockGCThings via using a custom hashtable in xpconnect that in fact can improve the performance while providing accurate debug output.

The timetable for this is 2-4 days after landing of bug 377884 and bug 340212.   

	Igor Bukanov Assignee
	Comment 3 • 18 years ago

(In reply to comment #0)
> See http://lxr.mozilla.org/seamonkey/search?string=GC_MARK_DEBUG for remaining
> consumers...  Some of these were just outputting extra data in GC_MARK_DEBUG
> mode to allow correlation of GC dumps with refcount logs, so we may just want a
> different define for them, but some of them use js_DumpGCHeap.

Ideally we should integrate JS objects into counting logs. This should be straightforward with after fixing bug 377884.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 4 • 18 years ago

Note that for the XPConnect code, I personally would be fine with having a separate define (other than just DEBUG) in which we pay the extra performance cost for better debugging information.  I don't think we want the "dump GC heap on shutdown" behavior in all DEBUG builds anyway, so we'd end up with a define for that, right?

	Igor Bukanov Assignee
	Comment 5 • 18 years ago

(In reply to comment #4)
> Note that for the XPConnect code, I personally would be fine with having a
> separate define (other than just DEBUG) in which we pay the extra performance
> cost for better debugging information.

The only performance cost for the tracing in DEBUG build is extra code bloat and populating of a struct with one 1 or 2 extra pointers. The extra code and the struct fields is used to provide an accurate debug info when one explicitly calls the tracing API. 

>  I don't think we want the "dump GC heap
> on shutdown" behavior in all DEBUG builds anyway, so we'd end up with a define
> for that, right?

Since the tracing code is available in any DEBUG build, one can have an environment variable or other configurable at runtime parameter to call dumping on shutdown.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 6 • 18 years ago

Sure.  That would be great too.  ;)

	Igor Bukanov Assignee
	Comment 7 • 18 years ago

(In reply to comment #0 by Boris Zbarsky)
> The patch for bug 375270 broke GC_MARK_DEBUG builds.

How have you used GC_MARK_DEBUG before? In particular, have you relied on GC() method in JS debugger that set js_DumpGCHeap with GC_MARK_DEBUG defined or have you used to set js_DumpGCHeap explicitly? 

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 8 • 18 years ago

> How have you used GC_MARK_DEBUG before?

By just defining it.  When it was defined, XPConnect dumped the heap at shutdown.

	Igor Bukanov Assignee
	Comment 9 • 18 years ago

The patch replaces GC_MARK_DEBUG via DEBUG code that dumps the heap on shutdown when XPC_SHUTDOWN_HEAP_DUMP is defined. The patch uses the value of the variable as a file name with a special treatment of stdout for platforms that lacks /dev/stdout or similar.

	Igor Bukanov Assignee
	Comment 10 • 18 years ago

Note that until bug 377895 is fixed it still make sense to define GC_MARK_DEBUG as it would provide more informative dump.

	Brendan Eich [:brendan]
	Comment 11 • 18 years ago

Comment on attachment 262802 [details] [diff] [review]
Dump via environment

Looks good, it'd be great to have bz testify it works for him.

/be

	Igor Bukanov Assignee
	Comment 12 • 18 years ago

(In reply to comment #11)
> (From update of attachment 262802 [details] [diff] [review])
> Looks good, it'd be great to have bz testify it works for him.

To really verify the patch one need to apply patches from bug 377751 and bug   	340212. Otherwise the dump would not see any JSObject* stored in XPConnect objects.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 13 • 18 years ago

I get a dump on shutdown, which is a start!  Well, that and compiling.  ;)

	Igor Bukanov Assignee
	Comment 14 • 18 years ago

(In reply to comment #13)
> I get a dump on shutdown, which is a start!  Well, that and compiling.  ;)
> 
Good. Now it is a matter of getting sr=something.

	Johnny Stenback (:jst)
	Comment 15 • 18 years ago

Comment on attachment 262802 [details] [diff] [review]
Dump via environment

sr=jst

	Igor Bukanov Assignee
	Comment 16 • 18 years ago

I committed the patch from comment 9 to the trunk:

Checking in xpconnect/src/nsXPConnect.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/nsXPConnect.cpp,v  <--  nsXPConnect.cpp
new revision: 1.105; previous revision: 1.104
done

	Igor Bukanov Assignee
	Comment 17 • 18 years ago

I am changing the dependency graph for the bug. Although from a functionality point of view it was indeed a regression from bug 375270, the committed patch effectively provided a new feature - heap dumping in a generic DEBUG build. So I making this bug to block the tracing meta bug 378742 instead.