724139 - Add new telemetry based sampler into VM

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Profiler, defect)

Description

[George Comninos]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111220165912

Steps to reproduce:

Created a new sampling profiler based on telemetry. This sampler works by examining the MethodFrame stack at the sample interval. Samples are stored in a buffer and sent over telemetry when the buffer fills up.

Comment 1

[George Comninos]

Attachment: Telemetry based sampler implementation

Patch includes all the code needed for the new sampler implementation in the VM. Note that this sampler relies on a telemetry implementation (using the ITelemetry interface). All the relevant code is wrapped in the VMCFG_TELEMETRY_SAMPLER flag, which is controlled by AVMFEATURE_TELEMETRY_SAMPLER. The main sampler class is in the existing Sampler.cpp/.h files, in hopes that eventually it can just replace the existing sampler.

Comment 2

[George Comninos]

Attachment: New patch file

Add new patch file.

Comment 3

[George Comninos]

Attachment: Updated patch with minor fixes for smapler code

Updated patch file with some minor fixes for compilation on iOS.

Comment 4

[Ruchi Lohani]

Comment on attachment 596742 [details] [diff] [review]
Updated patch with minor fixes for smapler code

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

Other comments:
1. Whitespace issues (VM uses spaces instead of tabs)
2. For comments on top of methods the general rule followed is that the comments start with capital letters 
3. I dont see sampler being set for GC
4. VMPI_startIntWriteTimer makes the OSDepWin.cpp and OSDepPosix.cpp redundant, though I have more comments in this regard below.

These are partial review comments. I haven't reviewed all the files yet. 
Will post more comments once that's done.

::: VMPI/GenericPortUtils.cpp
@@ +72,5 @@
>      return logFunc;
>  }
> +
> +// -------------------------------------------------------------------------------
> +// Telemetry Sampler additions

I dont think we should have Telemetry specific comments here.

@@ +93,5 @@
> +	data->writeLock = writeLock;
> +	data->totalTicks = 0;
> +	data->lastTime = getCurrentTime();
> +	data->lowestTimerInterval = 9999999;
> +	for(int j = 0; j < numTimerIntervals; j++) {

Could use VMPI_memset here.

::: VMPI/PosixPortUtils.cpp
@@ +178,5 @@
> +		VMPI_recursiveMutexLock(data->writeLock);
> +		*addr = *addr + 1;
> +		VMPI_recursiveMutexUnlock(data->writeLock);
> +	}
> +	

Should call pthread_exit()

@@ +186,5 @@
> +// starts the interval timer
> +uintptr_t VMPI_startIntWriteTimer(uint32_t micros, unsigned int *addr, vmpi_mutex_t* writeLock)
> +{
> +	pthread_t p;
> +	PosixIntWriteTimerData *data = new PosixIntWriteTimerData();

Though I think new/delete will not be a problem but may be better to use VMPI_alloc/Free ?

::: VMPI/VMPI.h
@@ +211,5 @@
>  */extern uint64_t   VMPI_getPerformanceCounter();
>  
>  /**
> +* Telemetry Sampler additions
> +*/

I do not think that the VMPI_ api for the timer is generic since the median facility makes it specific to sampler. In general will I need my timer to do the work done in updateMedianIntervalInfo() on every tick?

::: VMPI/WinPortUtils.cpp
@@ +414,5 @@
> +	delete ((WinIntWriteTimerData *)data);
> +}
> +
> +// -------------------------------------------------------------------------------
> +// End of Telemetry Sampler

This comment doesnt sound right here. If this timer is telemetry specific (which looks the case to me due to the calculate median facility, then VMPI is probably not the right place for it.

::: core/AvmCore-inlines.h
@@ +694,5 @@
>  {
> +	#ifdef VMCFG_TELEMETRY_SAMPLER
> +	// check if we should take a sample
> +	if (core->sampleTicks)
> +		core->takeSample();

Can we move this code into a separate function. Is there a specific reason to keep sampleTicks in AvmCore? Can we keep it in the TelememetrySampler and provide a wrapper such as sampleCheck() in TelemetrySampler which does something like this:
////
if(sampleTicks)
     takeSample();
////
and AvmCore can call that wrapper?

::: core/AvmCore.h
@@ +569,5 @@
> +		/** the number of ticks that have passed since the last sample */
> +		unsigned int sampleTicks;
> +
> +		/** indicates whether the sampler is currently enabled or not */
> +		bool		 samplerEnabled;

Where are we turning the sampler on (setting samplerEnabled to true) ?

@@ +962,5 @@
>  
> +#ifdef VMCFG_TELEMETRY_SAMPLER
> +		static void FASTCALL takeSampleWrapper(AvmCore *theCore);
> +		void FASTCALL takeSample();
> +		TelemetrySampler* GetSampler();

GetSampler() is not named consistently. Should probably be named getSampler.

::: core/Sampler.cpp
@@ +759,5 @@
> +	void TelemetrySampler::avmCoreConstructorHook() 
> +	{
> +		m_telemetry = m_core->getTelemetry();
> +		if (m_core->samplerEnabled && !m_timerStarted) {
> +			m_timerData = VMPI_startIntWriteTimer(1000, &m_core->sampleTicks, &m_counterLock);

Constant (1000) should be made an enum or const in TelemetrySampler class

::: core/Sampler.h
@@ +292,5 @@
> +	// sample definition
> +	typedef struct Sample
> +	{
> +		// the frames, i.e. the stack
> +		sample_frame_t frames[SAMPLE_MAX_STACK_DEPTH];

Memory is allocated in TelemetrySampler even if sampler is not enabled. Can we make this allocation dynamic may be in avmCoreConstructorHook.

@@ +317,5 @@
> +	} SamplesBuffer;
> +
> +public:
> +	TelemetrySampler(AvmCore* core);
> +	~TelemetrySampler();

Should TelemetrySampler destructor be virtual?

@@ +332,5 @@
> +	// should be called when the AvmCore is destroyed, performs cleanup
> +	void avmCoreDestructorHook();
> +
> +	// called from the host when playing starts
> +	void DoPlayStart();

DoPlayStart is not named consistently. method name should start with a small letter.

@@ +344,5 @@
> +	SamplesBuffer m_samplesBuffer;
> +	bool m_timerStarted;
> +	vmpi_mutex_t m_counterLock;
> +	uintptr_t m_timerData;
> +	MMgc::GCHashtableBase<int, MMgc::GCHashtableKeyHandler, MMgc::GCHashtableAllocHandler_VMPI> /*<sample_frame_t, int>*/ * m_mappedMethods;

Please remove commented out code.

::: core/avmfeatures.h
@@ +1221,5 @@
>  #if AVMFEATURE_TELEMETRY
>  #  define VMCFG_TELEMETRY
>  #endif
> +#if AVMFEATURE_TELEMETRY_SAMPLER
> +#  define VMCFG_TELEMETRY_SAMPLER

This file is a generated file. You should modify avmfeatures.as for the feature defines. Also, since AVMFEATURE_TELEMETRY is required to be defined for AVMFEATURE_TELEMETRY_SAMPLER, we should add a <requires> tag for it. See AVMFEATURE_OSR as an example in avnfeatures.as.

Comment 5

[Ruchi Lohani]

Comment on attachment 596742 [details] [diff] [review]
Updated patch with minor fixes for smapler code

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

::: core/Sampler.cpp
@@ +781,5 @@
> +			 curFrame != NULL;
> +			 curFrame = curFrame->next) {
> +			MethodEnv* env = curFrame->env();
> +			if (env && env->method) {
> +				if (nFramesWritten >= len) {

Will nFramesWriten ever be > len here?

@@ +812,5 @@
> +		
> +		Sample* curSample = m_samplesBuffer.samples + m_samplesBuffer.nSamples;
> +		int nFrames = takeStackSample(curSample->frames, SAMPLE_MAX_STACK_DEPTH);
> +		if (nFrames > SAMPLE_MAX_STACK_DEPTH) {
> +			// TODO something better here

What do we want to do here in case there are more frames than the MAX sample frames. Should we treat them as sample with MAX frames? Rightnow we are just skipping them.

@@ +899,5 @@
> +			methodNameMapBuffer << idToStr;
> +
> +			methodNameMapBuffer << "=";
> +			methodNameMapBuffer << StUTF8String(sampleFrameToString(frame)).c_str();
> +			methodNameMapBuffer << ",";

Why cant we do this in the first for loop where we add the methodId to the m_mappedMethods? That way we dont need this loop as well as the local lists. I am guessing this is sent to create meaningful mappings?

::: utils/nativegen.py
@@ +2756,5 @@
> +        if ret_ctype != CTYPE_VOID:
> +            ret_result = TYPEMAP_THUNKRETTYPE[ret_ctype] % "ret";
> +        else:
> +            ret_result = "undefinedAtom";
> +        out.println("return %s;" % ret_result)

Can you upload the changes in the generated files. I can get better idea of what changed from that.

Comment 6

[Ruchi Lohani]

Comment on attachment 596742 [details] [diff] [review]
Updated patch with minor fixes for smapler code

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

::: core/Sampler.cpp
@@ +835,5 @@
> +		DataList<int> methodIdsToMap(m_core->GetGC(), kListInitialCapacity);
> +
> +		if (!m_mappedMethods) {
> +			m_mappedMethods = new MMgc::GCHashtableBase<int, MMgc::GCHashtableKeyHandler, MMgc::GCHashtableAllocHandler_VMPI>();
> +			m_numMappedMethods = 1;

Can we inline m_mappedMethods in TelemetrySampler rather than keeping a reference there?

Comment 7

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

(In reply to Ruchi Lohani from comment #5)
> ::: utils/nativegen.py
> @@ +2756,5 @@
> > +        if ret_ctype != CTYPE_VOID:
> > +            ret_result = TYPEMAP_THUNKRETTYPE[ret_ctype] % "ret";
> > +        else:
> > +            ret_result = "undefinedAtom";
> > +        out.println("return %s;" % ret_result)
> 
> Can you upload the changes in the generated files. I can get better idea of
> what changed from that.

(I was surprised to see this request, we don't usually require generated code to be included in patch reviews.)

In any case, I don't object to providing changes to generated code in patch form on the bug ticket, but I do recommend keeping it in a separate patch so that people who want to focus on the core changes can continue to look at the original patch (and subsequent fixes to it).

Comment 8

[George Comninos]

Attachment: Sample showing diff of generated nativegen.py

Add a simple diff of one method in avmglue.cpp (generated file from nativegen.py) to illustrate changes to the python script. All generated methods have the same change, I just attached one method since the avmglue.cpp generated file is huge.

Comment 9

[George Comninos]

Attachment: New updated sampler code patch

Submitted new patch file with changes based on feedback as well as some more cleanup, changes include:
- got rid of tabs, extra space cleanup
- got rid of TelemetrySampler::DoPlayStart()
- renamed avmCoreConstructore/DestructorHook to start() / stop()
- don't pre-allocate large chunks of memory
- fixed up avmfeatures.as to generate the defines we need
- added code to handle stacks deeper than maximum depth allowed by sampler, now it sends an ID indicating stack was to deep, sends first 128 frames
- cleaned up flushSamples() code, more efficient

Comment 10

[Ruchi Lohani]

Comment on attachment 599737 [details] [diff] [review]
New updated sampler code patch

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

Edwin has the final say on whether VMPI.h is a good place for the sampler timer given that it also provides specific functionality (median calculation) for sampler. 
Patch looks fine to me overall. I have few more comments. 
R+ on a condition that those comments are considered and fixes are made as necessary now than leaving it for future.

::: VMPI/GenericPortUtils.cpp
@@ +71,3 @@
>  {
>      return logFunc;
>  }

Note: All the existing functions in this file start with an uppercase.

::: core/Sampler.cpp
@@ +770,5 @@
> +            MethodEnv* env = curFrame->env();
> +            if (env && env->method) {
> +                if (nFramesWritten >= len) {
> +                    // return 1 greater than the max length so we know there is more..
> +                    return ++nFramesWritten;

I think we should do this here (no functional difference but looks more clean to me):
if(nFramesWritten == SAMPLE_MAX_STACK_DEPTH) {  // more stack frames than SAMPLE_MAX_STACK_DEPTH.
    return nFramesWritten+1;
}
else ....  

Also, we can probably avoid passing len to this method? We can just check nFramesWritten against SAMPLE_MAX_STACK_DEPTH in takeStackSample().

@@ +833,5 @@
> +            TELEMETRY_DOUBLE(m_telemetry, ".sampler.nextSampleTime", sample->timestamp);
> +            TELEMETRY_UINT32(m_telemetry, ".sampler.nextSampleTicks", sample->nTicks);
> +            
> +            char idToStr[100];
> +            uint32_t* stackArray = mmfx_new_array(uint32_t, sample->nFrames);

Could we allocate it on stack here?

@@ +836,5 @@
> +            char idToStr[100];
> +            uint32_t* stackArray = mmfx_new_array(uint32_t, sample->nFrames);
> +
> +            // For each frame on the stack...
> +            for (int j = 0; j < sample->nFrames; j++) {

Can we do something like this:

//////
// SMSD = SAMPLER_MAX_STACK_DEPTH
int numFrames = (nFrames > SMSD) ? SMSD : nFrames;
for( j = 0; j < numFrames; j++)
{
   frame = sample->frames[j];
   .......
   stackArray[j] = methodId;
}

if(nFrames > SMSD)       // more frames than we handle
   stackArray[j] = SAMPLER_STACK_OVERFLOW_ID  // overflow ID

////////

Doing this, will eliminate the if-else check inside the loop every time. 

Also, please declare the methodId and frame locals outside loop.

::: core/Sampler.h
@@ +303,5 @@
> +            // the frames, i.e. the stack
> +            sample_frame_t frames[SAMPLE_MAX_STACK_DEPTH];
> +            
> +            // number of frames in the stack
> +            int nFrames;

Ideally I would have thought that we store the condition of overflow in the sample itself as a bool but may be that will add some unnecessary memory to the sample buffer. But I think we are trying to make use of the nFrames value for that. 

I this we should document the conditions on nFrames here. nFrames will actually be one more than the actual frames written in case of stack frame overflow condition.

::: core/avmfeatures.as
@@ +643,5 @@
>    </feature>
>  
>    <feature>
> +    <desc> Select support for Telemetry based sampler, requires a Telemetry implementation 
> +           (to be used in player) </desc>

We should not use 'player' here. 'host' may be better. I do notice that AVMFEATURE_TELEMETRY also uses keyword 'player' in its desc text.

Comment 11

[George Comninos]

Attachment: New updated sampler code patch

Updated patch with latest changes based on feedback.

Comment 12

[George Comninos]

Attachment: New updated sampler code patch

Updated patch to fix warnings on Windows.

Comment 13

[George Comninos]

> +            uint32_t* stackArray = mmfx_new_array(uint32_t, sample->nFrames);

> Could we allocate it on stack here?

No, because size is not constant, which generates a compiler error on Windows (and possibly other compilers, it's not part of standard C++).

Comment 14

[Ruchi Lohani]

Comment on attachment 600191 [details] [diff] [review]
New updated sampler code patch

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

::: core/Sampler.cpp
@@ +847,5 @@
> +                if (methodId == 0) {
> +                    methodId = m_numMappedMethods++;
> +
> +                    // add to the method name map
> +                    sprintf(idToStr, "%u", methodId);

Do we need this sprintf here? StringBuffer/PrintWriter has << operator overloaded for int32_t.

::: core/Sampler.h
@@ +305,5 @@
> +
> +            // number of frames in the stack.
> +            // if we have exceeded SAMPLE_MAX_STACK_DEPTH for a particulat stack, 
> +            // this will equal SAMPLE_MAX_STACK_DEPTH + 1 and the last frame in
> +            // the frames array above will have a value of SAMPLER_STACK_OVERFLOW_ID

The comment looks wrong to me since the last frame in the frames array will still be a valid frame even in case of overflow, correct?

Comment 15

[George Comninos]

Attachment: New updated sampler code patch

You're right, the sprintf is not needed, and I fixed the comment.

Comment 16

[George Comninos]

Attachment: New updated sampler code patch

Fixed signed/unsigned mismatch warnings for linux build.

Comment 17

[Tamarin Bot]

changeset: 7229:f84ca7c71f86
user:      Ruchi Lohani<[e-mail]>
summary:   Bug 724139 - Add new telemetry based sampler into VM (p=gcomnino, r=rulohani)

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

Comment 18

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

Xcode (version 3.2.6) is unhappy with this in the avmshell.xcodeproj, says:

  core/Sampler.cpp:899:34: error: no newline at end of file

I will fix.

(This does make me wonder about whether the avmshell Xcode project file is still in active use.  It would be nice to retire projects if they are not used by any active team members.)

Comment 19

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

(In reply to Felix S Klock II from comment #18)
> (This does make me wonder about whether the avmshell Xcode project file is
> still in active use.  It would be nice to retire projects if they are not
> used by any active team members.)

(Retiring Xcode projects is of particular interest, because unlike Visual Studio projects which have a relatively nice text format, it seems like the only reasonable way to maintain Xcode projects is by running the GUI Xcode.app itself, and by producing changesets that often have semi-random appearing changes to the file that make the diff's noisy.)

Comment 20

[Tamarin Bot]

changeset: 7248:e17135a38915
user:      Felix S Klock II <[e-mail]>
summary:   Bug 724139: fix line ending to appease xcode (r=fklockii).

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

Comment 21

[Edwin Smith]

Comment on attachment 600560 [details] [diff] [review]
New updated sampler code patch

delegating SR

Comment 22

[George Comninos]

Attachment: Additional fixes for sampler

Attached additional patch with these minor changes:
 - we don't do anything in flushSamples() if there are no samples, no allocation of temp objects etc
 - we only send the sample median interval when it has changed
 - turn off the extra sample check and take sample code in MethodFrame::enter/exit for AOT builds. This adds overhead in every call when using AOT, we now have a separate abcOP_enterMethodFrameWithSampler() in AOTStubs.cpp to handle this for AOT, which is only used when the sampler is enabled, specified at build time

Comment 23

[George Comninos]

Attachment: Additional fixes for sampler

Update additional fixes with one minor addition, make TelemetryMethod destructor virtual and member protected.

Comment 24

[George Comninos]

Attachment: Additional fixes for sampler

Updated patch with just more comments in AvmCore-inline.h, as per review request from Stan Switzer.

Comment 25

[Tamarin Bot]

changeset: 7304:9f7989340770
user:      Felix Klock II <[e-mail]>
summary:   Bug 724139: Add new telemetry based sampler into VM (p=gcomnino, r pending=rulohani).

http://hg.mozilla.org/tamarin-redux/rev/9f7989340770

Comment 26

[Tamarin Bot]

changeset: 7304:9f7989340770
user:      Felix Klock II <[e-mail]>
summary:   Bug 724139: Add new telemetry based sampler into VM (p=gcomnino, r pending=rulohani).

http://hg.mozilla.org/tamarin-redux/rev/9f7989340770

Comment 27

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

Attachment: patch F v1: (overly?) naive fix for AOT builds

George: The AOT builds on our end were broken by CL 1044829.  (The Actionscript QE team has details, as do I).

One way to replicate the build failure prior to putting in this patch:

  % xcodebuild -sdk iphoneos4.3 -project AIR.cocoatouch.xcodeproj  \
    -configuration Release -target 'SDK with iPhoneOS AVMShell'

Anyway, this is one naive fix for the problem; in particular, these guards might not belong here (perhaps the whole method definition should be commented out and any references to it be instead guarded by the preprocessor symbols.  Or perhaps we should unconditionally expose the state in AvmCore to keep ourselves sane.)

Please give this patch a double-check when you can, since I intend to include it (or whatever replaces it) in the next Tamarin integration to FlashRuntime Main.

Comment 28

[George Comninos]

Ah you're right, the #ifdef should be there, and yes in fact, they can go around the entire abcOP_enterMethodFrameWithSampler() and abcOP_exitMethodFrameWithSampler() methods. The only reference to it currently is from the LLVM emitter code.

Comment 29

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

(In reply to George Comninos from comment #28)
> Ah you're right, the #ifdef should be there, and yes in fact, they can go
> around the entire abcOP_enterMethodFrameWithSampler() and
> abcOP_exitMethodFrameWithSampler() methods. The only reference to it
> currently is from the LLVM emitter code.

Okay, I'll interpret that as an R+, with a suggestion to switch to the broader guard (assuming it still fixes the problem).

Comment 30

[Tamarin Bot]

changeset: 7306:b247cf232932
user:      Felix Klock II <[e-mail]>
summary:   Bug 724139: fix AOT builds broken by CL 1044829 (p=fklockii, r pending=gcomnino)

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

Comment 31

[Tamarin Bot]

changeset: 7307:95f95ff1d580
user:      Felix Klock II <[e-mail]>
summary:   Bug 724139: refine fix from CL@1045051 based on review comments (r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/95f95ff1d580

Comment 32

[Brent Baker]

The changes made to nativegen.py are causing the float enabled shells to fail to compile. For information on how to compile the float shells:
https://zerowing.corp.adobe.com/display/FlashPlayer/Float%2C+Float4+Project+Page


../generated/builtin.cpp: In function ‘float __vector__ avmplus::NativeID::flash_utils_ByteArray_readFloat4_sampler_thunk(avmplus::MethodEnv*, uint32_t, avmplus::Atom*)’:
../generated/builtin.cpp:4540: error: ‘flash_utils_ByteArray_readFloat4_thunk’ was not declared in this scope
..
..
..

Comment 33

[George Comninos]

Attachment: Fix for Float support in nativegen.py

The problem is that prototypes don't get generate for methods with a float return type (emitThunkProto checks if(thunk_ret_ctype != CTYPE_FLOAT4)), not sure why that is, but since the sampler thunk was generated before the native thunk for that method it can't find the declaration. A simple fix is to reverse the order of the sampler thunk and native thunk, patch for this attached.

Comment 34

[Brent Baker]

Comment on attachment 608412 [details] [diff] [review]
Fix for Float support in nativegen.py

+1, am not able to compile the float enabled shell again

Comment 35

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

(In reply to Brent Baker from comment #34)
> Comment on attachment 608412 [details] [diff] [review]
> Fix for Float support in nativegen.py
> 
> +1, am not able to compile the float enabled shell again

sed -e 's/not/now/' ?

Comment 36

[Brent Baker]

(In reply to Felix S Klock II from comment #35)
> (In reply to Brent Baker from comment #34)
> > Comment on attachment 608412 [details] [diff] [review]
> > Fix for Float support in nativegen.py
> > 
> > +1, am not able to compile the float enabled shell again
> 
> sed -e 's/not/now/' ?

Sorry, you sed command is correct:

+1, am now able to compile the float enabled shell again

Comment 37

[Tamarin Bot]

changeset: 7321:de88e6eec33e
user:      Brent Baker <[e-mail]>
summary:   Bug 724139: Fix for Float support in nativegen.py (p=gcomnino, r+brbaker, sr+fklockii)

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

Comment 38

[Tamarin Bot]

changeset: 7327:bf1e80ae22d0
user:      Felix Klock II <[e-mail]>
summary:   Bug 724139: updates to generated code from nativegen.py changes (r=fklockii).

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

Comment 39

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

Comment on attachment 600560 [details] [diff] [review]
New updated sampler code patch

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

Similar to Ruchi's note in comment 4, I am not 100% convinced all of this code belongs in the VMPI subdirectory.  It would be nice to check if we could refactor it so the platform-specific things live in VMPI/, while the next layer up that is built on top of the platform-specific kernel would live somewhere higher up, such as vmbase/.  (But I can see that such a refactoring is not trivial or obvious, given how there is some intermingling of the platform-dependent extensions of IntWriteTimerData, for some reason).

I haven't finished my review yet.  My feeling is that this code probably does something useful for its client, but I wouldn't want to be put in charge of maintaining it.  So there are follow-up work items to clean up the documentation.

I'm posting my comments so far.  Will hopefully finish the review on Monday.

::: VMPI/GenericPortUtils.cpp
@@ +86,5 @@
> +    data->addr = addr;
> +    data->writeLock = writeLock;
> +    data->totalTicks = 0;
> +    data->lastTime = GetCurrentSystemTime();
> +    data->lowestTimerInterval = 9999999;

Only half-silly question: is this definitely small enough?

Slightly more seriously: using this number made me wonder if it was somehow related to a hypothetical 10000000 value elsewhere, that I immediately started searching for.  But I assume your real intent is to use a large uint64_t.  Is UINT64_MAX available generally?  (If not, perhaps we should add it to our headers.)

@@ +110,5 @@
> +        uint32_t accumulator = 0;
> +        uint32_t halfTotalTicks = timerData->totalTicks / 2;
> +        uint64_t interval = timerData->lowestTimerInterval; // max is numTimerIntervals
> +        while(accumulator < halfTotalTicks) {
> +            accumulator += timerData->timerIntervalCounts[interval++];

I assume there is some invariant ensuring that there are at least halfTotalTicks positive entries in timerIntervalCounts.  This sort of representation invariant should be stated explicitly somewhere, especially since it is, IIUC, guarding here against both an infinite loop and an access off the end of an array.

As noted above, it is not crystal clear what the representation is in the timerIntervalCounts, and it is likewise not obvious to me what the specification for this procedure VMPI_calculateMedianTimerInterval is.  For these two reasons, I cannot determine if this code is correct.  I just have to trust that its doing whatever the Sampler needs from it.  That's not a happy place to be.

::: VMPI/PosixPortUtils.cpp
@@ +148,5 @@
> +    pthread_t thread;
> +};
> +
> +extern void Init_IntWriteTimerData(IntWriteTimerData* data, uint32_t micros, unsigned int *addr, vmpi_mutex_t* writeLock);
> +extern void UpdateMedianIntervalInfo(IntWriteTimerData *data);

* I'm not thrilled about the extern declarations in PosixPortUtils.cpp rather than putting them into a shared header, in order to catch the case where the main function signature changes.  There are some precedents for other extern declarations, but I think they are exceptional cases that should be cleaned up, not duplicated.  Still, fixing this can be a clean-up item we schedule for later.

::: VMPI/VMPI.h
@@ +223,5 @@
> +    // Data for calculating the median interval
> +    uint32_t totalTicks;
> +    uint64_t lastTime; // last time interval fired
> +    uint64_t lowestTimerInterval;
> +    uint32_t timerIntervalCounts[numTimerIntervals];

* There is not enough description of what abstraction this is representing and how it represents it.  In particular:
-- What is totalTicks keeping track of?  Is it something machine specific?  (I'm worried about potential confusion with the ticks that are referenced in the GC code and in vprof.)  I recommend that you choose a slightly longer phrase (i.e. "sampler ticks" with the first occurrence of the word "ticks" in each relevant spot in the code, and also put a full definition of what "sampler ticks" are in one place.  The references to tick count elsewhere that I noted (above VMPI_calculateMedianTimerInterval, above sampleTicks in AvmCore, and above nTicks in TelemetrySampler::Sample) but I have no idea who controls those ticks from the comments here.  From the code, it looks like it is only updated by UpdateMedianIntervalInfo, on a background thread; I don't want people to have to go searching for that, since I assume you intend a more abstract interpretation anyway.
-- Explain what mapping is represented by the timerIntervalCounts array: what are the keys that it is counting?  (I do not want to have to infer this abstraction from the code; there should be a short description here, and then the code implements that.)  If gcomnino cannot provide this in the short term, then I will allocate a bugzilla ticket to introduce such documentation.  (i.e., this is not a blocking issue.)

@@ +224,5 @@
> +    uint32_t totalTicks;
> +    uint64_t lastTime; // last time interval fired
> +    uint64_t lowestTimerInterval;
> +    uint32_t timerIntervalCounts[numTimerIntervals];
> +} IntWriteTimerData;

Hmm.  I understand that the IntWriteTiemrData is shared amongst the implementations since they all need this structure.  But VMPI clients are not meant to know about it or cast down to it, right?  (That is, they should treat the return type from VMPI_startIntWriteTimer as an opaque pointer, right?)

(I ask because I'm wondering if we should make a separate header for definitions that are shared internally within VMPI but not used externally; if we did, then this would be a candidate for going there.)

Anyway, its not a blocking issue.

@@ +246,5 @@
> +extern void         VMPI_stopIntWriteTimer(uintptr_t data);
> +
> +/**
> + * Calculates the median timer interval. There must have been at least 100
> + * ticks before this makes a calculation.

How is a client supposed to check that they can legally invoke this?  I do not see a way to observe the number of ticks, and the number of ticks, from the code, appears to be controlled by a concurrently running thread?

If the intention is that I can call this routine, but it will just return 0 if insufficient ticks have passed, that should be made more explicit.

::: core/Sampler.h
@@ +306,5 @@
> +            // the frames, i.e. the stack
> +            sample_frame_t frames[SAMPLE_MAX_STACK_DEPTH];
> +
> +            // number of frames in the stack.
> +            // if we have exceeded SAMPLE_MAX_STACK_DEPTH for a particulat stack, 

* typo: particulat ==> particular

@@ +356,5 @@
> +        vmpi_mutex_t m_counterLock;
> +        uintptr_t m_timerData;
> +
> +        // map of <sample_frame_t, unsigned int> key/vaue pairs for the methods we have already mapped and a unique id for each one
> +        MMgc::GCHashtableBase<unsigned int, MMgc::GCHashtableKeyHandler, MMgc::GCHashtableAllocHandler_VMPI> m_mappedMethods;

* Regarding the decision to use MMgc::GCHashtableAllocHandler_VMPI rather than the default (GCHashtableAllocHandler_new): I take it that the Sampler is not worried about the OOM scenario described in the comments in GCHashtable.h?  Or is there some sort of chicken/egg issue preventing you from using the default?

::: utils/nativegen.py
@@ +2852,1 @@
>          out.indent -= 1

(i am rubber-stamping the nativegen.py changes.)

Comment 40

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

(some of my comments were reordered by Splinter, so I occasionally referred to comments "above" or "below" that are no longer in the same relative position.  Sorry about that.)

Comment 41

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

Comment on attachment 600560 [details] [diff] [review]
New updated sampler code patch

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

SR+.

I didn't rush to get this review done because I knew the code had already landed and I was relatively confident that it was sound.  (That is, please do not use this instance as an example of the VM team dragging its feet on reviews.)

Having said that, I did note some follow-up work items with respect to documentation (namely for the state used for calculating the median interval, i.e. the timerIntervalCounts array in VMPI.h), and also a potential refactoring of where the pieces fall in our infrastructure (i.e., maybe all of that interval stuff should live in either AVMPI/ or in vmbase/).  Neither are blocking issues though.

::: VMPI/WinPortUtils.cpp
@@ +363,4 @@
>      return pagesize;
>  }
>  
> +// Timer data, Windows specific data

Rubber-stamping Windows specific changes.

::: core/AvmCore-inlines.h
@@ +711,2 @@
>  
>  REALLY_INLINE void MethodFrame::enter(AvmCore* core, CodeContext* cc)

I know very little about the sampler architecture.  Is there potentially useful/interesting information being discarded here by not passing along in the takeSample() method, namely the information that the sample is at the beginning or the end of the method call?  Anyway, that's really a quick throw-away question; I'll trust that this instrumentation serves your purposes as you like.

::: core/Sampler.cpp
@@ +763,5 @@
> +            m_samplesBuffer = NULL;
> +        }
> +    }
> +    
> +    unsigned int TelemetrySampler::takeStackSample(sample_frame_t* outFrameBuffer)

rubber stamping Sampler.cpp changes.

Comment 42

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.