Closed Bug 390844 Opened 17 years ago Closed 17 years ago

ActionMonkey: MMgc JSFunction finalization (resolve merge conflict w/ mozilla-central)

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch stabbity v1 (obsolete) — Splinter Review
Convert GCX_PRIVATE to GCX_FUNCTION. Add a js_TraceFunction for tracing children. JSFinalizedGCThing-ize JSFunction.

The last thing could potentially be split off and first two made for trunk.
Be great to get changes like this, which IIRC are prefigured by another bug and patch, into cvs.mozilla.org first.

/be
Merging with mozilla-central (cvs-trunk-mirror) results in some conflicts for actionmonkey that can be resolved fairly easily minus the MMgc JSFunction finalization. mozilla-central contains changes for switching GCX_PRIVATE to GCX_FUNCTION (bug 389757) and various other fixes for fast native calls and string gcflags removal.

I'll post a pseudo-patch of the merge conflict resolution with the major change being to have JSFunction finalize with a destructor.
Summary: ActionMonkey: GCX_FUNCTION, Trace JSFunction, MMgc JSFuncion finalization → ActionMonkey: MMgc JSFuncion finalization (resolve merge conflict w/ mozilla-central)
Attached patch trimmed "patch" (obsolete) — Splinter Review
jsgc.cpp: gc_type_is_finalized (bug 389420) vs gc_finalizers (bug 389757)
resolve: change JSFunction to MMgc style finalization
-    JS_FALSE,         /* GCX_PRIVATE */
+    JS_TRUE,          /* GCX_FUNCTION */

jsstr.cpp: no GCF_LOCK (bug 389832) vs remove gcflags (bug 389880)
resolve: use ATOM_TO_STRING(rt->atomState.emptyAtom);

jsstr.cpp: JSNativeString new (bug 389420) vs remove gcflags (bug 389880)
resolve: |new| w/o gcflags ORed in

jsstr.cpp: ~JSNativeString (bug 389420) vs Fast native call optimizations (bug 385393)
resolve: check !IN_UNIT_STRING_SPACE_RT(rt, chars)
Attachment #275153 - Attachment is obsolete: true
Actually.. there's the new JSString **unitStrings array that has elements initialized with GCF_LOCK, but since we don't have that flag anymore, we'll need to iterate through the unitStrings array, if necessary, from js_TraceRuntime and TEMP_TRACE(STRING, rt->unitStrings[i])?

Jason: How temporary is TEMP_TRACE if we're skipping MMgc's conservative object scanning to use our own CustomMark()?
You're right, it's not temporary.  The comment above TEMP_TRACE should be removed.
Summary: ActionMonkey: MMgc JSFuncion finalization (resolve merge conflict w/ mozilla-central) → ActionMonkey: MMgc JSFunction finalization (resolve merge conflict w/ mozilla-central)
For the push, I'll be pushing both the merge and this patch.
Attachment #275519 - Attachment is obsolete: true
Attachment #275734 - Flags: review?(jorendorff)
Comment on attachment 275734 [details] [diff] [review]
fix conflicts, ~JSFunction, remove TEMP_TRACE, manual locks v1

Looks good.
Attachment #275734 - Flags: review?(jorendorff) → review+
pushed to hg/actionmonkey
remote: added 612 changesets with 3122 changes to 1530 files
changeset 4355:  	2c47ad9a6a45
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
js_LockGCThing could fail, so we need to check it and unlock if necessary
Attachment #275893 - Flags: review?(jorendorff)
Flags: in-testsuite-
Comment on attachment 275893 [details] [diff] [review]
correctly lock unitStrings

Yes, you're right.  Good catch.
Attachment #275893 - Flags: review?(jorendorff) → review+
pushed to hg/actionmonkey
remote: added 1 changesets with 1 changes to 1 files
changeset 4358:  	02ca73bcd21f

(forgot to commit message "; correctly lock unitStrings")
You need to log in before you can comment on or make changes to this bug.