Closed Bug 409021 Opened 13 years ago Closed 12 years ago

TT: merge TC's MMgc to tamarin-tracing

Categories

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P1)

x86
Windows XP
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: edwsmith, Assigned: jimb)

References

Details

Attachments

(16 files, 3 obsolete files)

4.14 KB, patch
treilly
: review+
Details | Diff | Splinter Review
852 bytes, patch
treilly
: review+
Details | Diff | Splinter Review
544 bytes, patch
treilly
: review+
Details | Diff | Splinter Review
734 bytes, patch
treilly
: review+
Details | Diff | Splinter Review
485 bytes, patch
treilly
: review+
Details | Diff | Splinter Review
429 bytes, patch
treilly
: review+
Details | Diff | Splinter Review
2.20 KB, patch
treilly
: review+
Details | Diff | Splinter Review
13.07 KB, patch
treilly
: review+
Details | Diff | Splinter Review
747 bytes, patch
treilly
: review+
Details | Diff | Splinter Review
1.97 KB, patch
treilly
: review+
Details | Diff | Splinter Review
1.02 KB, patch
treilly
: review+
Details | Diff | Splinter Review
1.60 KB, patch
treilly
: review+
Details | Diff | Splinter Review
1.55 KB, patch
treilly
: review+
Details | Diff | Splinter Review
1.04 KB, patch
treilly
: review+
Details | Diff | Splinter Review
2.38 KB, patch
treilly
: review+
Details | Diff | Splinter Review
1.07 KB, patch
treilly
: review+
Details | Diff | Splinter Review
a brain-ded copy produced compile errors, but the two branches shouldn't be too far off from each other.
During the work on bug 411210 I figured I'd be wiser to start this merge. So this is the trivial stuff from TC's version of MMgc. The work on making it thread safe probably requires someone with at least basic level of understanding of what is going on there.
Excellent, does this supersede 411210?
Partial merge being applied for https://bugzilla.mozilla.org/show_bug.cgi?id=411210
Attachment #295975 - Attachment is obsolete: true
The "partial merge" patch was rolled into the fix for 411210 thus I marked it obsolete. Leaving this bug open for now because it is only a partial merge, and MMgc should be unified between TT and TC.
Blocks: 425818
Status: NEW → ASSIGNED
Assignee: nobody → jim
Status: ASSIGNED → NEW
Priority: -- → P1
Jason's going to take this, since he's already been working on MMgc.
Assignee: jim → jorendorff
Assignee: jorendorff → jim
Status: NEW → ASSIGNED
Attachment #320823 - Flags: review?(treilly)
Attachment #320826 - Flags: review?(treilly)
Attachment #320828 - Flags: review?(treilly)
Attachment #320829 - Flags: review?(treilly)
The attached patches are the changes in Tamarin Tracing's MMgc that aren't present in Tamarin Central's MMgc, phrased as patches against TC's MMgc.  I've started a TC test run with these applied, but they look pretty safe.  With these applied, I'll drop TC's MMgc into TT and see what breaks.

The r?'s are really just, "Do these look sane for TC's MMgc?"
Status: ASSIGNED → NEW
Attachment #320824 - Flags: review?(treilly) → review+
Attachment #320829 - Flags: review?(treilly) → review+
Comment on attachment 320828 [details] [diff] [review]
Add an optional 'extra' argument to the 'GC *' new.

I'm nervous about this change, code that used to pass an int for flags might be misconstrued as passing 0 for extra.   Why do we need this form?
Attachment #320827 - Flags: review?(treilly) → review+
Comment on attachment 320823 [details] [diff] [review]
Use 'uint32' for some bit manipulation.

