723210 - GC should keep a reference to Telemetry

Status: RESOLVED FIXED

(Tamarin Graveyard :: Profiler, defect)

Description

[Ruchi Lohani]

With Bugzilla 602663, AvmCore now has a reference to Telemetry. GC can get the telemetry reference from AvmCore by calling core()->getTelemetry(). Since GC gets created independent of AvmCore, there will be a time window when there is not avmcore but GC might want to report metrics. For that reason its reasonable to have GC get the telemetry directly.

Comment 1

[Ruchi Lohani]

Attachment: Patch

Comment 2

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

Comment on attachment 593635 [details] [diff] [review]
Patch

R+.  This is what I expected.

(Well, I was expecting this, plus a change to AvmCore::setTelemetry to propogate the assignment into the GC via GC::setTelemetry.  I do not know whether there's any good reason not to do that; we'll never want distinct Telemetrys hanging off the AvmCore and the GC, right?)

Comment 3

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

(In reply to Felix S Klock II from comment #2)
> (Well, I was expecting this, plus a change to AvmCore::setTelemetry to
> propogate the assignment into the GC via GC::setTelemetry.  I do not know
> whether there's any good reason not to do that; we'll never want distinct
> Telemetrys hanging off the AvmCore and the GC, right?)

Actually, automatically propogating the assignment is not as "obviously right" as I thought at first, since as you point out, the whole point of this ticket is to allow the client to set the Telemetry before we even have an AvmCore in the first place, and therefore propogating the assignment will either be redundant or wrong in those cases.

(But still, to me it seems like the right thing to do.)

Comment 4

[Ruchi Lohani]

(In reply to Felix S Klock II from comment #3)
> (In reply to Felix S Klock II from comment #2)
> 
> (But still, to me it seems like the right thing to do.)

I think an assert would be better? Uploading a new patch.

Comment 5

[Ruchi Lohani]

Attachment: Patch v1

Comment 6

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

Comment on attachment 594035 [details] [diff] [review]
Patch v1

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

::: core/AvmCore-inlines.h
@@ +99,4 @@
>  
>  REALLY_INLINE void AvmCore::setTelemetry(telemetry::ITelemetry* telemetry)
>  {
> +    AvmAssert(telemetry == gc->getTelemetry());

So now there's an undocumented rule in the protocol (and unchecked in Release builds) that before doing AvmCore::setTelemetry, you must ensure that you have also done setTelemetry on the GC associated with the core?

This does not strike me as particularly elegant.  (The suggestion I had made was perhaps not too elegant either; really, all three options that I see are ugly.)

I'm going to R- it because there's no documentation of the constraint.  But even if you add documentation, I will probably have to review how setTelemetry is called from the Flash Runtime side.

Have you tested this change in an FRR build; if so, I assume you must have a patch for that you could send along to me in email or by some other means?

Comment 7

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

(In reply to Felix S Klock II from comment #6)
> This does not strike me as particularly elegant.  (The suggestion I had made
> was perhaps not too elegant either; really, all three options that I see are
> ugly.)

FYI: The three options I was referring to here:

1. Your original patch; no assertion, no propogation.

2. My suggestion: propogation

3. Your suggestion: an assertion

There ore other options; we could however think a little bigger here, and try revising the protocol in the overall big picture.  I don't have time right now to really work out the implications of that though.

Comment 8

[Ruchi Lohani]

(In reply to Felix S Klock II from comment #6)
> Comment on attachment 594035 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 594035 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: core/AvmCore-inlines.h
> @@ +99,4 @@
> >  
> >  REALLY_INLINE void AvmCore::setTelemetry(telemetry::ITelemetry* telemetry)
> >  {
> > +    AvmAssert(telemetry == gc->getTelemetry());
> 
> So now there's an undocumented rule in the protocol (and unchecked in
> Release builds) that before doing AvmCore::setTelemetry, you must ensure
> that you have also done setTelemetry on the GC associated with the core?
> 
> This does not strike me as particularly elegant.  (The suggestion I had made
> was perhaps not too elegant either; really, all three options that I see are
> ugly.)
> 
> I'm going to R- it because there's no documentation of the constraint.  But
> even if you add documentation, I will probably have to review how
> setTelemetry is called from the Flash Runtime side.
> 
> Have you tested this change in an FRR build; if so, I assume you must have a
> patch for that you could send along to me in email or by some other means?


As I understand there is a protocol involved in the player. I will send you a diff for the player side. My understanding is that Telemetry (which should set itself into the GC) should be created in the client once the GC is created and then when AvmCore is created, setTelemetry on Avmcore will be called. I am now against my suggestion of an assertion. Will discuss more in an email with runtime related details.

Comment 9

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

(In reply to Ruchi Lohani from comment #8)

Okay.  I in turn will retract my suggestion for propogating the mutation from the AvmCore into the GC object.

I think we're in agreement about the short-term approach here: Namely, to land your patch as it was originally written, with no assertion.

I'll send some non-vm thoughts via e-mail.

Comment 10

[Tamarin Bot]

changeset: 7186:562d69dfe487
user:      Ruchi Lohani<[e-mail]>
summary:   Bug 723210 - GC should keep a reference to Telemetry(r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/562d69dfe487

Comment 11

[Edwin Smith]

This change appears to be causing an assert in FRR mac-32-debug-standalone builds,
when you load any SWF:

Assertion failed: "((pageMap.AddrIsMappable(addr)))" ("/Users/edwsmith/fr-redux/products/player/osx/../../../third_party/avmplus/MMgc/GC-inlines.h":718)

Comment 12

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

Ruchi: did you need to land follow-up material in FRR to make this all hold together properly?

(I cannot look into this right now; I have to go out for the evening, prior commitment.  If it has not been addressed by tomorrow morning CET I will look at it then.)

Comment 13

[Edwin Smith]

I'm not blocked, by the way, I'm just back to my normal workflow with a revert-this-change patch at the root of my MQ.

Comment 14

[Ruchi Lohani]

I am looking into it. I will land the fixes in FRR soon.

Comment 15

[Tamarin Bot]

changeset: 7195:757a9c953db1
user:      Ruchi Lohani<[e-mail]>
summary:   Bug 723210 - remove unnecessary code (r=fklockii)

http://hg.mozilla.org/tamarin-redux/rev/757a9c953db1