657675 - Expose blacklist (back pointer) facility as Actionscript built-in.

Status: RESOLVED WONTFIX

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

Description

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

Bug 527477 added a cool debugging API for "blacklisting": reporting concrete paths that the GC found to selected objects.

It currently requires one to add invocations in C++ to gc->addToBlacklist.  This is inconvenient when experimenting with Actionscript alone.

Exposing addToBlacklist (and removeFromBlacklist) seems easy to provide.  (Of course the functionality is meaningless if the heap-graph tweak is not been enabled; this is not meant to be an end-user visible feature, but rather an internal debugging aid.)

Comment 1

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

Attachment: patch B v1: Exposes blacklist methods at AS level in System class.

Three main notes I want to make while I have them fresh in my mind:

1. I thought about putting this solely in the avmshell package rather than as (yet another) method in avmplus.System.  But the reality is that this functionality may well be of use when trying to understand behavior of scripts in the Player, so I put it into System.

2. I did not see any way to make the generated shell_toplevel contents be themselves conditional on flags like AVMTWEAK_HEAP_GRAPH.  So I unconditionally included the methods, but they are essentially no-ops if you don't have the tweak turned on.

3. I solved my immediate problem that I was facing with a simpler version of this (analyzing retention beahavior in a microbenchmark), but then when I decided to write up more official looking tests, I realized that you really want some way to map back from the printed addresses to the invocations you made from Actionscript.

The reverse mapping itself is easy, since we already have a hashtable sitting here.  The hard part is deciding what codomain to map back to.

The two main alternatives I considered for codomain of the reverse mapping: Use a string (either derived from the object itself in some way, or passed along as a parameter), or use a uint.  This patch goes with uint.  The main reason I opted not to use a string is that I did not want to raise the spectre of having to manage the memory of the values held in the blacklist table.

This patch is enough to expose some latent bugs in the blacklist system; I'll file separate bugs for those.

Comment 2

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

Attachment: illustration of how add method is called

Here's the output I get when I run this example after applying attachment 533046 [details] [diff] [review] on a Debugger Debug build:

Blacklist item 2 = 0x0x101026fa8 was found in marker graph
Blacklist item 2 = 0x0x101026fa8 wasn't found in mutator graph
Dumping back pointer chain for 0x0x101026fa8
0x0x1011ec108(40)(gc) -> 0x0x101026fa8
0x0x10130fd88(50)(gc) -> 0x0x1011ec108
0x0x10134df18(30)(gc) -> 0x0x10130fd88
0x0x10131ed68(40)(gc) -> 0x0x10134df18
0x0x101201ee8(8)(gc) -> 0x0x10131ed68
0x0x1010a3708(8)(gc) -> 0x0x101201ee8
0x0x10122dfb8(0)(gc) -> 0x0x1010a3708
0x0x101020008(9c0)(gcroot) -> 0x0x10122dfb8
End back pointer chain for 0x0x101026fa8
Blacklist item 1 = 0x0x1011ec4c8 wasn't found in marker graph
Blacklist item 1 = 0x0x1011ec4c8 wasn't found in mutator graph
Dumping back pointer chain for 0x0x1011ec4c8
End back pointer chain for 0x0x1011ec4c8
Blacklist item 2 = 0x0x101026fa8 was found in marker graph
Blacklist item 2 = 0x0x101026fa8 wasn't found in mutator graph
Dumping back pointer chain for 0x0x101026fa8
0x0x1011ec108(40)(gc) -> 0x0x101026fa8
0x0x10130fd88(50)(gc) -> 0x0x1011ec108
0x0x10134df18(30)(gc) -> 0x0x10130fd88
0x0x10131ed68(40)(gc) -> 0x0x10134df18
0x0x101201ee8(8)(gc) -> 0x0x10131ed68
0x0x1010a3708(8)(gc) -> 0x0x101201ee8
0x0x10122dfb8(0)(gc) -> 0x0x1010a3708
0x0x101020008(9c0)(gcroot) -> 0x0x10122dfb8
End back pointer chain for 0x0x101026fa8
Blacklist item 1 = 0x0x1011ec4c8 wasn't found in marker graph
Blacklist item 1 = 0x0x1011ec4c8 wasn't found in mutator graph
Dumping back pointer chain for 0x0x1011ec4c8
End back pointer chain for 0x0x1011ec4c8
Blacklist item 1 = 0x0x1011ec4c8 wasn't found in marker graph
Blacklist item 1 = 0x0x1011ec4c8 wasn't found in mutator graph
Dumping back pointer chain for 0x0x1011ec4c8
End back pointer chain for 0x0x1011ec4c8

