TT: merge TC's MMgc to tamarin-tracing

VERIFIED FIXED

Status

defect
P1
normal
VERIFIED FIXED
12 years ago
10 years ago

People

(Reporter: edwsmith, Assigned: jimb)

Tracking

Details

Attachments

(16 attachments, 3 obsolete attachments)

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
Reporter

Description

12 years ago
a brain-ded copy produced compile errors, but the two branches shouldn't be too far off from each other.

Comment 1

12 years ago
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.

Comment 2

12 years ago
Excellent, does this supersede 411210?

Comment 3

12 years ago
Partial merge being applied for https://bugzilla.mozilla.org/show_bug.cgi?id=411210

Updated

12 years ago
Attachment #295975 - Attachment is obsolete: true

Comment 4

12 years ago
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.
Assignee

Updated

11 years ago
Blocks: 425818
Assignee

Updated

11 years ago
Status: NEW → ASSIGNED
Assignee

Updated

11 years ago
Assignee: nobody → jim
Status: ASSIGNED → NEW
Reporter

Updated

11 years ago
Priority: -- → P1
Assignee

Comment 5

11 years ago
Jason's going to take this, since he's already been working on MMgc.
Assignee: jim → jorendorff
Assignee

Comment 6

11 years ago
Assignee: jorendorff → jim
Status: NEW → ASSIGNED
Attachment #320823 - Flags: review?(treilly)
Assignee

Comment 9

11 years ago
Attachment #320826 - Flags: review?(treilly)
Assignee

Comment 11

11 years ago
Attachment #320828 - Flags: review?(treilly)
Assignee

Comment 12

11 years ago
Attachment #320829 - Flags: review?(treilly)
Assignee

Comment 14

11 years ago
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

Updated

11 years ago
Attachment #320824 - Flags: review?(treilly) → review+

Updated

11 years ago
Attachment #320829 - Flags: review?(treilly) → review+

Comment 15

11 years ago
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?

Updated

11 years ago
Attachment #320827 - Flags: review?(treilly) → review+

Comment 16

11 years ago
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?

Updated

11 years ago
Attachment #320825 - Flags: review?(treilly) → review+
Assignee

Comment 17

11 years ago
(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.
Assignee

Comment 18

11 years ago
(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.
Assignee

Updated

11 years ago
Attachment #320828 - Attachment is obsolete: true
Attachment #320828 - Flags: review?(treilly)
Assignee

Updated

11 years ago
Attachment #320823 - Attachment is obsolete: true
Attachment #320823 - Flags: review?(treilly)
Assignee

Comment 19

11 years ago
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)
Assignee

Comment 20

11 years ago
(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.)
Assignee

Comment 22

11 years ago
Attachment #321112 - Flags: review?(treilly)
Assignee

Comment 28

11 years ago
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 29

11 years ago
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.
Assignee

Comment 30

11 years ago
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/
Assignee

Comment 31

11 years ago
In Tamarin phone call, Rick suggested setting incrementalValidation, incrementalValidationPedantic, and greedy in the GC constructor while debugging.
Assignee

Comment 32

11 years ago
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. 
Assignee

Comment 33

11 years ago
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.
Assignee

Comment 34

11 years ago
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)

Updated

11 years ago
Attachment #325220 - Flags: review?(treilly) → review+
Assignee

Comment 35

11 years ago
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.

Updated

11 years ago
Attachment #320826 - Flags: review?(treilly) → review+

Comment 36

11 years ago
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...

Updated

11 years ago
Attachment #320831 - Flags: review?(treilly) → review+

Updated

11 years ago
Attachment #321115 - Flags: review?(treilly) → review+

Comment 37

11 years ago
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+

Updated

11 years ago
Attachment #321111 - Flags: review?(treilly) → review+

Updated

11 years ago
Attachment #321112 - Flags: review?(treilly) → review+

Updated

11 years ago
Attachment #321114 - Flags: review?(treilly) → review+

Updated

11 years ago
Attachment #321119 - Flags: review?(treilly) → review+

Updated

11 years ago
Attachment #321121 - Flags: review?(treilly) → review+

Updated

11 years ago
Attachment #321122 - Flags: review?(treilly) → review+

Comment 38

11 years ago
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
Assignee

Comment 39

11 years ago
Sloppy, sloppy.  I'll go and work this stuff out.  Thank you so much for the review!
Assignee

Comment 40

11 years ago
Tamarin Central includes a sampling profiler; Tamarin Tracing doesn't.
Attachment #326385 - Flags: review?(treilly)
Assignee

Comment 41

11 years ago
That seems to address the --enable-debugger builds on Linux.
Assignee

Comment 42

11 years ago
Pushed changeset 8a157c0ccd27, which includes the fix for --enable-debugger builds on Linux.  Mac and Windows builds are next.

Updated

11 years ago
Attachment #326385 - Flags: review?(treilly) → review+

Comment 43

11 years ago
No problem, let me know when you want me to throw it at our build bot again.
Reporter

Updated

11 years ago
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED

Updated

10 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.