654945 - Full complement of managed array types

Status: RESOLVED WONTFIX

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

Description

[Lars T Hansen]

Right now we have HeapList<T> and ExactHeapList<T> for variable-length arrays of GCObject, GCFinalizedObject, and RCObject, and the atrociously named ExactStructContainer<T> for fixed-length arrays of GCObject and GCFinalizedObject only (it improves on ExactHeapList because it requires only a single allocation).  Additionally there are some ad-hoc types like QuadContainer.

We want to develop a full complement of managed array types:

- pointer-containing (all types)
- byte arrays
- extensible and fixed-length
- finalizable and not

Getting this in place is a requirement for removing GC::Alloc and GC::Free from the MMgc API, and enforcing the invariant that every managed object subclasses some managed base type.

Comment 1

[Lars T Hansen]

By "byte arrays" I really meant to say "non-pointer-containing" arrays.  And so far, I've not seen a reason for those to be extensible, though DataList really provides that service so there's no reason to omit extensibility from the requirement list.

Comment 2

[Lars T Hansen]

Tommy notes (bug 658564) that structures like ExactStructContainer, which take a finalizer function as a function pointer and do not register themselves for finalization if that pointer is NULL, also should not have to store the NULL pointer, and that perhaps some template magic can be brought to bear on that problem.

Comment 3

[Tommy Reilly]

Attachment: Add a GCPrimitiveArray

Couple notes:
- went for GCAPI namespace with "GC" prefixed name, anticipation of having split public/private headers
- count passed in via new, not size, GC handles calloc due diligence
- a WB goes away as well as a raw Alloc as well as a suspect size multiplication

Comment 4

[Tommy Reilly]

Attachment: apply GCPrimitiveArray to invocation count field

Comment 5

[Lars T Hansen]

Comment on attachment 543145 [details] [diff] [review]
Add a GCPrimitiveArray

I'm SR-'ing this on principle:

