528977 - PrecomputedMultinames should not be a GCRoot

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Lars T Hansen]

This table tends to be large, and it's immutable after creation.  As a GCRoot it is scanned in its entirety, which may cause pauses.  Also, as a GCRoot it is scanned twice.  As an RCObject or GCFinalizedObject neither problem would be a factor.

Comment 1

[Steven Johnson]

Why are GCRoots scanned twice? That would seem to be a generic problem...

Comment 2

[Lars T Hansen]

(In reply to comment #1)
> Why are GCRoots scanned twice? That would seem to be a generic problem...

Consider that GC is incremental.  The root set is scanned when a GC cycle starts (otherwise there's nothing to work with).  As the cycle moves along, the write barrier takes care of intercepting stores of unmarked objects into marked objects, but there is no write barrier mechanism for the roots.  So at the end of the cycle we must re-scan the roots to look for pointers to still-unmarked objects.

Generally you want the root set to be quite small, so that the double scanning constitutes a very small expense and doesn't create significant pauses either at the beginning of the GC cycle or at the end.  A small root set also makes it much more likely that the second scan of the roots will find very little new scanning work, and that's important for incrementality too, because that scanning will have to be done there and then (or you get issues with termination).

Tommy was remarking just the other day that the root set (in the Flash Player) had grown unreasonably and that it may be time for a crackdown.

Comment 3

[Lars T Hansen]

The easy way to fix this is probably to make the table a GCFinalizedObject (linked from the pool) and then create a synthetic GCRoot that is live while the multiname table is being filled and which covers the table memory; that way we can avoid issues of the write barrier (Multiname can't have barriers embedded in it, it's supposed to be stack allocated, and efficiency is a concern).  Following the first GC after the completion of table construction that root can be deleted.

The hard way to fix this is to make Multiname objects immutable and to change all the code that depends on mutable Multiname objects, and then have explicit barrier when we store a fully constructed, immutable Multiname in the table.

Comment 4

[Edwin Smith]

Can we set the array up as an array of HeapMultiname instances and make PrecomputedMultinames be a GCObject?  HeapMultiname has the explicit WB's needed for setting name and ns when building.

Comment 5

[Lars T Hansen]

We could do that.  (Though code that accesses the PrecomputedMultiname table would have to change globally as a HeapMultiname isn't a Multiname but contains one.)

Comment 6

[Lars T Hansen]

Actually going the HeapMultiname route appears to be very straightforward, thanks to existing good encapsulation.

Comment 7

[Lars T Hansen]

Attachment: Patch

Comment 8

[Tommy Reilly]

Comment on attachment 417700 [details] [diff] [review]
Patch

This array is write once right (each slot gets lazily written one time)?   Just want to make sure all the new write barrier traffic won't be a perf problem.

Comment 9

[Lars T Hansen]

(In reply to comment #8)
> (From update of attachment 417700 [details] [diff] [review])
> This array is write once right (each slot gets lazily written one time)?   Just
> want to make sure all the new write barrier traffic won't be a perf problem.

Every entry is written once, but eagerly - when the array is initialized just following SWF loading.  Barrier traffic could be an issue, but the pools tend to live long so in principle we should recover that cost after a couple of collections (since we will only scan the array once per GC instead of twice, as we do now that it's a root).  Also, given the structure of the barrier, if marking is not running or if the table has tripped the barrier - which is likely given that we're storing fresh objects into it - then the barrier's not too terribly expensive.

Comment 10

[Lars T Hansen]

Comment on attachment 417700 [details] [diff] [review]
Patch

Actually does not work because GC::GetGC, used for the barrier in HeapMultiname, does not work except on the first block of an object.  (Acceptance test harness did not catch this because it's broken, see bug #535100.)

Comment 11

[Lars T Hansen]

Attachment: Patch, v2

This works around the problem with GC::GetGC by introducing a new setMultiname function on HeapMultiname that takes the GC and container as arguments and handles the write barriers properly.  I'm asking for a full re-review because this kind of hackery makes me nervous.

Comment 12

[Steven Johnson]

Comment on attachment 417896 [details] [diff] [review]
Patch, v2

re: "We must /not/ use WB_NULL here, only the full WB functions that
take the gc and container explicitly will be safe" ... why is this?

Comment 13

[Tommy Reilly]

Comment on attachment 417896 [details] [diff] [review]
Patch, v2

GetGC needs to die

Comment 14

[Lars T Hansen]

(In reply to comment #12)
> (From update of attachment 417896 [details] [diff] [review])
> re: "We must /not/ use WB_NULL here, only the full WB functions that
> take the gc and container explicitly will be safe" ... why is this?

It's the GetGC problem again, WB_NULL relies on GetGC to find the container.

Comment 15

[Lars T Hansen]

redux changeset:   3338:654a987ed9e7

Comment 16

[Steven Johnson]

(In reply to comment #14)
> It's the GetGC problem again, WB_NULL relies on GetGC to find the container.

urr?

inline void write_null(void* p) { *(uintptr_t*)(p) = 0; }
#define WB_NULL(addr) write_null((void*)addr)

Comment 17

[Lars T Hansen]

(In reply to comment #16)
> (In reply to comment #14)
> > It's the GetGC problem again, WB_NULL relies on GetGC to find the container.
> 
> urr?
> 
> inline void write_null(void* p) { *(uintptr_t*)(p) = 0; }
> #define WB_NULL(addr) write_null((void*)addr)

Thank you for persisting!  I made those changes because I had a crash during testing, but I went back and looked since your   Turns out I introduced a bug in that patch.  The code used to have one WB_NULL and one WBRC_NULL, and the latter was a problem (because it does use GetGC), but I replaced both by a WB() macro, which is wrong - one should be a WBRC().  Will fix this.

Comment 18

[Lars T Hansen]

Adjustment to code and comments pushed as redux changeset: 3348:80529452f86f

Comment 19

[Chris Peyer]

Engineering work item.  Marking as verified.