Closed Bug 554702 Opened 14 years ago Closed 6 years ago

Profiler should provide a way to get invocation count of pseudofunctions

Categories

(Tamarin Graveyard :: Profiler, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX
Q2 12 - Cyril

People

(Reporter: mike, Assigned: jbahuley)

References

Details

Attachments

(1 file, 3 obsolete files)

(For flash10.2)

The profiler provides the ability to track how much time was spent doing various pseudofunctions such as [mark], [sweep], and many more; but it does not provide a way to find out how many times each of these functions was called.

A reasonable solution would be to extend flash.sampler.getInvocationCount(obj, qname) so that if you pass obj==undefined and qname==QName(null, "[mark]"), it would return the invocation count for that pseudofunction.

To reproduce, compile and run this program.  The call to getInvocationCount() returns -1 for the pseudofunction "[reap]".

import flash.sampler.*;

function f() {
    // force a gc to happen by allocating lots of objects
    for (var i=0; i<100000; ++i)
        new Object();
}

// start sampling, and then force a gc to happen, so that a gc-related
// pseudofunction will be in the profiler results
startSampling();
f();
pauseSampling();

// find a pseudofunction in the profiler results
for each (var sample in getSamples()) {
    if (String(sample.stack).charAt(0) == '[') // is this a pseudofunction?
    {
        // get the pseudofunction name, and make a qname out of it
        var name = String(sample.stack[0]).replace( "()", "" );
        var qname:QName = new QName(null, name);

        // try getInvocationCount()
        var invocationCount = getInvocationCount(null, qname);
        trace(name ": getInvocationCount() returns " + invocationCount);
        break;
    }
}
Forgot to mention, the impact on the Flash Builder profiler is that it always shows every pseudofunction as having a call count of 1.  Even [enter-frame], which happens many many times, is shown with a call count of 1, which is misleading.
Assignee: nobody → mmoreart
Status: NEW → ASSIGNED
Flags: flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.2
Assignee: mmoreart → nobody
Component: Virtual Machine → Profiler
QA Contact: vm → profiler
I was thinking about the impact of this issue on the flash builder profiler. It seems to me that yes, the information which is currently shown by flash builder performance profiler for pseudo functions is misleading (count always reported 1). Though I am also wondering how much useful will for the developer if they have the correct information (correct invocation count)?

We can extend the sampler to make getInvocationCount return the correct count. We could probably maintain a hashtable of counts for each fake function and update it when the CallStackNode is created on the stack. Then in the getInvocationCount() we can return the count for these fake functions from this data structure in the sampler. This approach will only work if all the pseudofunctions use SAMPLE_FRAME and create a CallStackNode everytime they are called. I was wondering if we could somehow maintain the fake function names and their invocation count in one data structure. Currently the sampler maintains a List<Stringp> of fake function names.
Attached patch Patch (obsolete) — Splinter Review
I want to combine maintaining the fakeFunctionNames and fakeFunctionInvocationCounts in a single data structure in a way so that I can avoid calling hashtable 'get' and 'put' methods and simply increment the count in updateFakeFunctionCount(). Do you have any suggestions for it? 
Also currently getFakeFunctionName() is only called from CallStackNode ctor so calling updateFakeFunctionCount() in it would be fine otherwise it might be better to call it in CallStackNode ctor separately.
Assignee: nobody → rulohani
Attachment #462534 - Flags: superreview?(edwsmith)
Attachment #462534 - Flags: review?(treilly)
Comment on attachment 462534 [details] [diff] [review]
Patch

I don't see anything obviously wrong, so marking R+ as long as the issues below are addressed.  Since Tommy is out for several more days i'm redirecting that review request to Steven.

the hard coded constant "1024" in core/Sampler.cpp needs a justifying comment.  I presume the initial table size is based on experimentation?

Need to run utils/fixtabs to convert tabs->spaces.

From looking at the code, the only purpose for fakeMethodNames was to keep track of known names.  (ie, it implements a Set).  If that is still true, then a map from fake-name to invocation count can cover both use cases.

if more information about a name must be stored, then a map from name to struct would work.

The reuse of GCHashtable_VMPI seems fishy to me when we already have GCHashtable<K,V>; should you directly instantiate the template as GCHashtable<String*,uint64_t)?   I'm just not sure what the intention was (comments?)
Attachment #462534 - Flags: superreview?(edwsmith)
Attachment #462534 - Flags: superreview-
Attachment #462534 - Flags: review?(treilly)
Attachment #462534 - Flags: review?(stejohns)
Comment on attachment 462534 [details] [diff] [review]
Patch