This is a new MMgc or VM API to be used by lots of client code (I actually suspect the API really belongs in the VM, not in MMgc, with blessed access to MMgc through some as-yet-unspecified API but that's just my opinion, in the same way that the List APIs will probably end up looking) so let's do it properly and treat it like API design.

I want to see a reasoned API proposal, probably on the wiki because that's where we generally work, that lays out the problem to be solved, discusses possible solutions and provides not just this one piece of the puzzle but actually a solution for the entire puzzle to the extent where we can hide GC::Alloc and GC::Calloc from everyone but blessed clients (basically the array types).

Comment 6

[Tommy Reilly]

Why would a managed primitive array belong in the virtual machine?   I have always thought the list classes should move to MMgc too, or at least if not to MMgc to a "utils" library above it.

Frankly I'm at a loss for why we need everything this bug asks for so don't feel qualified to undertake a grand wiki puzzle solving endeavor.    

I suppose I could generate a list of all the ::Alloc call sites and use that to drive the effort but again that's scope creep over and above my current assignment.    I would think my solution is lightweight enough that if we decide later we easily revisit it.   GCPrimitiveArray will be used a grand total of 7 times (1 in tamarin, 3 in flash/core and 3 in flash/avmglue).

I'll do a quick GC::Alloc investigation and see what we're up against...

Comment 7

[Lars T Hansen]

(In reply to comment #6)
> Why would a managed primitive array belong in the virtual machine?

Because that's where all the List types currently are.

> I have
> always thought the list classes should move to MMgc too, or at least if not
> to MMgc to a "utils" library above it.

Then that's not in MMgc.  It's one thing for the VM to be a dumping ground for things we don't use; worse for MMgc to be it.

> Frankly I'm at a loss for why we need everything this bug asks for so don't
> feel qualified to undertake a grand wiki puzzle solving endeavor.

OK, then I guess we postpone that for the time being.

> I suppose I could generate a list of all the ::Alloc call sites and use that
> to drive the effort but again that's scope creep over and above my current
> assignment.

It may also not be terribly interesting to have that list; it's unclear what we would learn from it.

> I would think my solution is lightweight enough that if we
> decide later we easily revisit it.   GCPrimitiveArray will be used a grand
> total of 7 times (1 in tamarin, 3 in flash/core and 3 in flash/avmglue).

If we're introducing new data types to be used by the player then we should go through a design exercise to make sure we end up somewhere desirable, not hack in something that happens to work for the use cases that we've seen in existing code.  The SR- stands.

Comment 8

[Tommy Reilly]

Attachment: Alloc hits in FR

These results are almost all primitives and makes me think we're overthinking this bug.

Comment 9

[Lars T Hansen]

(In reply to comment #8)
> Created attachment 543424 [details]
> Alloc hits in FR
> 
> These results are almost all primitives and makes me think we're
> overthinking this bug.

I don't understand.  Clearly we have needs along all the dimensions I listed, and a subset of those needs are being served by various data structures we already have, while the rest are served by GC::Alloc and GC::Calloc.  No doubt we can invent various data structures to solve those latter in isolation, but what we will have is a set of non-uniform managed array types; this API burden benefits nobody.  All I'm arguing is that we need to discuss what the needs are and see if we can come up with a unified solution that makes it easier, and not harder, to make use of AVM+ facilities.

Comment 10

[Tommy Reilly]

Actually looking deeper almost all of these Alloc calls are for char* so String may be a better abstraction than a list (although the case I address in tamarin in my patch is a uint64_t* so we'd still need something for that).

Comment 11

[Tommy Reilly]

And I think m_invocationCounts could have been served by a DataList<uint64_t> with the difference being that it would be unmanaged memory and not GC memory.  Probably doesn't matter too much either way in this case but I'm starting to see some of the holes that inspired this bug.  

A GCDataList would be nice to have although arguably DataList itself should use GC memory, its used in AbcParser/PoolObject, describeType and Vector in tamarin which all make sense to be GC memory.   Need to look at how DataList is used in the player.

Comment 12

[Tommy Reilly]

Anyone remember why DataList uses FixedMalloc instead of GC memory?   Write barrier avoidance?

Comment 13

[Lars T Hansen]

(In reply to comment #12)
> Anyone remember why DataList uses FixedMalloc instead of GC memory?   Write
> barrier avoidance?

"Tradition, mostly"?  I consider it a bug.  Also see 643371.

Comment 14

[Steven Johnson]

"Reduce GC pressure", mutter mutter, something like that? No compelling reason I know of.

Comment 15

[Tamarin Bot]

changeset: 6481:47d6d75afd61
user:      Tommy Reilly <[e-mail]>
summary:   Bug 654945 - Full complement of managed array types, add DataList<char> template instantiation (r=me)

http://hg.mozilla.org/tamarin-redux/rev/47d6d75afd61

Comment 16

[Tommy Reilly]

Attachment: Allow by reference [] operator

Comment 17

[Steven Johnson]

Comment on attachment 548456 [details] [diff] [review]
Allow by reference [] operator

Review of attachment 548456 [details] [diff] [review]:
-----------------------------------------------------------------

::: core/avmplusList.h
@@ +690,5 @@
>      typedef ListImpl<Atom, AtomListHelper> AtomList;
>  
>      // ----------------------------
> +    // DataList wraps a ListImpl instead of inheriting from it or typedef'ing to it so it
> +    // can provide a by referece array operator. 

typo/spelling, plz fix

Comment 18

[Tamarin Bot]

changeset: 6487:6f6da34a03a3
user:      Tommy Reilly <[e-mail]>
summary:   Bug 654945 - Make DataList support by reference [] operator for more general usage (r=stejohns,r-pending=edwsmith)

http://hg.mozilla.org/tamarin-redux/rev/6f6da34a03a3

Comment 19

[Tommy Reilly]

Instead of trying to do a GCPrimitiveArray I've added a way to elide kContainsPointer in the type system via a new type "LeafObject".  That is probably all we need.  See bug 654946

Comment 20

[Lars T Hansen]

Comment on attachment 548456 [details] [diff] [review]
Allow by reference [] operator

Canceling SR?, the patch landed two months ago.

Comment 21

[Tommy Reilly]

Targetting to future, making DataList use GC memory and/or using the new LeafObject/LeafVector seems to suffice for now

Comment 22

[Felix S. Klock II [:pnkfelix, :fklock]]

If we don't see a motivation to continue this work, we probably should drop it.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.