Closed Bug 602663 Opened 9 years ago Closed 10 months ago

Add Telemetry Feature

Categories

(Tamarin Graveyard :: Profiler, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX
Q2 12 - Cyril

People

(Reporter: gpeacock, Assigned: gpeacock)

References

Details

Attachments

(2 files, 6 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-us) AppleWebKit/533.16 (KHTML, like Gecko) Version/5.0 Safari/533.16
Build Identifier: 

This is to track the initial integration of the Telemetry feature into the Tamarin source.

Reproducible: Always
Depends on: 594546
Blocks: 594546
No longer depends on: 594546
This is just the change to avmfeatures.as to have a Telemetry option. Compiling and running avmfeatures.abc changes avmfeatures.py, avmfeatures.h and avmfeatures.cpp which are all under source control. Do I submit the patches for those as well?
Nope, bugzilla reviews only need to include human-generated files.
This hooks Telemetry into the Player build. It isn't yet functional in the VM as I have to add networking. This is a preliminary version. I have code to replace this that makes use of dataio and is based on amf3. I'm just looking for feedback on the integration at this point.
Attachment #481665 - Attachment is obsolete: true
Very briefly some style comments: Tabstops at 4 spaces, actual tab characters are verboten.  Documentation on public methods in the header file + usage documentation over the class definition.  'tagId' is probably too generic to be defined at the top level of the avmplus namespace.  Try to group static and instance data separately, and bracket with a suitable comment, see eg core/MethodEnv.h.
Is this still the valid bug to use for this work?  Should this be assigned to Gavin or Srikumar's team?
Assignee: nobody → gpeacock
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Flags: flashplayer-bug-
Target Milestone: --- → Future
Target Milestone: Future → Q2 12 - Cyril
Attached patch VM-Telemetry interface changes (obsolete) — Splinter Review
First patch for VM-Telemetry interface changes.

I haven't included any generated files including avmfeatures.h and avmfeatures.cpp in the patch.

There is no shell implementation and the feature is disabled for shell.

(Please ignore the patch that was uploaded by Gavin long back, entitled 'Preliminary Telemetry API' -- I couldn't mark it obsolete)
Attachment #579609 - Flags: superreview?(edwsmith)
Attachment #579609 - Flags: review?(kpalacz)
Attachment #579609 - Flags: feedback?(gpeacock)
This is a first step toward full telemetry support in the VM. It will allow telemetry metrics to be added to the VM and used when the VM is built as part of a player with existing telemetry support. 
The next step is to add an implementation of an ITelemetry interface in the vmShell. I have some source code available and provide a sample for this. My sample simply dumps text to standard out, but a full implementation can also be integrated.
Please review this as a first step. The other phases will follow.
(i suspect this probably falls on to my plate rather than kpalacz's.  I am busy working on a different problem today, but I can start reviewing this on Monday.  Nonetheless I'll leave the review assigned to kpalacz just in case he wants first stab at it today.)
Comment on attachment 579609 [details] [diff] [review]
VM-Telemetry interface changes

Looks good to me.
Nits:
ITelemetry - to me, telemetry is a problem domain, not an entity that participates in solving the problem. I'd prefer a name like ITelemetryAgent, if it's appropriate.
Comments - capitalize first word, end with a period.
Attachment #579609 - Flags: review?(kpalacz) → review+
Thanks for the comments, Krzysztof.

I will fix the comments.

As an interface name, ITelemetry looks fine to me and the implementation class could probably be better named as TelemetryAgent rather than Telemetry (but we are using the name Telemetry itself in the current player implementation also). 

Felix, you are welcome to review this change. Let me know if you would like me to add your name as reviewer.
Fixed the comments in the header file as per Krzysztof's comments (and rebased to the latest tamarin-redux).
Attachment #579609 - Attachment is obsolete: true
Attachment #580874 - Flags: superreview?(edwsmith)
Attachment #580874 - Flags: review?(kpalacz)
Attachment #580874 - Flags: feedback?(gpeacock)
Attachment #579609 - Flags: superreview?(edwsmith)
Attachment #579609 - Flags: feedback?(gpeacock)
Attachment #580874 - Flags: review?(kpalacz) → review+
Attachment #580874 - Flags: review?(fklockii)
Comment on attachment 580874 [details] [diff] [review]
Revision 1 for VM-Telemetry interface changes

R+

I assume the only list of the supported types for the third argument to the TELEMETRY_* macros is the code itself, which is probably good enough.

Most of the macros seem self-explanatory.  Two exceptions:

1. Consider transcribing the text specifying the behavior of macros like TELEMETRY_TIME into the Telemetry.h header file.

2. I'm curious about whether any atom at all is acceptable to TELEMETRY_ATOM, or if there are constraints on what values it can deal with.  (This question can be stated more generally: can one expect Telemetry to simply discard inputs that it does not know how to deal with, or will it crash/signal an error, which implies that one should take care in selecting the inputs for a telemetry invocation?)

Nit: It would be nice if the trailing whitespace in Telemetry.h were removed; we aren't perfect about following this convention but it helps cut down on extraneous diffs.  You can use utils/hooks/tamarin-commit-hook.py to detect it and utils/fixtabs to remove it from the non-blank lines.

Nit: I did notice an extraneous space here:
        t->WriteAtomValue( x, v); \
in the TELEMETRY_ATOM definition.
Attachment #580874 - Flags: review?(fklockii) → review+
(In reply to Felix S Klock II from comment #12)
> 2. I'm curious about whether any atom at all is acceptable to
> TELEMETRY_ATOM, or if there are constraints on what values it can deal with.
> (This question can be stated more generally: can one expect Telemetry to
> simply discard inputs that it does not know how to deal with, or will it
> crash/signal an error, which implies that one should take care in selecting
> the inputs for a telemetry invocation?)

Update: from skimming the telemetry implementation, it looks like it does indeed handle aribtrary atoms (which is good).

This reduces the priority on answering my parenthetical question above.  I suspect that other telemetry implementations (i.e. whatever we put in avmshell for the first go-round) will either do a best-effort transcription of arbitrary atoms or simply ignore the input (which, hey, might be its best-effort).

The main thing I want to tease out is a clear statement about whether there exist otherwise valid inputs that could telemetry to crash or raise an error.  My current assumption is that the answer is "No".  E.g. if you have an Atom that Actionscript could handle, then TELEMETRY_ATOM will not error on it.
Thanks for reviewing, Felix.

Player's Telemetry implementation does handle all types of atoms (float & float4 need to be added once it is available). If it doesn't recognize a type, it will ignore the input.

I will work on your other comments and upload a new patch.
Let me correct my previous comment: If Telemetry doesn't recognize a type, it should write null, but it should never crash. 

The reason being Telemetry writes metrics as name, value pairs and only after name is serialized, we would try to check & serialize value (which could be Atom in the current scenario). So, a null value has to be written.

Currently, the implementation handles all types.
(In reply to Sateesh from comment #15)
> Let me correct my previous comment: If Telemetry doesn't recognize a type,
> it should write null, but it should never crash. 
> 
> The reason being Telemetry writes metrics as name, value pairs and only
> after name is serialized, we would try to check & serialize value (which
> could be Atom in the current scenario). So, a null value has to be written.

Off-hand note: JSON solves a similar problem in some cases by delaying the serialization (or at least emission of the serialized string) of the name until after it has confirmed that it will also serialize and emit the associaed value.

But even that trick does not suffice for JSON in all cases; namely arrays that contain unmarshallable values.  So in those cases such values map to null, just like what you describe here.

So I don't object to the implementation you describe; just noting one potential alternative.  I suspect "fixing" it would not be worth the effort.
(In reply to Felix S Klock II from comment #16)
> Off-hand note: JSON solves a similar problem in some cases by delaying the
> serialization (or at least emission of the serialized string) of the name
> until after it has confirmed that it will also serialize and emit the
> associaed value.
> 
> But even that trick does not suffice for JSON in all cases; namely arrays
> that contain unmarshallable values.  So in those cases such values map to
> null, just like what you describe here.
> 
> So I don't object to the implementation you describe; just noting one
> potential alternative.  I suspect "fixing" it would not be worth the effort.

That's right...even if it is fixed that way, it would not be uniform and has to be handled differently in the code depending on the 'depth' of the value.

It is better to write 'null' always.
This is a minor update to the previous patch and also rebased to the latest branch.

Fixed spaces and wrote comments for TELEMETRY_TIME (only; as others are self explanatory).
Attachment #580874 - Attachment is obsolete: true
Attachment #581235 - Flags: superreview?(edwsmith)
Attachment #581235 - Flags: review?(kpalacz)
Attachment #581235 - Flags: review?(fklockii)
Attachment #581235 - Flags: feedback?(gpeacock)
Attachment #580874 - Flags: superreview?(edwsmith)
Attachment #580874 - Flags: feedback?(gpeacock)
Attachment #581235 - Flags: review?(fklockii) → review+
Comment on attachment 488058 [details] [diff] [review]
Preliminary Telemetry API

Local style in avmplus namespace and the core module is camelCase methods; please rename.  Overall, underdocumented; at the very least, some one line class and method comments explaining whats-it-for and what-it-does.  Overall, local std is 4-spc indent, no tabs.  Telemetry.cpp/h probably had tabs and seem 8-indented in this patch.  Copy modelines from another .cpp/h file and put them above the copyright comment.

Telemetry.GetElapsedTime() needs to document what the units are.  (millis?)  is it valid to store uint64_t m_startTime(), but only return elapsed time as uint32_t?  Silently truncated elapsed time isn't good; need a usable/testable failure mode or just return uint64_t.  if clients know they're short-running they can truncate.  Should GetElapsedTime() be const?  what other methods can be const?

Telemetry.Log(tagId, const char *str) should talk about ownership of str.  is it copied?  if not, how long must the pointer be active?  Same for Log(tagId, uint8_t*, uint32_t len) - who owns the buffer, how long should pointer be valid.

ditto for tagname strings.

Multiple instances of Telemetry allow for multiple buffers and timers, but tag names are process-wide.  Is this ok for workers?

Document the implications of changing the enabled flag on the fly while a bunch of Telemetry instances are active.  (okay to disallow - just say it)

There's no api to associate code-position information with events;  future?

One selftest is better than none at all, but its not enough.

Messy indentation in Telemetry.cpp for #define TAG_MASK and others.
Comment on attachment 581235 [details] [diff] [review]
Revision 2 for VM-Telemetry interface changes

Ok, I see some of my comments for the first patch are addressed in this patch, others not so much, so please review.

If you're using InitCaps to adhere to the module style guidelines, insert a comment saying so.

Updates needed in avm.gypi?
Attachment #581235 - Flags: superreview?(edwsmith) → superreview+
(In reply to Edwin Smith from comment #20)
> Comment on attachment 581235 [details] [diff] [review]
> Revision 2 for VM-Telemetry interface changes
> 
> Ok, I see some of my comments for the first patch are addressed in this
> patch, others not so much, so please review.
> 
> If you're using InitCaps to adhere to the module style guidelines, insert a
> comment saying so.
> 
> Updates needed in avm.gypi?

Thanks for reviewing, Edwin. 

Yes, I will ignore your previous comments which are based on 'Preliminary API' which Gavin has uploaded long back (please comment again, if you would like to repeat any of those comments on the new patch), and I couldn't obsolete the attachment -- I will ask Gavin, if he can remove that patch.

I didn't use any tools to adhere to the module style guidelines -- please let me know if I should be using any tool (I used fixtabs, though).

No updates needed to avm.gypi, as only a header file has been newly added (but it was added to all the required project files, i.e. vcproj and xcodeproj).
Comment on attachment 581235 [details] [diff] [review]
Revision 2 for VM-Telemetry interface changes

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

Hi Sateesh, I reviewed this API with Gavin and there are a number of changes we're like to request. See my detailed Splinter review notes for Telemetry.h. 

Thanks,
Mark S

::: core/Telemetry.h
@@ +20,5 @@
> + * Portions created by the Initial Developer are Copyright (C) 2004-2006
> + * the Initial Developer. All Rights Reserved.
> + *
> + * Contributor(s):
> + *   Adobe AS3 Team

The group submitting this patch is not actually part of the AS3 team. Let's just say "Adobe Engineering".

@@ +36,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#ifndef __avmplus_Telemetry__

Telemetry is not part of AVM, it is a separate feature. "avmplus" should not appear anywhere in this header file.

@@ +39,5 @@
> +
> +#ifndef __avmplus_Telemetry__
> +#define __avmplus_Telemetry__
> +
> +#ifndef VMCFG_TELEMETRY

General note: this file needs to be fully commented - including a comment at the top of the file that briefly explains what Telemetry is and how to use it (including maybe examples), and informative comments for each macro and public method.

@@ +41,5 @@
> +#define __avmplus_Telemetry__
> +
> +#ifndef VMCFG_TELEMETRY
> +
> +#define TELEMETRY_VALUE(t, x, v)

Please remove the TELEMETRY_VALUE macro. This macro relies on the C++ compiler correctly choosing one of the overloaded WriteValue functions. Both Gavin and I have experienced bugs resulting from an unexpected choice by the compiler. So, the best solution is to remove TELEMETRY_VALUE entirely.

@@ +50,5 @@
> +#define TELEMETRY_DOUBLE(t, x, v)
> +#define TELEMETRY_STRING(t, x, v)
> +#define TELEMETRY_BYTES(t, x, v,l)
> +#define TELEMETRY_TIME(t, x)
> +#define TELEMETRY_ATOM(t, x)

Please remove all references to Atom, including the TELEMETRY_ATOM macro, the WriteAtomValue function, and the WriteSpan(Atom) funcion. The Telemetry API should not require or depend on any VM-specific data types. plus we are not yet committed to providing full support for Atoms.

@@ +61,5 @@
> +    // If you are seeing any compiler/linker errors while using these macros, you probably are passing
> +    // unsupported data types and you would need to either use a supported data type or typecast the
> +    // arguments to one of the supported data types.
> +    
> +    // For all the macros, first argument is always ITelemetry*.

The following comments should be split into multiple lines instead of one big long line.

@@ +63,5 @@
> +    // arguments to one of the supported data types.
> +    
> +    // For all the macros, first argument is always ITelemetry*.
> +    // Second argument is metric name (please use reverse DNS notation and for native metrics (i.e. not from ActionScript) the metric name should start with '.' -- for example, a metric in VM could be ".vm.core.internString").
> +    // Thrird argument, if applicable, is the value of metric of any supported type  (for TELEMETRY_VALUE, expected type of arguments is any type allowed by WriteValue function in ITelemetry class or union of all other macros with same number of arguments excluding Atoms; for other macros, the type may be of the type indicated by the name of the macro).

Please remove all references to Atom, and all references to TELEMETRY_VALUE. 

"Thrird" should be "Third"

@@ +65,5 @@
> +    // For all the macros, first argument is always ITelemetry*.
> +    // Second argument is metric name (please use reverse DNS notation and for native metrics (i.e. not from ActionScript) the metric name should start with '.' -- for example, a metric in VM could be ".vm.core.internString").
> +    // Thrird argument, if applicable, is the value of metric of any supported type  (for TELEMETRY_VALUE, expected type of arguments is any type allowed by WriteValue function in ITelemetry class or union of all other macros with same number of arguments excluding Atoms; for other macros, the type may be of the type indicated by the name of the macro).
> +    
> +    // TELEMETRY_VALUE macro can be used for writing value metrics of all supported types, but excluding Atoms (also excluding TELEMETRY_BYTES).

Please remove all references to Atom.

@@ +80,5 @@
> +    do { if (t && t->IsActive()) { \
> +        t->WriteValue(x, v, l); \
> +    } } while(0)
> +        
> +    // Use this for writing AS3 atoms.

Please remove all references to Atom

@@ +138,5 @@
> +        t->WriteValue( x, (const char*)v); \
> +    } } while(0)
> +
> +
> +namespace avmplus

Telemetry is not part of AVM, it is a separate feature that can be used in any host application. We should use the namespace "telemetry" instead.

@@ +158,5 @@
> +        virtual void WriteValue(telemetryId id, const char* value) = 0;
> +        virtual void WriteValue(telemetryId id, uint64_t value) = 0;
> +        virtual void WriteValue(telemetryId id, const uint8_t *value, uint32_t len) = 0; // Writes bytearray.
> +        
> +        virtual void WriteAtomValue(telemetryId id, Atom atom, bool fromAS = false) = 0;

Please remove all references to Atom

@@ +161,5 @@
> +        
> +        virtual void WriteAtomValue(telemetryId id, Atom atom, bool fromAS = false) = 0;
> +        
> +        virtual void WriteSpan(telemetryId id, uint64_t start, bool force, bool fromAS = false) = 0;
> +        virtual void WriteSpan(telemetryId id, uint64_t start, Atom atom, bool force, bool fromAS = false) = 0;

Please remove all references to Atom

@@ +183,5 @@
> +                m_telemetry->WriteSpan(m_id, m_start, m_force);
> +            }
> +        }
> +        
> +        void Init(ITelemetry *telem, telemetryId id, bool force)

There doesn't need to be a separate function TelemetryMetrhod::init(). All the code from this function should go into the constructor.
(In reply to Sateesh from comment #21)
> (In reply to Edwin Smith from comment #20)
> > Comment on attachment 581235 [details] [diff] [review]
> > Revision 2 for VM-Telemetry interface changes
> >  
> > If you're using InitCaps to adhere to the module style guidelines, insert a
> > comment saying so.
>
> I didn't use any tools to adhere to the module style guidelines -- please
> let me know if I should be using any tool (I used fixtabs, though).

FYI: When Ed said "If you're using InitCaps ...", he was not referring to the name of a tool for adhering to style guidelines.  Ed was referring (somewhat obtusely) to your using an initial upper-case letter in the method names, as opposed to using an initial lower-case letter in the method names as one sees in other code within the tamarin-redux core/ directory.

(I don't have a horse in this race; I just wanted to clear up a potential misundertanding.)
Thanks for the comments, Mark and Gavin.

I have updated the patch. Let me know if you still have any comments.

Also, made ITelemetry::IsActive and AvmCore::GetTelemetry as non-virtual, so that they could be inlined and introduced ITelemetry::SetActive and AvmCore::SetTeleemtry protected methods. These changes would ensure that the impact would be minimal when Telemetry is disabled or enabled, but not connected.
Attachment #581235 - Attachment is obsolete: true
Attachment #583454 - Flags: superreview?(edwsmith)
Attachment #583454 - Flags: review?(kpalacz)
Attachment #583454 - Flags: review?(fklockii)
Attachment #583454 - Flags: feedback?(mshepher)
Attachment #583454 - Flags: feedback?(gpeacock)
Attachment #581235 - Flags: review?(kpalacz)
Attachment #581235 - Flags: feedback?(gpeacock)
Edwin, Krzysztof, Felix, Gavin and Mark,
Can you please review this patch? Thanks.
Comment on attachment 583454 [details] [diff] [review]
Revision 3 for VM-Telemetry interface changes

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

Looks good to me, some nits below.

::: core/AvmCore.h
@@ -72,5 @@
>          ,VB_parse        = 1<<30 // abc parsing information
>          ,VB_verify       = 1<<29 // verification information
>          ,VB_interp       = 1<<28 // interpreter information
>          ,VB_jit          = 1<<27 // jit information
>          ,VB_traits       = 1<<26 // traits creation information

The following line (unexpected const declaration) seems like a diff glitch, probably innocuous.

::: core/Telemetry.h
@@ +190,5 @@
> +            m_id = id;
> +            m_telemetry = telem;
> +            m_force = force;
> +            if (m_telemetry && m_telemetry->IsActive()) {
> +                m_start = (uint64_t)m_telemetry->GetElapsedTime();

The cast seems superfluous.
Attachment #583454 - Flags: review?(kpalacz) → review+
Comment on attachment 583454 [details] [diff] [review]
Revision 3 for VM-Telemetry interface changes

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

At some point the avm.gypi file will need to be updated in the same manner that the Xcode and VS projects have been.  Unfortunately the avmshell build process does not currently test the gyp files, so to test such changes, one would need to plug AVM into FlashRuntime-Redux.

If you file a bug for this work and assign that Bug to me, I can take care of it after this lands; alternatively, you can update the patch here (and not worry about another round of reviews, assuming the super-review from Ed is positive).
Attachment #583454 - Flags: review?(fklockii) → review+
The XCode and VS project files have been updated with 'Telemetry.h', but my understanding is that, since there are no cpp file additions/deletions, no changes are required for gyp files.
(In reply to Sateesh from comment #28)
> The XCode and VS project files have been updated with 'Telemetry.h', but my
> understanding is that, since there are no cpp file additions/deletions, no
> changes are required for gyp files.

Oops, you are right; ignore my comment then.
This patch is same as the previous patch, but rebased to the latest changes in tamarin-redux.

Thank you all for reviewing.
Attachment #583454 - Attachment is obsolete: true
Attachment #587973 - Flags: superreview?
Attachment #587973 - Flags: feedback?
Attachment #583454 - Flags: superreview?(edwsmith)
Attachment #583454 - Flags: feedback?(mshepher)
Attachment #583454 - Flags: feedback?(gpeacock)
Attachment #587973 - Flags: superreview?(edwsmith)
Attachment #587973 - Flags: superreview?
Attachment #587973 - Flags: feedback?(mshepher)
Attachment #587973 - Flags: feedback?(gpeacock)
Attachment #587973 - Flags: feedback?
Comment on attachment 587973 [details] [diff] [review]
Revision 3 for VM-Telemetry interface changes (rebased)

As per earlier comments, please use camelCase for method names, especially in AvmCore where everything should already be camelCase.  If you already have decided to stick with InitialCaps, please only do it in the new telemetry code, not in existinc classes (consistency).

I've already SR+ on earlier patches, so just delegating this arbitrarily to Felix.  Krzysztof - poach if you want.
Attachment #587973 - Flags: superreview?(edwsmith) → superreview?(fklockii)
I will do that Edwin. I also have in mind whether Telemetry.h should have actually been named as ITelemetry.h (to allow for the actual implementation class to use Telemetry.h later) -- now that I anyway have to update the patch after this change, I will do both the changes and update the patch. I guess this would be fine with everybody.
This patch has the following changes relative to the previous patch

1. Rename AvmCore::GetTelemetry and AvmCore::SetTelemetry to AvmCore::getTelemetry and AvmCore::setTelemetry, respectively.
2. Rename Telemetry.h as ITelemetry.h
Attachment #587973 - Attachment is obsolete: true
Attachment #588360 - Flags: superreview?(fklockii)
Attachment #588360 - Flags: feedback?(gpeacock)
Attachment #587973 - Flags: superreview?(fklockii)
Attachment #587973 - Flags: feedback?(mshepher)
Attachment #587973 - Flags: feedback?(gpeacock)
Comment on attachment 588360 [details] [diff] [review]
Revision 4 for VM-Telemetry interface changes

SR+; (I am working on landing this in TR now.)
Attachment #588360 - Flags: superreview?(fklockii) → superreview+
(In reply to Sateesh from comment #32)
> I will do that Edwin. I also have in mind whether Telemetry.h should have
> actually been named as ITelemetry.h (to allow for the actual implementation
> class to use Telemetry.h later) -- now that I anyway have to update the
> patch after this change, I will do both the changes and update the patch. I
> guess this would be fine with everybody.

Good thinking -- easier to set the filename now than to change it later.
(In reply to Edwin Smith from comment #35)
> (In reply to Sateesh from comment #32)
> > I will do that Edwin. I also have in mind whether Telemetry.h should have
> > actually been named as ITelemetry.h (to allow for the actual implementation
> > class to use Telemetry.h later) -- now that I anyway have to update the
> > patch after this change, I will do both the changes and update the patch. I
> > guess this would be fine with everybody.
> 
> Good thinking -- easier to set the filename now than to change it later.

I think the change to ITelemetry.h makes sense.
The patch is already reviewed/superreviewed but since I was going through it and had few comments, I will post them here. 

1. Some of the methods can be made const. ex: getTelemetry, IsActive.
2. The macros do this: if(t && t->isActive()) ..... if telemetry is passed as GetTelemetry() (as mentioned in the example in comment), it will expand to if(GetTelemetry() && GetTelemetry()->isActive()) and then GetTelemetry()->WriteValue(...). Macros should probably store the telemetry passed to the macro in a local var?
Ruchi: thanks for the feedback.  Let's file a bug for the follow-up items.  Landing this has been delayed too long already and I'd rather take care of the landing first since it needs to be coordinated with FRR.  Sound okay?

(As for the follow-ups themselves: (1) I agree with making methods const where we can.  For (2), I'm divided as to whether it is good practice to introduce local variable bindings within C macros, probably because of my exposure to hygenic macro systems a la Scheme.  I agree that the redundant invocation of GetTelemetry() is bad; I just wonder whether it would be simpler to change the example to bind the GetTelemetry() result outside fo the macro invocation.)
(In reply to Felix S Klock II from comment #38)
> Ruchi: thanks for the feedback.  Let's file a bug for the follow-up items. 
> Landing this has been delayed too long already and I'd rather take care of
> the landing first since it needs to be coordinated with FRR.  Sound okay?
> 
> (As for the follow-ups themselves: (1) I agree with making methods const
> where we can.  For (2), I'm divided as to whether it is good practice to
> introduce local variable bindings within C macros, probably because of my
> exposure to hygenic macro systems a la Scheme.  I agree that the redundant
> invocation of GetTelemetry() is bad; I just wonder whether it would be
> simpler to change the example to bind the GetTelemetry() result outside fo
> the macro invocation.)

My comments should not stop the landing of the patch. 
I am not sure that even if the example is changed, it will help. Someone calling the macros later might do the same. 
For the case here, it should be fine inside the do.. while()...  or may be we could just use inline functions than macros ?
Ruchi and Felix, thanks for your comments.

1. I agree that methods could be made const wherever possible. However, I don't see what one can do with a const ITelemetry object beyond calling IsActive, as methods to write metrics are not const.
2. I don't think a local variable is required, as the calls should get inlined. Also, the way you need to get ITelemetry* is not mandated by the macros (although the example shows a specific option). Of course, even the functions could be directly called, if necessary.
changeset: 7166:ed95f4eb68da
user:      Sateesh <sateesh@adobe.com>
summary:   Bug 602663: Revision 4 for VM-Telemetry interface changes (r=kpalacz,fklockii)

http://hg.mozilla.org/tamarin-redux/rev/ed95f4eb68da
changeset: 7168:55816669a217
user:      Sateesh <sateesh@adobe.com>
summary:   Bug 602663: move telemetry out of avmplus namespace (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/55816669a217
changeset: 7169:293859694976
user:      Felix S Klock II <fklockii@adobe.com>
summary:   Bug 602663: generated code for Telemetry feature (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/293859694976
Duplicate of this bug: 683708
Comment on attachment 588360 [details] [diff] [review]
Revision 4 for VM-Telemetry interface changes

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

Looks fine but obsolete now I think.
Status: UNCONFIRMED → RESOLVED
Closed: 10 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.