Comment 3

[Lars T Hansen]

(In reply to comment #1)
> 
> 1. I thought about putting this solely in the avmshell package rather than
> as (yet another) method in avmplus.System.  But the reality is that this
> functionality may well be of use when trying to understand behavior of
> scripts in the Player, so I put it into System.

Yoy may have figured this out by now but the Player has a different System class in a different file, so you'll end up porting the code anyway.

Comment 4

[Lars T Hansen]

(In reply to comment #1)

> 2. I did not see any way to make the generated shell_toplevel contents be
> themselves conditional on flags like AVMTWEAK_HEAP_GRAPH.  So I
> unconditionally included the methods, but they are essentially no-ops if you
> don't have the tweak turned on.

This is true, and yet we have conditional compilation in AS3 as well as "include", and it would be an interesting exercise to make the feature system generate an .as file that is included into shell files and where APIs are conditionally compiled.  Now that I think about it it does not seem difficult.

Comment 5

[Tommy Reilly]

> This is true, and yet we have conditional compilation in AS3 as well as
> "include", and it would be an interesting exercise to make the feature
> system generate an .as file that is included into shell files and where APIs
> are conditionally compiled.  Now that I think about it it does not seem
> difficult.

This would break the status quo of one shell.abc for the entire API surface.  I'm not sure how bad the ramifications would be but we've backed away from that cliff repeatedly in the past.

Comment 6

[Tommy Reilly]

Comment on attachment 533046 [details] [diff] [review]
patch B v1: Exposes blacklist methods at AS level in System class.

Cool!

Comment 7

[Lars T Hansen]

(In reply to comment #5)
> > This is true, and yet we have conditional compilation in AS3 as well as
> > "include", and it would be an interesting exercise to make the feature
> > system generate an .as file that is included into shell files and where APIs
> > are conditionally compiled.  Now that I think about it it does not seem
> > difficult.
> 
> This would break the status quo of one shell.abc for the entire API surface.
> I'm not sure how bad the ramifications would be but we've backed away from
> that cliff repeatedly in the past.

Would be interesting to know why we backed away from it - lack of appropriate supporting technology (which we now appear to have) or other reasons.

Comment 8

[Tommy Reilly]

> Would be interesting to know why we backed away from it - lack of
> appropriate supporting technology (which we now appear to have) or other
> reasons.

I think we just didn't want to have shell-debugger.abc and shell.abc (although more so for the player).  For the shell we can probably do whatever we want.   If asc.jar was in utils and it automatically used ../shell/shell_toplevel.abc then doing this kind of conditional compilation would be more seamless, dealing with multiple abc's to import into seems cumbersome.   Although in an ideal world asc is dropped on the floor and esc/eval/repl are first class citizens.

Comment 9

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

Attachment: patch B v2: Exposes blacklist methods at AS level in System class

(there's an additional method I threw in here that I still need to implement.  I'm just checkpointing work-state at the moment.)

Comment 10

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

(patch B v2 did not quite capture the same amount of implementation that was in B v1.  Will fix; for now, removing obsolete marker from B v1.)

Comment 11

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

Attachment: patch B v3: Exposes blacklist methods at AS level in System class

patch B v3 has something v2 lacked: implementations of the functions.  :)

This version includes a drive-by replacement of the pattern GCAssert(gc->IsPointerToGCObject(ptr)) to the new assertion method gc->AssertIsPointerToGCObject(ptr) so that when the assertion fails, the debugger actually stops at a spot in the control flow with relevant state to inspect.

(I will probably factor that revision into its own separate changelist shortly; I just wanted to checkpoint my work so far.)

Comment 12

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

Reassigning to Phil.  I wish I had gotten this to the point where I could land it, but other tasks took priority.

The blacklist facility is a really useful piece of machinery (at least when it works), and the MMgc team will almost certainly want some sort of abstract heap traversal/serialization code anyway.  I do not know whether you would want to start from the blacklist/heap-graph api or not, but it *is* a start of sorts.

And besides, the ability to perform these queries from Actionscript code is pretty  awesome.  (Even more impressive would be to expose the output to Actionscript as well; this currently does not do that, it just spits the backpointer chains to the output file, e.g. gcbehavior.txt if that is turned on.

Comment 13

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

(In reply to Felix S Klock II from comment #12)
> The blacklist facility is a really useful piece of machinery (at least when
> it works)

The comment above was tongue-in-cheek, but it would not be invalid to say "lets close this as wontfix" simply based on the blacklist code being potentially broken.

In particular, see Bug 663386.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.