624443 - Convert Flash Player data types to exact tracing in a profile-driven manner

Status: RESOLVED FIXED

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P1)

Description

[Lars T Hansen]

This bug mirrors WE #2726355 and exists for the purpose of connecting the two bug hierarchies.

Comment 1

[Tommy Reilly]

Attachment: exactly trace namespaces

Top hit on checkinapp believe it or not.  I'm reluctant to believe it actually and am doing a quick audit of the conservative tracing profiler.

Comment 2

[Lars T Hansen]

Could be an accidental regression.

Comment 3

[Tamarin Bot]

changeset: 5932:490d83d72463
user:      Tommy Reilly <[e-mail]>
summary:   Bug 624443 - One Namespace new site was missed or reverted, switch it back to Namespace::create (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/490d83d72463

Comment 4

[Tamarin Bot]

changeset: 5934:638802762bcc
user:      Tommy Reilly <[e-mail]>
summary:   Bug 624443 - Make MMGC_CONSERVATIVE_PROFILER compile again

http://hg.mozilla.org/tamarin-redux/rev/638802762bcc

Comment 5

[Tommy Reilly]

Attachment: Get the data we want

This collapses the trace down to one frame so we get profile data on the new site and don't regard the preceding or succeeding frames.  Ex:

Object population profile: Conservative scanning volume incurred by allocation site
Nodes: 20
Roots:  4912 bytes scanned  - 11 objs scanned
  Class 4-7:  8 bytes scanned  - 2 objs scanned
  Class 32-63:  200 bytes scanned  - 5 objs scanned
  Class 512-1023:  2000 bytes scanned  - 2 objs scanned
  Class 1024-2047:  2704 bytes scanned  - 2 objs scanned
Stacks: 6848 bytes scanned  - 4 objs scanned
  Class 512-1023:  944 bytes scanned  - 1 objs scanned
  Class 1024-2047:  5904 bytes scanned  - 3 objs scanned
Objects:
  8448 bytes scanned - 176 objs scanned
			avmplus::AvmCore::newNamespace(avmplus::String*, avmplus::Namespace::NamespaceType, avmplus::ApiVersion) (in avmshell) (AvmCore.cpp:3909)
  48 bytes scanned - 3 objs scanned
			avmplus::InlineHashtable::initialize(MMgc::GC*, int) (in avmshell) (avmplusHashtable.cpp:69)

  48 bytes scanned - 3 objs scanned
			avmplus::Domain::Domain(avmplus::AvmCore*, avmplus::Domain*, unsigned int) (in avmshell) (Domain.cpp:51)

Comment 6

[Tommy Reilly]

Comment on attachment 512797 [details] [diff] [review]
Get the data we want

Canceling review, works nicely on avmshell but too slow for player

Comment 7

[Tommy Reilly]

Attachment: Improve profiler performance

Speedups to make this usable in the player:

Cache first non-mmgc frame in StackTrace
Use a hashtable of StackTrace->PopulationNode intead of linked list

Comment 8

[Lars T Hansen]

Comment on attachment 513123 [details] [diff] [review]
Improve profiler performance

You have a bunch of tab characters here.

Comment 9

[Tommy Reilly]

assume they will be fixed

Comment 10

[Tamarin Bot]

changeset: 5951:a712cf7464cd
user:      Tommy Reilly <[e-mail]>
summary:   Bug 624443 - fix conservative profiler compiler errors with new mark stack and add exact percentage output (r=treilly)

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

Comment 11

[Tommy Reilly]

Attachment: support default C++ namespace in exactgc script

Comment 12

[Tamarin Bot]

changeset: 5962:850a93b0cbf9
user:      Tommy Reilly <[e-mail]>
summary:   Bug 624443 - Improve conservative tracing profiler performance (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/850a93b0cbf9

Comment 13

[Tommy Reilly]

Attachment: add RCObject to no useful tracer interlock exclusion

Comment 14

[Tamarin Bot]

changeset: 5968:944af07df97e
user:      Tommy Reilly <[e-mail]>
summary:   Bug 624443 - enable trace gen to support default C++ namespace (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/944af07df97e

Comment 15

[Tommy Reilly]

RCObject exlusion  fixed in mislabeled change http://hg.mozilla.org/tamarin-redux/rev/4f9427937ffc

Comment 16

[Tommy Reilly]

Attachment: Make nested tracer gen work

Comment 17

[Lars T Hansen]

Comment on attachment 514484 [details] [diff] [review]
Make nested tracer gen work

R+ on the condition that you insert semicolons to terminate statements properly.  Emacs and implicit semicolons do not mix.

Comment 18

[Tamarin Bot]

changeset: 5970:8659a67fff1e
user:      Tommy Reilly <[e-mail]>
summary:   Bug 624443 - Support nested class trace gen (r=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/8659a67fff1e

Comment 19

[Tommy Reilly]

switching back and forth between as and python has me in semi-colon confusion ;-)

Comment 20

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

(In reply to comment #17)
> Comment on attachment 514484 [details] [diff] [review]
> Make nested tracer gen work
> 
> R+ on the condition that you insert semicolons to terminate statements
> properly.  Emacs and implicit semicolons do not mix.

"M-x js-mode" seems to handle implicit semicolons for me in those contexts; but maybe you are doing or anticipating something more sophisticated than my simple test of hitting tab on various lines after removing the semi-colons and seeing how emacs re-indents in response.  Or maybe 

There's supposedly something called "js2-mode" that includes a full Javascript parser (which sounds a little scary).

  http://mihai.bazon.net/projects/editing-javascript-with-emacs-js2-mode

Comment 21

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

(In reply to comment #20)
> how emacs re-indents in response.  Or maybe 

invoking continuation: Or maybe the js-mode's naive regexp is going to burn me later.

Comment 22

[Lars T Hansen]

I have always avoided js mode; in my past experience it was unusable.  I use Java mode exclusively for AS3.  It works well except for "for each" statements (and provided you use semicolons, of course).

Comment 23

[Tamarin Bot]

changeset: 5977:5d7dab9731da
user:      Tommy Reilly <[e-mail]>
summary:   Bug 624443 - Exact flag missing from Dictionary allocations

http://hg.mozilla.org/tamarin-redux/rev/5d7dab9731da

Comment 24

[Ruchi Lohani]

Attachment: HeapHashtableRC, WeakKeyHashtable, WeakValueHashtable tracer generation

This will also require changes on the player side to get the player building once these land. All these hashtables now need to be allocated using their create method. Significantly improves the exactly traced %.

Comment 25

[Lars T Hansen]

Comment on attachment 518512 [details] [diff] [review]
HeapHashtableRC, WeakKeyHashtable, WeakValueHashtable tracer generation

Watch your editor settings - lots of tab characters in these diffs.

Comment 26

[Tommy Reilly]

Comment on attachment 518512 [details] [diff] [review]
HeapHashtableRC, WeakKeyHashtable, WeakValueHashtable tracer generation

+! check out utils/fixtabs before submitting though

Comment 27

[Tommy Reilly]

actually just post another patch with the tabs fixed and I'll push it

Comment 28

[Ruchi Lohani]

Attachment: Fixed tabs

Fixed Tabs. Tom, Please push. Thanks!

Comment 29

[Tamarin Bot]

changeset: 6068:7209cb8d476c
user:      Tommy Reilly <[e-mail]>
summary:   Bug 624443 - Convert avmplus::Hashtable subclasses to exact tracing (author=rlohani,r=treilly,sr=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/7209cb8d476c

Comment 30

[Ruchi Lohani]

Copying this conversation about BindingCache tracing which happened over email: 

******************************** 
It doesn't feel urgent to do anything more than option (b), ie, make the
tracer function an empty function.  There is no invariant in the system,
nor is there likely to be one for a long time yet, that every pointer has
to be visible to the GC; the only invariant is that at least one pointer
to a live object must be visible to the GC.

--lars

On 3/9/11 1:49 PM, "Edwin Smith" <[e-mail]> wrote:

Yes, that's option (b).

i'm happy to fix this up to do something more legitimate once there
is a facility for it.  the sketchy part is copying gc pointers
into fixedmem.

Ed

On Mar 9, 2011, at 1:41 AM, Lars Hansen wrote:

If bindingCaches points to non-mmgc memory then that memory won't be
scanned by the GC.  What the code below does is say that it does not
know
what's in the bindingCaches pointet and that the usual conservative
scanning checks will therefore have to be applied to the pointer.  This
is
equivalent to old behavior, I believe.

What you're pointing out is that we could remove the call because
bindingCaches is known not to point to GC'd memory, I think.

--lars

On 3/9/11 3:58 AM, "Edwin Smith" <[e-mail]> wrote:

I just ran across this code in class CodeMgr in CodegenLIR.h

 void gcTrace(MMgc::GC* gc)
 {
     // Punt until we figure out if this is what's intended
     gc->TraceConservativeLocation((uintptr_t*)&bindingCaches);
 }

apologies, I hope this doesn't trigger a TLDR fault on your end(s)...

bindingCaches is a pointer into a linked list of BindingCache
instances that are allocated from a fixed-mem arena.  Each cache
may or may not contain an Atom, VTable*, and a MethodEnv*.  The
fields are unioned together with no tag.  Each cache subclass
has several handlers (see CallCache, SetCache, GetCache), and each
handler dictates the contents of the unioned fields.

I designed all this under the assumption none of it would be traced.
All these atoms and pointers are copies of values that live in other
structures, and we explicitly clear the fields of all caches when
an AbcEnv is destroyed, which is the event that would invalidate
these pointers.

When this code was written, Lars and I talked and IIRC agreed its
legal, and likely to stay legal, so long as there's no way any of
the cached pointers can be destructed sooner than ~AbcEnv(), and
so long as we dont have a moving collector.

At this point it would be worth revisiting those assumptions, duh :-)

options:
a) do nothing
b) dont trace any of it, including &bindingCache
c) trace the whole list conservatively
d) trace the whole list exactly; any cache in any state has
known values in the unioned fields, but the tracer needs to know
the cache state.  each state change could install a different
tracer pointer, or we could do something else with some spare bits.

We've talked about a templatized way to have tagged union pointers,
If that exists, I'd be game for using it here.  not hard.

Also, any suggestions to improve the comments (see "Binding Cache
Design") would be appreciated.  The first two paragraph explain
how the fields are used but don't spell out exactly which states
set which fields.

Ed
*************************************

Comment 31

[Ruchi Lohani]

So I see the current plan is to remove the tracing for bindingCache from CodeMgr. In that case we can also remove the tracing of CodeMgr from PoolObject which is currently getting traced through the hook? Am I right or we just want to keep (why?) the empty CodeMgr gcTrace() ?

Comment 32

[Edwin Smith]

Not tracing CodeMgr sounds okay to my untrained eye; it is also always allocated via mmfx_new.

Comment 33

[Lars T Hansen]

(In reply to comment #32)
> Not tracing CodeMgr sounds okay to my untrained eye; it is also always
> allocated via mmfx_new.

Agree that should be fine, the hook only does this because of the binding cache anyway.   And then the hook on PoolObject goes away too!

Comment 34

[Ruchi Lohani]

Attachment: Remove BindingCache tracing + the tracing hook in PoolObject

See Comment 31.

Comment 35

[Lars T Hansen]

Comment on attachment 520721 [details] [diff] [review]
Remove BindingCache tracing + the tracing hook in PoolObject

If I were you I would leave a comment on the definition of the codeMgr member to the effect that it points to mmfx memory, so that the next person reading the code is not inclined to start worrying about whether a tracer was missed.

Comment 36

[Ruchi Lohani]

Attachment: Remove CodeMgr tracer and PoolObject tracer hook

Comment added to CodeMgr member in PoolObject. 
Lars, please push this, after you have reviewed/approved it.

Comment 37

[Tamarin Bot]

changeset: 6121:3cbaff8b72bd
user:      Lars T Hansen <[e-mail]>
summary:   For 624443 - Convert Flash Player data types to exact tracing in a profile-driven manner: Remove CodeMgr tracer and PoolObject tracer hook (patch=rulohani, r=fklockii, sr=lhansen, push=lhansen)

http://hg.mozilla.org/tamarin-redux/rev/3cbaff8b72bd

Comment 38

[Ruchi Lohani]

Attachment: Use ExactHeaplist instead of HeapList

Comment 39

[Ruchi Lohani]

Comment on attachment 521104 [details] [diff] [review]
Use ExactHeaplist instead of HeapList

HeapE4XNodeList and HeapNamespaceList will be exactly traced with this change.

Comment 40

[Lars T Hansen]

Comment on attachment 521104 [details] [diff] [review]
Use ExactHeaplist instead of HeapList

Seems fine.

I'm having some second thoughts about ExactHeapList.  As a follow-up item (not a blocker for this one, and probably a separate bug) it would be good if you could weight the pros and cons of ExactHeapList vs. providing the HeapList template with a second flag that specifies exactness (ideally MMgc::kExact would be that flag but that may not work; don't know).  I don't know how reliable that would be or anything like that, or if there are real advantages.  I just think it's something we should investigate in the name of clean APIs.  When I created ExactHeapList I was probably in make-it-work mode.

Comment 41

[Ruchi Lohani]

I am also wondering why do we need HeapList now that we have ExactHeapList? There is only one place I found in player where I could not convert HeapList to ExactHeapList. It was used for UnsafeGCObjectList<SObject> with a comment saying "...its dangerous to do this and in future UnsafeGCObjectList will be removed...". What are the other cases where we need HeapList ?

Comment 42

[Lars T Hansen]

(In reply to comment #41)
> I am also wondering why do we need HeapList now that we have ExactHeapList?

IMO we should retain HeapList for now, as an option for people who do not do exact tracing for a type.

That said...

As Tommy has pointed out, the writing's on the wall: Going to exact tracing for all object types is in our future, as soon as we have a little more experience with it, develop a few more tools, and so on.  It means that every pointer-containing object will pay for a vtable, but that's a small cost - most objects pay already.  It means that every array type must be a structured array type (no more GC::Alloc and GC::Calloc) but that's just for the good.

On the other hand, it means that the tracer no longer needs to check the kVirtualGCTrace bit, no fallback to conservative tracing, probably other simplifications and minor speedups.  (Stack scanning remains conservative.)

Comment 43

[Tamarin Bot]

changeset: 6176:3948cab16c24
user:      Tommy Reilly <[e-mail]>
summary:   Bug 624443 - Use ExactHeapList instead of HeapList for E4X (author=ruchi,r=felix,sr=lars)

http://hg.mozilla.org/tamarin-redux/rev/3948cab16c24

Comment 44

[Dan Smith]

Ruchi, any reason to leave this bug open?

Comment 45

[Ruchi Lohani]

Closing this. Changes for Serrano are in TR and FP mainline.