378255 - Patch for bug 375270 breaks GC_MARK_DEBUG builds

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Igor Bukanov]

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.   

Comment 3

[Igor Bukanov]

(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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Igor Bukanov]

(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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sure.  That would be great too.  ;)

Comment 7

[Igor Bukanov]

(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? 

Comment 8

[Boris Zbarsky [:bzbarsky]]

> How have you used GC_MARK_DEBUG before?

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

Comment 9

[Igor Bukanov]

Attachment: Dump via environment

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.

Comment 10

[Igor Bukanov]

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

Comment 11

[Brendan Eich [:brendan]]

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

Comment 12

[Igor Bukanov]

(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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Igor Bukanov]

(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.

Comment 15

[Johnny Stenback (:jst)]

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

sr=jst

Comment 16

[Igor Bukanov]

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

Comment 17

[Igor Bukanov]

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.