Ditto for Edwin's question about reusing GCHashtable_VMPI; in particular, since it's really storing void*, we're limited to a 32-bit invocation count on 32-bit systems. If we need/want 64-bit counters, we need a different data structure. But do we need/want counters that large?

Question: if we have fakeFunctionInvocationCount, why do we even need fakeFunctionNames anymore? IIUC we only need fakeFunctionNames to ensure the strings don't go stale, and to implement isFakeFunction, both of which could be accomplished using fakeFunctionInvocationCount. (Wait, scratch that: GCHashtableVMPI just stores raw pointers, so it wouldn't serve the purpose of properly retaining the Strings. If Bug 568664 ever lands then we could combine them.)

Style nit: "Stringp m_fakename" is unacceptable for an argument name; "m_" should be used only for private member vars in our codebase.

Style nit: instead of 

        return (fakeFunctionNames.indexOf(name) < 0) ? false : true;

can't we just do

         return (fakeFunctionNames.indexOf(name) >= 0);
I think I forgot to mention that it as an initial patch. I do want to combine the string and count or may be a struct in a single data structure. Working on it currently. How about having a single hash table with the string keys and a struct for each fake function string the invocation count and the name Stringp (as Edwin suggested above)? It might be worth to keep the struct as fakefunction might have more info in future to be stored somewhere. These fake function structs will need to be deleted in ~Sampler(). 

Agree on m_fakename stupidity and also with the style nit.
I would want to avoid paying the overhead for future expansion if possible.  if the hashtable is <String*, FakeInfo>  // note, not FakeInfo*  then the struct will be inline in the hashtable, and the space required will be the same for  uint64_t vs struct { uint64_t count }. 

if we have to allocate each FakeInfo then i'd defer it until the struct has more than just a counter.  (adding a comment with intentions for the future is good enough).
fakeFunctionNames was used to ensure Strings dont get gc'ed. Once I removed the use of fakeFunctionNames and just rely on fakeFunctionInvocationCount for isfakeFunction, this assert :
#define NULL_OR_MARKED(_x) GCAssert(_x == NULL || GC::GetMark(_x))
started to fail in Sampler::presweep(). 
GCHashtable is currently not using the template for the hashtable value. Changing/improving the GCHashtable will need to be dealt with in a separate bug.

Could we use List<uint64_t, LIST_NonGCObjects> for the counts to avoid the data loss. Indexes of both the name in the fakeFunctionNames and the invocationCount List will be same for a fakename.
that would be an improvement certianly.  Still, earlier in the sampler code I noticed it calls list.indexOf(name) to test for name membership.  with lots of names, that's a lot of O(n) searching.  is the list typically small?  if not, a (name,count) hashtable is appropriate, and we just have to craft it to ensure it pins the name strings.
the List length currently is 35/36 (fake function count).
The invocation count can overflow the 32 bit hash table value. GCHashTable will need modification to support template for Hashtable Value. 

