Closed Bug 330692 Opened 18 years ago Closed 18 years ago

GC_MARK_DEBUG: using GC_MARK, not JS_MarkGCThing internally

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

Details

Attachments

(1 file, 3 obsolete files)

In quite a few places SpiderMonkey calls JS_MarkGCThing API and not GC_MARK macro to mark GC things. This forces to use NULL or other placeholder values to supply 2 GC_MARK_DEBUG-only arguments even when GC_MARK_DEBUG is not defined.

Thus the idea is to use GC_MARK consistently so all debug-only code would be removed when compiling without GC_MARK_DEBUG defined.

Another optimization is to use a field in JSContext to record current debug node instead of using extra debug arg which currently is used in many internal functions even if GC_MARK_DEBUG is not defined.
Attached patch Implementation (obsolete) — Splinter Review
The patch shrinks optimized build of js shell on Linux with GCC 4.0 from  502992 to 498992 or by 0.8%.

The patch assumes that the patch from bug 324278 is applied.
Assignee: general → igor.bukanov
Status: NEW → ASSIGNED
Flags: in-testsuite-
Attached patch Implementation v2 (obsolete) — Splinter Review
This version besides synchronizing with changes from bug 324278 adds GC_MARK_DEBUG-only js_MarkNamedGCThing and defines GC_MARK when GC_MARK_DEBUG defined as:

# define GC_MARK(cx, thing, name) js_MarkNamedGCThing(cx, thing, name)

In this way all code to trace/dump GC things are gathered in one place and GCMarkNode struct needs to be defined only in jsgc.c.
Attachment #215279 - Attachment is obsolete: true
Attachment #215910 - Flags: review?(brendan)
Comment on attachment 215910 [details] [diff] [review]
Implementation v2

>+++ src/jscntxt.h	2006-03-22 18:11:00.000000000 +0100
>@@ -550,6 +550,11 @@
> 
>     /* Flag to indicate that we run inside gcCallback(cx, JSGC_MARK_END). */
>     JSBool              insideGCMarkCallback;

I forgot to ask when reviewing 324278: why use this flag in js_MarkGCThing, risking O(n^2) rescanning of delayed children, instead of testing it once after the JSGC_MARK_END callback returns and rescanning there?

Any way to pack this JSBool as a JSPackedBool with other JSPackedBool and uint8 members?

r=me with these addressed.

Good cleanup, should have been that way from the start.  Thanks,

/be
Attachment #215910 - Flags: review?(brendan) → review+
(In reply to comment #3)
> >     /* Flag to indicate that we run inside gcCallback(cx, JSGC_MARK_END). */
> >     JSBool              insideGCMarkCallback;
> 
> I forgot to ask when reviewing 324278: why use this flag in js_MarkGCThing,
> risking O(n^2) rescanning of delayed children, instead of testing it once after
> the JSGC_MARK_END callback returns and rescanning there?

I did this in one of the versions of the patch. Unfortunately it does not work since xpconnect callback assumes that when it is done with marking its own things, it can start the finalization of non-marked things. And if the mark would come from the delayed list, the callback would free live things.

Not to break API compatibility I added "insideGCMarkCallback" flag to ensure that after the last mark call from the callback, the finalization can start.

Note there is no danger of O(n^2) behaviour since running time of ScanDelayedChildren is proporcional to the number of things in the unscanned bag and that can not exceed the total number of GC things. Thus the only penalties of extra calls to ScanDelayedChildren are extra initialization code for arena scanning and sligtly deeper C stack as it contains the callback itself.
Attached patch Implementation v3 (obsolete) — Splinter Review
This version uses JSPackedBool for insideGCMarkCallback. Since there are already 4 packed boolean or uint8 values, this has benefits only with 64 bit CPU.
Attachment #215910 - Attachment is obsolete: true
This is a patch to commit synchronized with CVS head
Attachment #215943 - Attachment is obsolete: true
I committed the patch "v4" from attachment 216004 [details] [diff] [review].
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: