499692 - Implement performance and memory sampler hooks for external scripting languages

Status: RESOLVED FIXED

(Tamarin Graveyard :: Tools, defect, P4)

Description

[Alexandru Chiculita]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/530.5 (KHTML, like Gecko) Chrome/2.0.172.31 Safari/530.5
Build Identifier: 

1. Implement a method to inject callstack nodes from external scripting languages, so that you can profile both AS3 and the other scripting language.
2. Add sample types that for external scripting languages objects.

Reproducible: Always

Comment 1

[Alexandru Chiculita]

Attachment: patch

Comment 2

[Alexandru Chiculita]

Attachment: patch

Comment 3

[Tommy Reilly]

Comment on attachment 385084 [details] [diff] [review]
patch

Getting closer!   Can we union functionId with the fields you don't need in CallStackNode/Element?

What's forceWrite for?

Comment 4

[Alexandru Chiculita]

If we union the functionId with something else it will become hard to write the samples in Sampler::writeRawSample (considering x86 / x64 differences), so that reading them is as simple as reinterpreting that buffer as StackTrace::Element. I'm looking at a solution right now.

forceWrite: It is used to bypass the check for samplingAllAllocs. In case samplingAllAllocs is disabled (and is disabled by default) I still want to sample the objects from the external script language. It will only  be true when called from the other language allocator.

Comment 5

[Alexandru Chiculita]

If m_info == 0 it means the stack node is a fake CallFrame or it is an external script callframe. The fake CallFrame uses m_name and both use m_linenum. We can use another value for m_info (for example 1) in the case of the external script frame and union {{m_name, m_filename} with m_functionId} in the x86 case and {m_filename with functionId} in the x64 bit build. I'm working on a patch.

Comment 6

[Alexandru Chiculita]

Attachment: patch with union in StackTrace::Element

Comment 7

[Alexandru Chiculita]

Attachment: patch fix

Moved "#ifdef DEBUGGER" out of AvmCore::createSampler(). The patch with StackTrace::Element already contains this fix.

Comment 8

[Tommy Reilly]

Comment on attachment 385138 [details] [diff] [review]
patch with union in StackTrace::Element

Looks good to me, asking Ed for superreview.

Comment 9

[Edwin Smith]

Comment on attachment 385138 [details] [diff] [review]
patch with union in StackTrace::Element

The #ifdef AVMPLUS_64BIT code isn't needed, since it doesn't reduce the structure size.

If it's really needed, a good comment explaining why is required.

Also, AVMPLUS_64BIT is deprecated, please use VMCFG_64BIT.

Comment 10

[Alexandru Chiculita]

Attachment: patch without AVMPLUS 64 BIT

I've removed AVMPLUS 64 BIT.

Comment 11

[Alexandru Chiculita]

Attachment: added comment and used VMCFG_64BIT

Renamed hasFunctionId() to isAS3Sample() and used VMCFG_64BIT instead.

I've used the #ifdef VMCFG_64BIT becuase Element is serialized in Sampler.cpp and we can not let the compiler auto arrange the elements in the union. 

(In Sampler::writeRawSample the Element is serialized by writing values one by one, while in Sampler::readSample the Element is deserialized by reinterpreting the pointer to the start of the buffer as an Element*)

Comment 12

[Edwin Smith]

Comment on attachment 387663 [details] [diff] [review]
added comment and used VMCFG_64BIT

The part about how we decode by casting to Element* (explaining why the 64bit specialization is needed) also deserves a comment, since it actually explains *why* we do this layout.  (ok, it forces the compiler, but why...).  add that explanation to the comment, and otherwise, looks ok.

Comment 13

[Alexandru Chiculita]

Attachment: added a better comment

Comment 14

[Edwin Smith]

Comment on attachment 387664 [details] [diff] [review]
added a better comment

cool, by the way: once we R+, no need to re-review even if minor tweaks were requested.

Comment 15

[Raul Hudea]

Attachment: fixed an issue and rearranged Element struct to avoid MSVC compiler padding

* fixed isAS3Sample to actually detect AS3, not script samples
* rearranged the Element struct elements as MSVC++ compiler increased the struct size to 24 bytes because the largest element (uint64_t) was in the middle of the struct and padded with 4 bytes after first (m_info) and after the last (m_linenum)

Comment 16

[Steven Johnson]

Last patch needs a review request (if you want it reviewed)....

Comment 17

[Steven Johnson]

Comment on attachment 389892 [details] [diff] [review]
fixed an issue and rearranged Element struct to avoid MSVC compiler padding

Patch will no longer apply cleanly after the patch for https://bugzilla.mozilla.org/show_bug.cgi?id=487027 was pushed. (Cleanup of the patch results in code that won't compile). Please rebase the patch to the current tip with that in mind.

Comment 18

[Alexandru Chiculita]

Attachment: rebased patch

Comment 19

[Steven Johnson]

Comment on attachment 391297 [details] [diff] [review]
rebased patch

-- General comment: while I understand the need and motivation for this change (to wit: dealing with struct padding issues while conserving memory via intelligent unioning), I'm concerned that the way this is coded is a bit brittle and perhaps subject to breakage from future changes, since both StackTrace::Element and Sampler::writeRawSample have to remain in careful synchronization to avoid memory corruption. Could we consolidate all the 32/64 special-casing into StackTrace::Element and avoid having to have Sampler::writeRawSample also know about the intimate details?

-- FAKE_CALL_FRAME doesn't appear to be used anywhere. Do we need it? What's it for?

-- In Sampler.cpp, the comment:

	// FIXME: can filename can be stored in the AbstractInfo?

is troublesome; what is the "AbstractInfo"? In general, checking in code with known things that require fixing is not something we want to do.

-- Style Nit: existing code in Sampler favors using NULL (rather than 0) for null pointers; can we maintain this rather than mixing usage?

-- Style Nit: the coding style

	if (condition) {
		foo();
	} else {
		bar();
	}

is not preferred in this codebase, instead please use

	if (condition) 
	{
		foo();
	} 
	else 
	{
		bar();
	}

Comment 20

[Tommy Reilly]

That FIXME is an ancient one from me and isn't introduced by this patch.  It refers to the fact having 3 elements (line,file,info) could be 2 elements (line,info) if we stored the file in the info

Comment 21

[Alexandru Chiculita]

Attachment: patch

Addressed Steven's comments.

Comment 22

[Steven Johnson]

Comment on attachment 391580 [details] [diff] [review]
patch

much nicer! thanks for taking the time to clean it up. who's pushing this, me or Tommy?

Comment 23

[Tommy Reilly]

pushed:

changeset:   2288:0d9774c9f7ac
user:        Tommy Reilly <[e-mail]>
date:        Fri Jul 31 10:43:12 2009 -0400
summary:     implement performance and memory sampler hooks for external scripting languages (r=stejohns,me,ed)

Comment 24

[Alexandru Chiculita]

Attachment: patch

The CallStackNodes for the external scripting language do not use stack memory, instead I create a big buffer from the start. 

In that case I need to be able to remove CallStackNodes from the list when sampling stops while still having CallStackNodes from the external language in the list.

Comment 25

[Steven Johnson]

This change makes me nervous -- I don't want to approve it until I can see the context of how it is being used.

Comment 26

[Alexandru Chiculita]

I cannot keep CallStackNodes on the stack and I use a 200 elements buffer instead. When the buffer is full (recursion level is > 200) the stack trace is not updated anymore, but it still keeps taking samples. However, when sampling is stopped while external script CallStackNodes are in the list, the nodes have to be removed, because the buffer is deleted. This is what I use to delete the external script nodes:

CallStackNode* prevAS3StackNode = NULL; // points to the last AS3 node
CallStackNode* stackNode = core->callStack;
while (stackNode)
{
    if (stackNode->functionId())
    {
        // Remove this external script node
        if (prevAS3StackNode)
        {
            // Update the previous AS3 node to link to the following one.
            prevAS3StackNode->setNext(stackNode->next());
        }
        else
        {
            // This is the first node in the list. Update the head of the list.
            core->callStack = stackNode->next();
        }
    }
    else 
    {
        // update the last AS3 stack node
        prevAS3StackNode = stackNode;
    }
    
    stackNode = stackNode->next();
}

Comment 27

[Steven Johnson]

Comment on attachment 394048 [details] [diff] [review]
patch