List solution will be accurate but will be slow as all invocation Count related function will be O(n). Though the list length for fake functions is less, updateFakeInvocationCount() is called as many times as the fake function is called.
Attachment #462534 - Attachment is obsolete: true
Attachment #462534 - Flags: review?(stejohns)
This patch had been waiting for the GCHashtable template changes.
Attachment #464087 - Attachment is obsolete: true
Attachment #471387 - Flags: superreview?(edwsmith)
Attachment #471387 - Flags: review?
Attachment #471387 - Flags: review? → review?(stejohns)
isFakeFunction() returns true only if the fake function has been called once -- not what I would have expected from the name. If this is as-intended, a better name might be in order.
Attachment #471387 - Flags: superreview?(stejohns)
Attachment #471387 - Flags: superreview?(edwsmith)
Attachment #471387 - Flags: review?(treilly)
Attachment #471387 - Flags: review?(stejohns)
Attachment #471387 - Flags: superreview?(stejohns) → superreview+
Would be cool if there was a get variant that returned a reference so we didn't have to do a get and put to update the value.
Comment on attachment 471387 [details] [diff] [review]
Patch using hashtable of fake invocation count of type uint64_t

It seems wrong for a function called getFakeFunctionName to cause an update on the invocation count.   Maybe we could call it enterFakeFunction or getAndEnter or something?   Withholding + for fixing that and the get/put performance issue.  Actually I'm kinda worried about performance even with that being fixed.   We want a SAMPLE_FRAME to be really low overhead.   One hashtable lookup is bad enough (the findInternedString call) b/c the intern table can get big.  What we really want here is the Sampler to have a const char * keyed table of FakeMethod objects which contain the Stringp that's interned and the invocation count.   That should be an optimization because then we'd be hitting the smaller fake name's only ht instead of the intern table.   The Stringp's need to be reachable by the GC, that's how they stay in the intern table.
Attachment #471387 - Flags: review?(treilly) → review-
And of course the List<Stringp> of names goes away
Patch with modifications:

- Implemented hashtable getRef method, though I am not clear why find returns the initial hash index even if the key is not found. Is that as designed? I did checks in getRef for: the index returned in find actually has the key that it was called with. If not, I return Null as the value reference. 
- changed the name of getFakeFunctionName to enterFakeFunction() as it does more than just returning the name of the function.
- changed updateFakeFunctionInvocationCount() to use hashtable getRef.
- changed isFakeFunction(), its return value was misleading. 

findInternedString call remains there in enterFakeFunction as it was before. Will handle that optimization in another bug later.
Attachment #471387 - Attachment is obsolete: true
Attachment #505677 - Flags: superreview?(stejohns)
Attachment #505677 - Flags: review?(treilly)
Comment on attachment 505677 [details] [diff] [review]
Review feedback changes

style nit: "const void * k" is kinda weird; existing code is generally cuddle-left (void* k) or cuddle-right (void *k) -- let's not add a third style
Attachment #505677 - Flags: superreview?(stejohns) → superreview+
Comment on attachment 505677 [details] [diff] [review]
Review feedback changes

I think we need some guarantee that a fake name and a real name don't collide.   I think (we need to check, I'd ask Lars) that the square bracket, (ie "[gc]") style names are not valid ecmascript names but this style isn't enforced, we should enforce it and assert if someone tries to use a fake name that doesn't conform.   Some comments about this in getInvocationCount would be good too.
Attachment #505677 - Flags: review?(treilly) → review-
Any string is a valid ECMAScript name for the purposes of dynamic property lookup, if that's what you mean.  In AS3 every method name (really slot name) is restricted to the usual identifier syntax, but in ABC it isn't so, it's possible to construct classes that have non-AS3 names on the slots I believe.

Leaving the ABC problem aside for the moment, we may be rescued by the fact that the function name is restricted to being something identifierish even for functions that are stored in dynamic properties, and presumably it's the function's name, not the slot's name, that you care about.

So it really depends on how hard you want to avoid collisions.  If I were you I would ignore the ABC issue, and I assume you're looking at function names.  In that case, the [bracket] pattern is almost certainly good enough for your needs.
Flags: flashplayer-bug-
Target Milestone: Q3 11 - Serrano → Q1 12 - Brannan
Assignee: rulohani → jbahuley
Flags: flashplayer-injection-
Target Milestone: Q1 12 - Brannan → Q2 12 - Cyril
Status: ASSIGNED → NEW
Tamarin is a dead project now. Mass WONTFIX.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Tamarin isn't maintained anymore. WONTFIX remaining bugs.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: