604702 - Convert most VM managed types to use exact tracing

Status: RESOLVED FIXED

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

Description

[Lars T Hansen]

Once we have a proper script for automating the generation of tracers (bug #604701), we should convert as many managed typed in the VM to use exact tracing as seems reasonable (probably most of them).

Comment 1

[Lars T Hansen]

Attachment: Non-as3 (VM-internal) types

Comment 2

[Lars T Hansen]

Attachment: String and Namespace

Comment 3

[Lars T Hansen]

Attachment: ScriptObject (only)

Comment 4

[Lars T Hansen]

Attachment: Subclasses of ScriptObject

I left off a bunch of generated code.  You want to do this:

  ( cd core ; ASC=~/lib/asc.jar AVM=avmshell ./builtin.py )
  ( cd shell ; ASC=~/lib/asc.jar AVM=avmshell ./shell_toplevel.py )

Comment 5

[Lars T Hansen]

These apply in order, and on top of 612561, 612562, and 604701.  In general you may find it easier simply to import my patch queue:
hg.mozilla.org/users/lhansen_adobe.com/redux-revolution.

Comment 6

[Lars T Hansen]

For the record the order is:

Non-as3 (VM-internal) types
String and Namespace
ScriptObject (only)
Subclasses of ScriptObject

Comment 7

[Lars T Hansen]

Attachment: Non-as3 (VM-internal) types, v2

Very minor updates to deal with MMGC_HEAP_GRAPH and AVMFEATURE_WORDCODE_INTERP not compiling.

Comment 8

[Lars T Hansen]

Attachment: ScriptObject (only), v2

Ditto very minor changes.

Comment 9

[Tommy Reilly]

bindingCaches are not GC memory:

            c = new (codeMgr.allocator) C(name, codeMgr.bindingCaches);
            codeMgr.bindingCaches = c;

We don't need to trace them.

Comment 10

[Tommy Reilly]

I have a patch to fix the uintptr_t field in E4XNode to be a DRCWB(GCObject*) that will clean up the uintptr stuff.

For MultinameHashtable I landed the fix for the API abuse today, it uses a template class to handle writing the value.   You could delegate GCTrace to a new VALUE_TYPE, that'll get rid of one conservative trace call.

Hmm with ExactStructContainer I wonder if HeapMultiname is even necessary.   I wonder if we could have:

template <class VALUE_WRITER> 
class MultinameBase

and have two writers a StackWriter and a HeapWriter and then typedef Multiname and HeapMultiname to the appropriate writer.   Not a defect, just thinking out loud.  Actually I don't even know why the ExactStructContainer generated this thought.

Its a shame to add a vtable to MethodInfo but hopefully we'll fix that by not individually allocating them some day.

Comment 11

[Tommy Reilly]

For future reference I prefer to have generated code separated out into separate patches.

Comment 12

[Lars T Hansen]

(In reply to comment #11)
> For future reference I prefer to have generated code separated out into
> separate patches.

Fair enough.  I've not done so in the current patch queue but it's possible to do it if it's a major hardship to review the current patches.  The practical consequence is that you won't be able to test patches one by one necessarily, it may be that they have to be applied and tested in pairs (or in aggregate).

Comment 13

[Tommy Reilly]

Comment on attachment 491172 [details] [diff] [review]
Subclasses of ScriptObject

One real issue:

ArrayObject create method impls should be in ArrayObject-inlines.h

Minor comments:

Why ctor protected for ArrayObject and not private?

Did s/prototype/m_prototype/ occur for a reason or just drive by cleanup, no objections just curious.

I find T::create easier to read than new (gc) T(), less parens.  Maybe we should make it universal?

Out of scope comment: the Error macro cruft could probably be replaced with templates/typedefs.

Comment 14

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

(I'm working on the review now, but my focus is jittery in short term so I do not know if I will finish review tonight.  Will try best.)

Comment 15

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

Comment on attachment 491479 [details] [diff] [review]
Non-as3 (VM-internal) types, v2

I reviewed the nonAs3Types from redux-revolution rev 81:3557fd477624 which I assume is close enough to the attachment that I won't sound looney.

  May be too late now, but would it be possible to change the
  exactgc.as script so that the GC_STRUCTURES, GC_POINTERS, GC_ATOMS
  (etc) declarations were written as e.g.:
    GC_POINTERS_SMALL(m_bases[1], m_baseCount);
  rather than:
    GC_POINTERS_SMALL(m_bases, 1, m_baseCount);
  I think it would reduce the cognitive overhead in reading the
  declarations.  (I would understand reluctance to have to jury-rig
  the parser to extract the field name from the array declaration, but
  it seems like even just hard-coding the special case of <id>[1]
  would be better than reading the presentation here.)

core/DomainEnv.h

  The GC_NO_DATA abbreviation is made for cases like
  GlobalMemorySubscriber, right?

core/avmplusDebugger.h

  Ditto.

core/MethodEnv.h

  Orthogonal: I found this (already present) comment interesting
        // pointers are write-once so we don't need WB's
  Does this pattern occur often?  Can we make a wrapper template to
  assert it dynamically?

core/AbcEnv.cpp

  The diff (at least in redux-revolution) just shows a deleted
  whitespace lines.  Please revert that to reduce changeset-noise.

core/MethodInfo-inlines.h

  nit: A part of the diff is just removing a whitespace line.

core/ScopeChain.cpp

  nit: Parts of the diff is just changing whitespace.

core/Toplevel.h

  Can you briefly justify why you removed the ~Toplevel() destructor
  (that says it was put in to silence compiler warnings?)

core/VTable.h

  You deleted the DATA SECTION BEGIN / END comments here; you
  have not been doing that elsewhere in the patch.  Probably
  better to leave them be (or delete them all).

core/avmplusList.h

  I thought we were deliberately not calling setExact in ctors.
  I'm guessing TracedListData<> is never subclassed?

Comment 16

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

Comment on attachment 491170 [details] [diff] [review]
String and Namespace

(noticed again the use of setExact in the ctors; in this case I assume it was deliberate rather than an oversight.)

Comment 17

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

Comment on attachment 491481 [details] [diff] [review]
ScriptObject (only), v2

core/ScriptObject.cpp

  In gcTrace:
  I understand leaving in existing uses of reinterpret-bits-via-union,
  but shouldn't new code use a reinterpret_cast instead?  Or do I
  misunderstand the pattern here?  (Or do we put more value on
  consistency between the code here and where it was ripped out of
  getTableNoInit?)

core/Traits.cpp

  (same as above; I guess someone will tell me what's up here.)

  I guess we should open a bugzilla ticket to investigate the choice
  of bitmap/list-based tracing.  (Fun stuff!)

Comment 18

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

Comment on attachment 491172 [details] [diff] [review]
Subclasses of ScriptObject

core/ByteArrayGlue.cpp

  What's up with the two ASSUME notes?  Is there something further to
  investigate there, or are they documentation...?  (I don't see
  all-caps ASSUME elsewhere in AVM; I doubt this is the time to
  introduce the notation to denote system invariants...)

core/ErrorClass.h

  Possibly consider breaking up the #if 0 hack block and put each set
  of four with their corresponding DECLARE_NATIVE_ERROR_CLASS macro
  invocation?  Doesn't really make the hack any less ugly; I just
  like seeing related things go together... murgh.

extensions/DictionaryGlue.h

   DictionaryObject and DictionaryClass seem like another place to use
   GC_NO_DATA

shell/DomainClass.h

   DommainClass ditto re GC_NO_DATA.

Comment 19

[Lars T Hansen]

(In reply to comment #13)
> Comment on attachment 491172 [details] [diff] [review]
> Subclasses of ScriptObject
> Minor comments:
> 
> Why ctor protected for ArrayObject and not private?

I believe ArrayObject is subclassed in the Flash Player, but I will double check.

> Did s/prototype/m_prototype/ occur for a reason or just drive by cleanup, no
> objections just curious.

I believe there was a naming conflict, but that conflict may have disappeared again, and I'll check that too.

> I find T::create easier to read than new (gc) T(), less parens.  Maybe we
> should make it universal?

I'd be in favor but I haven't dared ask the FlashRuntime module owners yet :-)

> Out of scope comment: the Error macro cruft could probably be replaced with
> templates/typedefs.

Indeed it should.

Comment 20

[Lars T Hansen]

(In reply to comment #15)
> Comment on attachment 491479 [details] [diff] [review]
> Non-as3 (VM-internal) types, v2
> 
> I reviewed the nonAs3Types from redux-revolution rev 81:3557fd477624 which I
> assume is close enough to the attachment that I won't sound looney.
> 
>   May be too late now, but would it be possible to change the
>   exactgc.as script so that the GC_STRUCTURES, GC_POINTERS, GC_ATOMS
>   (etc) declarations were written as e.g.:
>     GC_POINTERS_SMALL(m_bases[1], m_baseCount);
>   rather than:
>     GC_POINTERS_SMALL(m_bases, 1, m_baseCount);
>   I think it would reduce the cognitive overhead in reading the
>   declarations.  (I would understand reluctance to have to jury-rig
>   the parser to extract the field name from the array declaration, but
>   it seems like even just hard-coding the special case of <id>[1]
>   would be better than reading the presentation here.)

It is broken out the way it is because there are actually a few constant-length arrays in the code whose length is not 1, but I don't know if that actually matters any more.  I will check.

> core/DomainEnv.h
> 
>   The GC_NO_DATA abbreviation is made for cases like
>   GlobalMemorySubscriber, right?

Turns out there are lots of classes that use this in the VM builtins (though I expect fewer in the Flash Player glue).

> core/MethodEnv.h
> 
>   Orthogonal: I found this (already present) comment interesting
>         // pointers are write-once so we don't need WB's
>   Does this pattern occur often?  Can we make a wrapper template to
>   assert it dynamically?

The "pattern" is actually a bit dangerous because it depends on no concurrent marking and no allocation in base classes.  With a faster write barrier we'd want to disallow it completely.

> core/AbcEnv.cpp
> 
>   The diff (at least in redux-revolution) just shows a deleted
>   whitespace lines.  Please revert that to reduce changeset-noise.

Will look for it, ditto others you bring up.

> core/Toplevel.h
> 
>   Can you briefly justify why you removed the ~Toplevel() destructor
>   (that says it was put in to silence compiler warnings?)

I believe that was because Toplevel now subclasses an object that already provides an empty virtual destructor, so the one in Toplevel is redundant even for the purposes of silencing warnings.

> core/VTable.h
> 
>   You deleted the DATA SECTION BEGIN / END comments here; you
>   have not been doing that elsewhere in the patch.  Probably
>   better to leave them be (or delete them all).

Oversight, will fix.

> core/avmplusList.h
> 
>   I thought we were deliberately not calling setExact in ctors.
>   I'm guessing TracedListData<> is never subclassed?

It isn't, and with a private constructor you can count on that.  But it would be better to follow a pattern.  It may have been that with the list template rewrites I got lost in rebasing pain and forgot to clean up.

Comment 21

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

(In reply to comment #20)
> (In reply to comment #15)
> > Comment on attachment 491479 [details] [diff] [review] [details]
> > Non-as3 (VM-internal) types, v2
> > 
> >   May be too late now, but would it be possible to change the
> >   exactgc.as script so that the GC_STRUCTURES, GC_POINTERS, GC_ATOMS
> >   (etc) declarations were written as e.g.:
> >     GC_POINTERS_SMALL(m_bases[1], m_baseCount);
> >   rather than:
> >     GC_POINTERS_SMALL(m_bases, 1, m_baseCount);
> >   I think it would reduce the cognitive overhead in reading the
> >   declarations.  (I would understand reluctance to have to jury-rig
> >   the parser to extract the field name from the array declaration, but
> >   it seems like even just hard-coding the special case of <id>[1]
> >   would be better than reading the presentation here.)
> 
> It is broken out the way it is because there are actually a few constant-length
> arrays in the code whose length is not 1, but I don't know if that actually
> matters any more.  I will check.

You probably realize this, but just to be clear: My suggestion was not to omit the "1" parameter entirely, but rather to merge it in with the first argument.  So one would still be able to handle the constant-length arrays whose length is not 1 (though of course then you could not just hard-code it as a special case).

Comment 22

[Lars T Hansen]

> > Why ctor protected for ArrayObject and not private?
> 
> I believe ArrayObject is subclassed in the Flash Player, but I will double
> check.

It is, at least for "_ArrayObject" in HTMLScriptProxyGlue.h.

Comment 23

[Lars T Hansen]

(In reply to comment #20)
> (In reply to comment #15)

> >   May be too late now, but would it be possible to change the
> >   exactgc.as script so that the GC_STRUCTURES, GC_POINTERS, GC_ATOMS
> >   (etc) declarations were written as e.g.:
> >     GC_POINTERS_SMALL(m_bases[1], m_baseCount);
> >   rather than:
> >     GC_POINTERS_SMALL(m_bases, 1, m_baseCount);
> >   I think it would reduce the cognitive overhead in reading the
> >   declarations.  (I would understand reluctance to have to jury-rig
> >   the parser to extract the field name from the array declaration, but
> >   it seems like even just hard-coding the special case of <id>[1]
> >   would be better than reading the presentation here.)
> 
> It is broken out the way it is because there are actually a few constant-
> length arrays in the code whose length is not 1, but I don't know if that
> actually matters any more.  I will check.

The script no longer needs to know that value, and it's not much work to fix uses of the macro, but I think I understand why I did it this way.  The problem is that arguments to the annotations can't contain arbitrary C++ expressions except in certain contexts, in which case those expressions must be quoted (ie represented as strings) - the script does not want to parse arbitrary C++ expressions, as evidenced by the already-hairy regexes.  That hack works well enough because those quoted expressions are used only by the generator script and not by the C++ code.  It's far from pretty.

In the common case where the expression is just a name or a constant the parsing would be straightforward, however, so it's a question of whether the cleaner syntax weighs up for restrictions on the field.  The restrictions could then be lifted by having variant macros (GC_ATOMS3 comes to mind) that take broken-out expressions.

In the interest of actually landing this code so that people can get on with things I will not fix this now but I will file a follow-up high priority bug.

Comment 24

[Lars T Hansen]

(In reply to comment #16)
> Comment on attachment 491170 [details] [diff] [review]
> String and Namespace
> 
> (noticed again the use of setExact in the ctors; in this case I assume it was
> deliberate rather than an oversight.)

Yes, because these classes are not subclassable.

Comment 25

[Lars T Hansen]

(In reply to comment #17)
> core/Traits.cpp
> 
>   I guess we should open a bugzilla ticket to investigate the choice
>   of bitmap/list-based tracing.  (Fun stuff!)

Will do.

(Aliasing questions taken to another forum.)

Comment 26

[Lars T Hansen]

(In reply to comment #18)
> Comment on attachment 491172 [details] [diff] [review]
> Subclasses of ScriptObject
> 
> core/ByteArrayGlue.cpp
> 
>   What's up with the two ASSUME notes?  Is there something further to
>   investigate there, or are they documentation...?  (I don't see
>   all-caps ASSUME elsewhere in AVM; I doubt this is the time to
>   introduce the notation to denote system invariants...)

They document assumptions.  I'll rephrase.

> core/ErrorClass.h
> 
>   Possibly consider breaking up the #if 0 hack block and put each set
>   of four with their corresponding DECLARE_NATIVE_ERROR_CLASS macro
>   invocation?  Doesn't really make the hack any less ugly; I just
>   like seeing related things go together... murgh.

Murgh indeed.

Tommy has the better idea - abandon the macros and go with templates and typedefs, if we can.

> extensions/DictionaryGlue.h
> 
>    DictionaryObject and DictionaryClass seem like another place to use
>    GC_NO_DATA
> 
> shell/DomainClass.h
> 
>    DommainClass ditto re GC_NO_DATA.

WILLFIX.

Comment 27

[Tamarin Bot]

changeset: 5662:77d21674c487
user:      Lars T Hansen <[e-mail]>
summary:   For 604702 - Convert most VM managed types to use exact tracing: Non-as3 (VM-internal) types (r=fklockii, r=treilly)

http://hg.mozilla.org/tamarin-redux/rev/77d21674c487

Comment 28

[Tamarin Bot]

changeset: 5663:48254cbe2d23
user:      Lars T Hansen <[e-mail]>
summary:   For 604702 - Convert most VM managed types to use exact tracing: String and Namespace (r=fklockii, r=treilly)

http://hg.mozilla.org/tamarin-redux/rev/48254cbe2d23

Comment 29

[Tamarin Bot]

changeset: 5664:df352e72f12e
user:      Lars T Hansen <[e-mail]>
summary:   For 604702 - Convert most VM managed types to use exact tracing: scriptObject (r=fklockii, r=treilly)

http://hg.mozilla.org/tamarin-redux/rev/df352e72f12e

Comment 30

[Tamarin Bot]

changeset: 5665:e0f7b8b8f8f9
user:      Lars T Hansen <[e-mail]>
summary:   For 604702 - Convert most VM managed types to use exact tracing: subclasses of ScriptObject (r=fklockii, r=treilly)

http://hg.mozilla.org/tamarin-redux/rev/e0f7b8b8f8f9

Comment 31

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

(In reply to comment #17)
> Comment on attachment 491481 [details] [diff] [review]
> ScriptObject (only), v2
> 
> core/ScriptObject.cpp
> 
>   In gcTrace:
>   I understand leaving in existing uses of reinterpret-bits-via-union,
>   but shouldn't new code use a reinterpret_cast instead?  Or do I
>   misunderstand the pattern here?  (Or do we put more value on
>   consistency between the code here and where it was ripped out of
>   getTableNoInit?)
> 
> core/Traits.cpp
> 
>   (same as above; I guess someone will tell me what's up here.)

Historical record: the basis for this is documented in Bug 504316 (which in Bug 504316, comment 9 includes a link to a cogent explanation of the problem).