Approved on the condition a comment is added, explaining this is only for use by the external profiler, etc...

Comment 28

[Alexandru Chiculita]

Attachment: fix

Fixing "void CallStackNode::init(AvmCore* core, uint64_t functionId, int32_t lineno)" to correctly set the m_info to NULL for External Script call stack nodes.

Comment 29

[Steven Johnson]

Comment on attachment 394048 [details] [diff] [review]
patch

pushed as changeset:   2381:bf258eb2e048

Comment 30

[Steven Johnson]

Comment on attachment 395055 [details] [diff] [review]
fix

pushed as changeset:   2381:bf258eb2e048

Comment 31

[Alexandru Chiculita]

Attachment: Write the id as as double not as uint64

Comment 32

[Steven Johnson]

Comment on attachment 401398 [details] [diff] [review]
Write the id as as double not as uint64

want me to push this for you?

Comment 33

[Alexandru Chiculita]

yes please

Comment 34

[Steven Johnson]

Comment on attachment 401398 [details] [diff] [review]
Write the id as as double not as uint64

pushed to redux as changeset:   2574:15ecbc9640a2

Comment 35

[Alexandru Chiculita]

Attachment: API rename after review

Renamed StackFrame.id to StackFrame.scriptID. 

(Please push this for me)

Comment 36

[Alexandru Chiculita]

Attachment: fixing assert

This fixes the assert in Sampler.cpp on line 596 for script call stacks
NULL_OR_MARKED(e->fakename());

The fakename is in a union that is overwritted by the script id.

Comment 37

[Steven Johnson]

Did you mean to ask for a review for these patches? If you, you generally should ask a specific person for a review, rather than just flagging it "review?"

I'll do the review on these for now (and push if OK), just FYI for next time

Comment 38

[Steven Johnson]

Comment on attachment 403995 [details] [diff] [review]
API rename after review

pushed as changeset:   2661:de8dea4d1d13

Comment 39

[Steven Johnson]

Comment on attachment 404028 [details] [diff] [review]
fixing assert

pushed as changeset:   2661:de8dea4d1d13

Comment 40

[Alexandru Chiculita]

Attachment: GC fix

GC reaps objects without notifying the sampler. That's because the GC is not set active when objects are reaped in the SetStackEnter() call.

Comment 41

[Steven Johnson]

Comment on attachment 406424 [details] [diff] [review]
GC fix

This looks pretty harmless and safe to me, but I'm not qualified to review this code section -- removing myself from review request. Lars/Tommy really need to review this.

Comment 42

[Lars T Hansen]

Comment on attachment 406424 [details] [diff] [review]
GC fix

I believe the fix is correct.  We'll wait for Tommy, then think about landing it.

Comment 43

[Lars T Hansen]

I'll land it when I have a chance, within the next day or so.

Comment 44

[Alexandru Chiculita]

Attachment: patch

The presweep callbacks can explicitly add GC work items and a mark is necessary after they are called. There's no need to flush barriers as the the presweep callbacks can't drive marking or trigger write barriers as the barrier is disabled.

Comment 45

[Alexandru Chiculita]

Attachment: patch

Fixed the sampler, now it disables itself when stack overflows while marking.

Comment 46

[Lars T Hansen]

Comment on attachment 407979 [details] [diff] [review]
patch

Sure, let's try it.  I will push it.

Comment 47

[Lars T Hansen]

Comment on attachment 406424 [details] [diff] [review]
GC fix

redux changeset:   2858:c56c51ed52a0

Comment 48

[Lars T Hansen]

Comment on attachment 407979 [details] [diff] [review]
patch

redux changeset:   2858:c56c51ed52a0

Comment 49

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 50

[Edwin Smith]

can we close this one?  only one non-obsolete patch is outstanding and it's R+.  Someone needs to rebase/push.

Comment 51

[Dan Smith]

Retargeting for landing by Steven in Anza.

Comment 52

[Dan Smith]

Tom, rebase, land, and close in Brannan.

Comment 53

[Tommy Reilly]

changeset:   2288:0d9774c9f7ac
user:        Tommy Reilly <[e-mail]>
date:        Fri Jul 31 10:43:12 2009 -0400
summary:     implement performance and memory sampler hooks for external scripting languages (r=stejohns,me,ed)