Closed Bug 390486 Opened 17 years ago Closed 17 years ago

ActionMonkey: Reimplement AddThingToUnscannedBag(), ScanDelayedChildren() without referring to page-level JSGC data structures

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
The tracing API currently relies on detailed knowledge of the JSGC allocator data structures to do its thing, particularly in AddThingToUnscannedBag() and ScanDelayedChildren().  This makes it impossible to switch to MMgc.

The attached patch replaces these two functions with a much simpler implementation that keeps an explicit stack of unscanned objects.
Attachment #274805 - Flags: review?(igor)
Comment on attachment 274805 [details] [diff] [review]
v1

I have to minus this as this uses dynamic memory allocation for a queue which can fail.

Given how hard it was to switch JSGC to O(1) in space implementation as an after thought, I think even for stage 0 the dynamic allocation is no go. 

But remedy is simple: just add yet another field to GCThing to use as a link for the stack. Later that can be optimized in exactly the same way as vtable allocation can be optimized. Alternatively better integration with Tamarin should allow to use its space-bounded marking.
Attachment #274805 - Flags: review?(igor) → review-
Thanks for looking at this, Igor.

Can you clarify your last statement?  I was trying to mimic what MMgc does in Mark()/MarkItem(), and I didn't see anything that indicated its use of GCStack would be space-bounded.
(In reply to comment #2)
> Thanks for looking at this, Igor.
> 
> Can you clarify your last statement?  I was trying to mimic what MMgc does in
> Mark()/MarkItem(), and I didn't see anything that indicated its use of GCStack
> would be space-bounded.

Then my memory served me wrong. In any case, I suggest for now add a link field to GCThing and optimize it later.

Attached patch v2Splinter Review
This version avoids allocating memory at GC time, using a linked list as suggested.

This new code to JS_CallTracer() might seem confusing:

+    if (!gc_type_has_vtable[*flagp & GCF_TYPEMASK])
+        goto out;

This is not an optimization; it's necessary because the v2 implementation of AddThingToUnscannedBag() only works on JSFinalizedGCObjects.

It's safe to bail out there because tracing the children of a JSFunction or a jsdouble is a no-op anyway.

Maybe I should rename gc_type_has_vtable to gc_type_is_JSFinalizedGCObject.
Attachment #274805 - Attachment is obsolete: true
Attachment #275007 - Flags: review?(igor)
Comment on attachment 275007 [details] [diff] [review]
v2

Thanks, now the stage for future optimizations is clear.
Attachment #275007 - Flags: review?(igor) → review+
(In reply to comment #4)
> Maybe I should rename gc_type_has_vtable to gc_type_is_JSFinalizedGCObject.

Or just gc_type_has_finalizer or gc_type_is_finalized.

/be
Pushed to actionmonkey, changeset 1e1dfd9265b5.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.