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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: igor, Assigned: igor)
Details
Attachments
(1 file, 3 obsolete files)
33.75 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•18 years ago
|
||
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 | ||
Updated•18 years ago
|
Assignee: general → igor.bukanov
Assignee | ||
Updated•18 years ago
|
Status: NEW → ASSIGNED
Updated•18 years ago
|
Flags: in-testsuite-
Assignee | ||
Comment 2•18 years ago
|
||
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 3•18 years ago
|
||
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+
Assignee | ||
Comment 4•18 years ago
|
||
(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.
Assignee | ||
Comment 5•18 years ago
|
||
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
Assignee | ||
Comment 6•18 years ago
|
||
This is a patch to commit synchronized with CVS head
Attachment #215943 -
Attachment is obsolete: true
Assignee | ||
Comment 7•18 years ago
|
||
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.
Description
•