Is this diff backwards?  I thought we were removing the extra uint32 casts?
Attachment #320825 - Flags: review?(treilly) → review+
(In reply to comment #15)

I was wondering about that ambiguity, too.

The 'extra' argument isn't needed by Tamarin Tracing.  You added it in changeset e9c63f605939 (obscured by the fact that MMgc/ also moves to space/MMgc/); the log message doesn't say why.

So, let's drop this unless we learn more.
(In reply to comment #16)
> (From update of attachment 320823 [details] [diff] [review])
> Is this diff backwards?  I thought we were removing the extra uint32 casts?   

Sorry --- this is just the patch directly from the TT history, not revised based on the tamarin-devel thread.  Once I understand what's needed, I'll replace this patch with something better.
Attachment #320828 - Attachment is obsolete: true
Attachment #320828 - Flags: review?(treilly)
Attachment #320823 - Attachment is obsolete: true
Attachment #320823 - Flags: review?(treilly)
This patch implements the suggestions Tommy made re: GetIndex on the tamarin-devel mailing list:
https://mail.mozilla.org/pipermail/tamarin-devel/2008-May/000541.html
Attachment #320961 - Flags: review?(treilly)
(In reply to comment #17)
> I was wondering about that ambiguity, too.
> 
> The 'extra' argument isn't needed by Tamarin Tracing.  You added it in
> changeset e9c63f605939 (obscured by the fact that MMgc/ also moves to
> space/MMgc/); the log message doesn't say why.
> 
> So, let's drop this unless we learn more.

I've learned more.  :)

My check of whether TT needed the extra argument was too shallow: it turns out that TT *only* needs operator new's 'extra' argument, and never the 'flags' argument.  I didn't find any three-argument or two-argument-passing-flags calls to the 'new' operator.  (If anyone knows of a quick way to find such, let me know.)
Attachment #321112 - Flags: review?(treilly)
With patches 321111 on, Tamarin Tracing builds against Tamarin Central's MMgc, (copied into space/MMgc).  I'll be making a repo of this available shortly.  There are still a number of test suite failures, which I'll look into.
Comment on attachment 321122 [details] [diff] [review]
Make new(GC*) take an optional 'extra' arg, not an optional 'flags' arg.

we should remove the GCObject's new operator and just have the global one.  Or just make them the same for now.  When the SPAM/MMgc API redux takes place GCObject dies hopefully.
With these patches applied, many tests pass, but many also fail:
as3/RuntimeErrors/Error1119DeleteDoesNotSupportXMLListOperand.as
dies with a segfault, apparently referring to a GC'd object.
This tree is available as revision 0d92a40e1c16 in:
http://hg.mozilla.org/users/jblandy_mozilla.com/tt-mmgc-update/
In Tamarin phone call, Rick suggested setting incrementalValidation, incrementalValidationPedantic, and greedy in the GC constructor while debugging.
There seems to be a problem with deferred reference counting.

First, the symptom.  If I build the revision cited above with --enable-shell and --enable-debug, and then run avmshell with the -Dgreedy option, it segfaults because a ScriptObject that is referenced on the interpreter stack gets collected.

The first op for stack2array in vm_fpu_interp.h, op_stack2array_plus_0000, calls constructobject; the object returned by this call the first time it is reached is the one that gets collected while still live.

The object seems to have a zero reference count (i.e. the 'composite' field is 0x80000201, and (composite & 0xff)-1 is the reference count) every time GC is performed, from the time the object is first allocated.  However, this should be okay: the interpreter's avmplus::AvmCore::GCInterface::prereap callback is supposed to tell the zero count table about the objects referenced from the interpreter stack; the object's STACK_PIN bit in 'composite' is supposed to get set, so that ZCT::Reap will leave it alone.

At the 36th call to CollectWithBookkeeping, however, it no longer has its STACK_PIN bit set, and gets freed.  The object is still on the value stack.  GC::Free poisons its contents, and the next time the interpreter tries to fetch one of its fields, it crashes.

I've run out of time today, but the obvious thing to do is figure out why the prereap hook fails to set the object's STACK_PIN flag the 36th time. 
The 'prereap' callback has changed between the old and new versions of MMgc: TT expects MMgc::GCCallback to have a prereap method that is passed a ZCT *, whereas the new MMgc's GCCallback has prereap() and prereap (void *).  So where TT expects to be overriding an existing virtual method, it's actually adding a new, non-virtual method, which the new MMgc never calls.

What's probably allowing the object to stay alive as long as it does is some stray reference from the C stack.

Fix should be obvious.
Unlike Tamarin Central, Tamarin Tracing doesn't keep the ABC value
stack on the C stack, so MMgc's stock behavior of pinning RC objects
referenced from the C stack doesn't handle TT's ABC stack.  Instead,
Tamarin Tracing registers a prereap callback that invokes the ZCT
PinStackObjects method on the ABC (and Forth) value and return stacks
explicitly.  AvmCore.h expects a virtual GCCallback::prereap (ZCT *)
method it can override.
Attachment #325220 - Flags: review?(treilly)
Attachment #325220 - Flags: review?(treilly) → review+
With the latest patch attached, and the fix for bug 439502, TT runs with no regressions using a TC MMgc.

A complete series of patches --- those attached to this bug, plus adaptations of changes people have made to TT's MMgc since the last time around --- is available as the series of changesets ending with bf8a0833bd2d in:
http://hg.mozilla.org/users/jblandy_mozilla.com/tt-mmgc-update

There have only been minor changes made to the currently attached patches waiting for review.
Attachment #320826 - Flags: review?(treilly) → review+
Sweet, glad to hear it!  I'll spend the morning reviewing the outstanding patches.    We have an internal build system that we throw changes like this at before pushing that might help.  It builds TT on 3 platforms (win/mac/linux), in all configurations, for 2 architectures (x86/arm) using 3 different compilers.   I'll throw this at that and see what happens...
Attachment #320831 - Flags: review?(treilly) → review+
Attachment #321115 - Flags: review?(treilly) → review+
Comment on attachment 320961 [details] [diff] [review]
Use uint32 for GCAlloc's multiple, shift, and GetIndex return value.

In the TT these should all be C99 types, but that's a bigger change, this is fine for now.
Attachment #320961 - Flags: review?(treilly) → review+
Attachment #321111 - Flags: review?(treilly) → review+
Attachment #321112 - Flags: review?(treilly) → review+
Attachment #321114 - Flags: review?(treilly) → review+
Attachment #321119 - Flags: review?(treilly) → review+
Attachment #321121 - Flags: review?(treilly) → review+
Attachment #321122 - Flags: review?(treilly) → review+
Jim, do have a way to test out the windows and mac builds?    Your personal branch doesn't build in either. Also the "Debugger" builds don't build on linux
Sloppy, sloppy.  I'll go and work this stuff out.  Thank you so much for the review!
Tamarin Central includes a sampling profiler; Tamarin Tracing doesn't.
Attachment #326385 - Flags: review?(treilly)
That seems to address the --enable-debugger builds on Linux.
Pushed changeset 8a157c0ccd27, which includes the fix for --enable-debugger builds on Linux.  Mac and Windows builds are next.
Attachment #326385 - Flags: review?(treilly) → review+
No problem, let me know when you want me to throw it at our build bot again.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.