PrecomputedMultinames should not be a GCRoot

VERIFIED FIXED in flash10.1

Status

Tamarin
Virtual Machine
P3
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: Lars T Hansen, Assigned: Lars T Hansen)

Tracking

unspecified
flash10.1

Details

Attachments

(1 attachment, 1 obsolete attachment)

7.73 KB, patch
Tommy Reilly
: review+
Steven Johnson
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

8 years ago
Why are GCRoots scanned twice? That would seem to be a generic problem...
(Assignee)

Comment 2

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

Comment 3

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

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

Comment 5

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

Comment 6

8 years ago
Actually going the HeapMultiname route appears to be very straightforward, thanks to existing good encapsulation.
(Assignee)

Comment 7

8 years ago
Created attachment 417700 [details] [diff] [review]
Patch
Attachment #417700 - Flags: review?(treilly)
Attachment #417700 - Flags: review?(edwsmith)

Updated

8 years ago
Attachment #417700 - Flags: review?(edwsmith) → review+

Comment 8

8 years ago
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.
Attachment #417700 - Flags: review?(treilly) → review+
(Assignee)

Comment 9

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

Comment 10

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

Comment 11

8 years ago
Created attachment 417896 [details] [diff] [review]
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.
Attachment #417700 - Attachment is obsolete: true
Attachment #417896 - Flags: review?(treilly)
Attachment #417896 - Flags: review?(stejohns)
Attachment #417896 - Flags: review?(edwsmith)

Comment 12

8 years ago
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?
Attachment #417896 - Flags: review?(stejohns) → review+

Comment 13

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

GetGC needs to die
Attachment #417896 - Flags: review?(treilly) → review+
(Assignee)

Comment 14

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

Comment 15

8 years ago
redux changeset:   3338:654a987ed9e7
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Attachment #417896 - Flags: review?(edwsmith)

Comment 16

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

Comment 17

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

Comment 18

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

Comment 19

8 years ago
Engineering work item.  Marking as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.