766714 - Tamarin sampler needs to support new Sampler API's.

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Profiler, defect)

Description

[Mark Shepherd]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_8) AppleWebKit/536.11 (KHTML, like Gecko) Chrome/20.0.1132.34 Safari/536.11

Steps to reproduce:

We need to refactor the "telemetry sampler" so that all timing, triggering, collecting, and reporting of sampler data is handled by a new host library that can support multiple VMs and hosts. This new host library does not live in the AVM codebase, and supercedes all the code related to telemetry sampler that used to live in Sampler.h and Sampler.cpp. The AVM is now only responsible to support a small "sampler target" API of 6 functions, which is documented in AvmCore.h and implemented in AvmCore.cpp. The sampler code in the MethodFrame stack and the JITter remains unchanged, since that is the core functionality that the VM must still provide.

Comment 1

[Mark Shepherd]

Attachment: A patch that address this issue.

Comment 2

[Ruchi Lohani]

Comment on attachment 635064 [details] [diff] [review]
A patch that address this issue.

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

::: users/FlashRuntime/mshepherd/Main/code/third_party/avmplus/core/AvmCore.cpp.~1~
@@ +5433,4 @@
>      // the sampler is enabled.
>      void AvmCore::takeSample()
>      {
> +        if (sampler) sampler->takeSample();

Is all this code wrapped inside #ifdef VMCFG_TELEMETRY_SAMPLER? Also, is takeSample going to call AvmCore::clearSampleRequest() ?

@@ +5442,5 @@
>      {
> +        if (theCore->sampler) theCore->sampler->takeSample();
> +    }
> +
> +    /* Implementation of ISamplerTarget... */

This comment somehow looks wrong. Whats ISamplerTarget? I did not find it.

@@ +5490,5 @@
> +
> +        return nFramesWritten;
> +    }
> +
> +    avmplus::Stringp AvmCore::functionHandleToString(telemetry::FunctionHandle handle)

I would be interested in seeing the usecases of this method. Rightnow FunctionHandle is a void* and we are assuming its a MethodInfo* everywhere.

Comment 3

[Mark Shepherd]

Attachment: A patch to address comment #2

Hi Ruchi, here is a new patch, and answers to your questions.

Is all this code wrapped inside #ifdef VMCFG_TELEMETRY_SAMPLER?
- yes. The #ifdef is just a couple of lines above the code shown in the splinter diff, around line 5431. 

Is takeSample going to call AvmCore::clearSampleRequest() ?
- yes, there's a comment in AvmCore.h that says "The host will always call this from within takeSample()"

This comment somehow looks wrong. Whats ISamplerTarget? I did not find it.
- Sorry that comment was obsolete. I've removed it.

I would be interested in seeing the usecases of this method. Right now FunctionHandle is a void* and we are assuming its a MethodInfo* everywhere.
- there was no need to be void*. I've changed it to MethodInfo*.
- I added some more comments to AvmCore.h: "from within takeSample(), the host can fetch the current call stack, in the form of an array of MethodInfo (which is an opaque token whose details are known only to the VM). From time to time, the host may call functionHandleToString() to fetch the human-readable string corresponding to a MethodInfo."

Comment 4

[Ruchi Lohani]

Comment on attachment 636812 [details] [diff] [review]
A patch to address comment #2

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

::: users/FlashRuntime/mshepherd/Main/code/third_party/avmplus/core/AvmCore.cpp.~1~
@@ +5465,5 @@
> +        return true;
> +    }
> +
> +    unsigned int AvmCore::recordCallStack(telemetry::FunctionHandle* outFrameBuffer, unsigned int maxDepth)
> +    {

Need an assert(AvmAssert) to make sure outFrameBuffer passed is not null.

@@ +5488,5 @@
> +    }
> +
> +    avmplus::Stringp AvmCore::functionHandleToString(telemetry::FunctionHandle handle)
> +    {
> +        return ((MethodInfo*)handle)->getMethodName();

The cast can go away now.

::: users/FlashRuntime/mshepherd/Main/code/third_party/avmplus/core/AvmCore.h.~1~
@@ +646,4 @@
>          bool            samplerEnabled;
> +
> +        /** one of these functions is called by the executing actionscript code,
> +            whenever that code notices that sampleTicks != 0. They just call sampler->takeSample() */

Spacing issue.

Comment 5

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

Comment on attachment 636812 [details] [diff] [review]
A patch to address comment #2

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

SR+.  Sorry for the delay Mark!

::: users/FlashRuntime/mshepherd/Main/code/third_party/avmplus/core/AvmCore.cpp.~1~
@@ +90,5 @@
> +    /*
> +        This exists solely to ensure that tamarin-security differs from tamarin-redux/tamarin-central,
> +        and that the canonical form for the MARK_SECURITY_CHANGE macro remains in place.
> +        Please do not ever remove this ifdef or this comment. (srj)
> +    */

You've injected some artificial changes here.  But the change appears to be removing tabs, so I'll go along with it.  (Normally I'd push to eschew injecting unrelated noise into a changelist.)

Comment 6

[Mark Shepherd]

FYI, this is now checked into perforce dolores mainline, changelist 1082966.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.