Unify Shark/vtune/jscall/dtrace/xperf hooks

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: (dormant account), Assigned: sfink)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 12 obsolete attachments)

34.27 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
There are many similar function-call hooks in spidermonkey, they should be unified
Current plan: rename the existing static probe points (DTrace::*) to generic name, use the MSR list of interesting events (for xperf) as the basis for what events to probe, remove the ones that require information incompatible with running JITted.

See https://wiki.mozilla.org/Sfink/JS_Profiling-Related_APIs for some investigation of the current state of affairs.

Andreas Gal has the probe list from MSR.

Comment 2

8 years ago
Events we are interested in:

ExecuteBegin filename, lineno, funname (call into JS API to run JS code)
ExecuteDone filename, lineno, funname

ScriptCompileBegin filename, lineno, funname
ScriptCompileDone filename, lineno, funname

CalloutBegin filename, lineno, classname, funname (engine calls embedding provided native function/getter/setter)
CalloutDone filename, lineno, classname, funname

FunctionEntry filename, lineno, classname, funname
FunctionExit filename, lineno, classname, funname

ObjectCreate filename, lineno, classname, address, size (includes function objects)
ObjectFinalize classname, address
ObjectResize filename, lineno, classname, address, oldsize, newsize

StringCreate filename, lineno, address, size
StringFinalize address, size

GCBegin compartment
GDDone compartment
GCMarkBegin compartment
GCMarkDone compartment
GCSweepBegin compartment
GCSweepDone compartment

HeapResize compartment oldsize newsize

Updated

8 years ago
Assignee: general → sphink

Comment 3

8 years ago
I can help with inserting the probes in the right places if you wire them up.

Comment 4

8 years ago
Created attachment 462585 [details]
Sample xperf manifest (doens't match exactly what we want, but very close)

Comment 5

8 years ago
Steve, can you estimate how long this bug will take until we have basic dtrace and xperf reporting in place?

Discussion notes:
- we will strip out the current dtrace callouts in javascript.d, they are terrible and use our new callouts instead
- Steve will rename DTrace:: to a suitable name but keep most of the code and extend it with the probes above
- We want one interface and 3 reporting implementations: xperf, dtrace, custom jsapi (the latter maybe only for FunctionEntry/Exit)
- we will double check this code doesn't cost anything if compiled out
- a subset might be enabled by default, i.e. FunctionEntry/Exit with the JSAPI hook for use by taras (further discussion needed)

Taras, can you make a case why we want this in default product builds? And if we do, what do we need exactly? Just functions or more? Some of the reporting stuff would be cheap and nice to have by default (i.e. the GC reporting, HeapResize). I am worried about having FunctionEntry/Exit by default for performance reasons, same for Object* and String*.

Comment 6

8 years ago
This is a high priority item for performance analysis and the JSBench/JSMeter work by MSR is blocked on this. I would like to get this done asap. Let me know if you need help with anything.
(Reporter)

Comment 7

8 years ago
(In reply to comment #5)

> Taras, can you make a case why we want this in default product builds? And if
> we do, what do we need exactly? Just functions or more? Some of the reporting
> stuff would be cheap and nice to have by default (i.e. the GC reporting,
> HeapResize). I am worried about having FunctionEntry/Exit by default for
> performance reasons, same for Object* and String*.

Andreas, a function-hooking is essential for being able to diagnose performance issues in the Firefox, addons and Fennec. It might be of help in measuring webpage performance too, but that's not something I've focused on.

So far we've been producing special builds with this functionality which an extra overhead for us and is too restrictive because we can't effectively debug performance issues that arise in the wild with stock builds.
I have renamed the files, pruned out the unwanted dtrace detail, coerced dtrace into compiling again, and updated the xperf manifest to provide everything requested. I haven't finished putting in appropriate messages for everything in the manifest, and probably won't until I see whether they're useful in the UI.

The next step is probably to set up a Windows dev environment, or at least enough to run the manifest compiler. I've started writing the xperf probe code, but it'd be easier if I had the actual generated .h file. It's hard to give a time estimate before I have that environment set up. If you have something already set up that I could rdesktop/VNC into, that might speed things along.

Comment 9

8 years ago
#7: My question was aiming at who needs these builds. If developers need access to these builds, we need tinderbox columns for them. If users need access to this functionality, thats a different issue. Which one is it?
(Reporter)

Comment 10

8 years ago
(In reply to comment #9)
> #7: My question was aiming at who needs these builds. If developers need access
> to these builds, we need tinderbox columns for them. If users need access to
> this functionality, thats a different issue. Which one is it?

In this context we are targeting Users. Steve is working on landing this stuff to allow implementing tools for extension developers in bug 558200. Extensions contribute a lot of slowdowns to the product and we need to help extension authors optimize their own code.

Currently our infrastructure is too poor to be of help to 99% of extension authors.

Comment 11

8 years ago
Steve, did you get access to the machine(s) you need to make progress with this?
It took me embarrassingly long, but yes, I have everything building under XP now. 

(I already had an XP disk image with everything necessary it, dammit, but converting it from a once-physical-single-partition to a usable VM image was not straightforward. I no longer possess the physical hardware that I grabbed a backup off of.)

Now that I'm unblocked on Windows, I should have this done early next week.

Updated

8 years ago
Summary: Unify Shark/vtune/jscall/dtrace hooks → Unify Shark/vtune/jscall/dtrace/xperf hooks
Blocks: 586088
I split off a separate bug 588537 for the ETW work, and will attach the unification portion here. The ETW stuff has lots of build changes and other messy bits. The unification portion is fairly trivial. (Note that the ETW work includes adding a bunch of probes throughout the code, which means that the unified probe implementations will have those events accessible as well. So the ETW bug is not entirely just about ETW.)
Blocks: 588537
Created attachment 467124 [details] [diff] [review]
Rename "dtrace" to "probes"

Rename. No behavior should be affected as a result of this patch.
Attachment #462585 - Attachment is obsolete: true
Attachment #467124 - Flags: review?(gal)
Created attachment 467127 [details] [diff] [review]
eliminate use of fp in probes

Throw out all probes that use the frame pointer, so that probes can be independent of whether any JITting is going on. (I may add back some of this later, in a way that allows the data to be available when it can be without interfering.)
Attachment #467127 - Flags: review?(gal)

Comment 16

8 years ago
Comment on attachment 467127 [details] [diff] [review]
eliminate use of fp in probes

I think we can join the object-create probes into one. I don't think anyone cares about the individual times.

jsprobes_frame_linenumber should be something like FrameLineNumber() and use ? : for compactness.
Attachment #467127 - Flags: review?(gal) → review+

Comment 17

8 years ago
Reading up on the parallel email thread. Awesome work Steve.

Updated

8 years ago
Attachment #467124 - Flags: review?(gal) → review+
(In reply to comment #16)
> Comment on attachment 467127 [details] [diff] [review]
> eliminate use of fp in probes
> 
> I think we can join the object-create probes into one. I don't think anyone
> cares about the individual times.

Oh, sorry. I actually do that in a later patch attached to bug 588537, but you're right that it would probably be better done as part of this cleanup portion. I need to talk to you about the bug 588537 stuff today anyway, so I'll leave it there for now.

> jsprobes_frame_linenumber should be something like FrameLineNumber() and use ?
> : for compactness.

I'm assuming you're talking about jsprobes_fun_linenumber and jsprobes_filename? Because the patch you're reviewing eliminates jsprobe_frame_linenumber entirely. The others are still left there, although I didn't add them; they're existing code that I only touched to rename from dtrace and eliminate the use of fp.

I see that they're still there even after my 588537 patches. I can add another cleanup patch here to fix up the dtrace stuff.

(And I'll probably have to revise some other static functions that I added, because it looks like I copied the wrong naming convention for them.)
Created attachment 469252 [details] [diff] [review]
Rename "dtrace" to "probes"

Unbitrotted (switched to fp accessors)
Attachment #467124 - Attachment is obsolete: true
Attachment #469252 - Flags: review?(gal)
Created attachment 469259 [details] [diff] [review]
eliminate use of fp in probes

Mindless update to handle the fp accessors
Attachment #467127 - Attachment is obsolete: true
Attachment #469259 - Flags: review?(gal)
Created attachment 469267 [details] [diff] [review]
Update to new set of probes, cleanup

Ok, I moved this patch out of 588537 because it's just a cleanup and it addresses your earlier review comments. This prunes out a few probes, adds a bunch more, and eliminates the RAII classes (ObjectCreationScope and ExecutionScope). This series should be ready for landing.
Attachment #469267 - Flags: review?(gal)
Attachment #469267 - Flags: approval2.0?
Attachment #469259 - Flags: approval2.0?
Attachment #469252 - Flags: approval2.0?

Comment 22

8 years ago
Comment on attachment 469252 [details] [diff] [review]
Rename "dtrace" to "probes"

NPOTB. We should be able to land this without approval.
Attachment #469252 - Flags: review?(gal) → review+

Updated

8 years ago
Attachment #469259 - Flags: review?(gal) → review+

Comment 23

8 years ago
Comment on attachment 469267 [details] [diff] [review]
Update to new set of probes, cleanup

Move objectSize into JSObject?
Attachment #469267 - Flags: review?(gal) → review+

Comment 24

8 years ago
sayrer, do you agree this is NPOTB and we can land it on TM?
Created attachment 469368 [details] [diff] [review]
New set of probes

Address gal's comment 23: create byteSize() method in jsobj.h.
Attachment #469267 - Attachment is obsolete: true
Attachment #469368 - Flags: review?(gal)
Attachment #469368 - Flags: approval2.0?
Attachment #469267 - Flags: approval2.0?
Comment on attachment 469368 [details] [diff] [review]
New set of probes

>+    size_t byteSize(uint32 nslots) const;
>+    size_t byteSize() const { return byteSize(numSlots()); }

This is really misnamed (gal :-P). House style tries to use size for sizeof/size_t byte-size, length for element count. ISTM slotBytes is a better name.

>+JSObject::byteSize(uint32 nslots) const
>+{
>+    int ndslots = nslots - JS_INITIAL_NSLOTS;
>+    if (ndslots <= 0)
>+        ndslots = 0;
>+    else
>+        ndslots++; /* number of dslots is stored at index -1 */

I think we want to be more precise. Right now, dense arrays store unbiased dslots (net) length in dslots[-1], all other objects store biased. We shouldn't just subtract and then clamp at 0.

This is all going to change, though, so maybe it doesn't matter -- but my experience is that precision (assertions at least) about fussy sizing and aligning issues always matters, even in the short run. Soliciting feedback from Nick.

>+    return sizeof(js::Value) * ndslots
>+        + (isFunction() && ! getPrivate()) ? sizeof(JSFunction) : sizeof(JSObject);

Nit: prevailing style would underhang the + below the s in sizeof(js::Value).

Great to see this probe unification work coming along.

/be
Attachment #469368 - Flags: feedback?(nnethercote)

Comment 27

8 years ago
Comment on attachment 469368 [details] [diff] [review]
New set of probes

>+    return sizeof(js::Value) * ndslots
>+        + (isFunction() && ! getPrivate()) ? sizeof(JSFunction) : sizeof(JSObject);

Align the + with the sizeof and no space between ! and the getPrivate.
Attachment #469368 - Flags: review?(gal) → review+
(In reply to comment #26)
> 
> >+JSObject::byteSize(uint32 nslots) const
> >+{
> >+    int ndslots = nslots - JS_INITIAL_NSLOTS;
> >+    if (ndslots <= 0)
> >+        ndslots = 0;
> >+    else
> >+        ndslots++; /* number of dslots is stored at index -1 */
> 
> I think we want to be more precise. Right now, dense arrays store unbiased
> dslots (net) length in dslots[-1], all other objects store biased. We shouldn't
> just subtract and then clamp at 0.

I believe dslots[-1] is no longer used in dense arrays;  the capacity is stored in an fslot.  Hence bug 587397.  So this function should have a dense-array-only case.  And adding a comment to bug 587397 saying that byteSize() will need to change if dslots[-1] is removed for dense arrays would be very helpful!
Thanks, Nick (forgot about the fslot change). You can probably mark feedback+ with fixes as noted, at this point.

slotBytes, please -- no byteSize!

/be
Attachment #469368 - Flags: feedback?(nnethercote) → feedback+
(In reply to comment #26)
> Comment on attachment 469368 [details] [diff] [review]
> New set of probes
> 
> >+    size_t byteSize(uint32 nslots) const;
> >+    size_t byteSize() const { return byteSize(numSlots()); }
> 
> This is really misnamed (gal :-P). House style tries to use size for
> sizeof/size_t byte-size, length for element count. ISTM slotBytes is a better
> name.

I didn't like the name much either, so I'm happy to change. But doesn't slotBytes() imply only the size consumed by the slots themselves? Currently, it is returning sizeof(JSObject or JSFunction) plus the space taken by the slots.

> This is all going to change, though, so maybe it doesn't matter -- but my
> experience is that precision (assertions at least) about fussy sizing and
> aligning issues always matters, even in the short run. Soliciting feedback from
> Nick.

Well, now that this function is in jsobjinlines.h, someone may call it who actually needs the answer to be correct...

I have added comment to bug 587397.
(In reply to comment #30)
> (In reply to comment #26)
> > Comment on attachment 469368 [details] [diff] [review] [details]
> > New set of probes
> > 
> > >+    size_t byteSize(uint32 nslots) const;
> > >+    size_t byteSize() const { return byteSize(numSlots()); }
> > 
> > This is really misnamed (gal :-P). House style tries to use size for
> > sizeof/size_t byte-size, length for element count. ISTM slotBytes is a better
> > name.
> 
> I didn't like the name much either, so I'm happy to change. But doesn't
> slotBytes() imply only the size consumed by the slots themselves? Currently, it
> is returning sizeof(JSObject or JSFunction) plus the space taken by the slots.

Oh, ok. grossObjectSize? Doesn't include meta-data that is usually shared (map/lastProp). slotsAndBaseThingSize? Explicit is said to be better than implicit ;-).

/be
Created attachment 470572 [details] [diff] [review]
New set of probes, object size accessor

New patch with the updated slot size computation logic and renamed objectSize to the awkward but accurate slotsAndBaseThingSize. Or I can rename again to

  slotsAndStructSize
  approximateSize
  privateSize
  unsharedSize
  profilerSize
  marginalSize
  randomNumberCorrelatedWithSize

if you like one of those better.
Attachment #469368 - Attachment is obsolete: true
Attachment #470572 - Flags: review?(gal)
Attachment #470572 - Flags: approval2.0?
Attachment #469368 - Flags: approval2.0?
I like objectSize but someone is gonna get confused, so without going too crazy, can we pick a more precise name? I would go for slotsAndStructSize, myself. Else grossObjectSize, because this whole naming issue is kinda gross ;-).

/be

Comment 34

8 years ago
Comment on attachment 470572 [details] [diff] [review]
New set of probes, object size accessor

>+    size_t slotsAndBaseThingSize(uint32 nslots) const;
>+    size_t slotsAndBaseThingSize() const { return slotsAndBaseThingSize(numSlots()); }
>+

I really don't care. I liked objectSize().

>-static char dempty[] = "<null>";
>+const char *Probes::nullName = "(null)";
> 

Can this be static?
Attachment #470572 - Flags: review?(gal) → review+
(In reply to comment #34)
> >-static char dempty[] = "<null>";
> >+const char *Probes::nullName = "(null)";
> > 
> 
> Can this be static?

Define "static". It's already static; that's defined in class Probes:

  class Probes {
      static const char *nullName;
      ...
  };

But if you mean to leave it as a jsprobes.cpp file-scoped static, I moved it because I used it in inline functions defined in jsprobes.h that were previously non-inlined class methods defined in jsprobes.cpp, which is why it could get away with the file-scoped static. I could put it back to a file-scoped static defined in the header file if you think it would make the optimizer happy. I figured it didn't matter, so I may as well only have one copy.

Comment 36

8 years ago
Sounds good. r=me
Created attachment 470603 [details] [diff] [review]
Net set of unified probes

This addresses all comments (now at slotsAndStructSize).
Attachment #469252 - Attachment is obsolete: true
Attachment #469259 - Attachment is obsolete: true
Attachment #470572 - Attachment is obsolete: true
Attachment #469252 - Flags: approval2.0?
Attachment #469259 - Flags: approval2.0?
Attachment #470572 - Flags: approval2.0?

Comment 38

8 years ago
Not part of the product build, so no approval needed (double checked with sayrer). Roy will help landing.
(In reply to comment #35)
> (In reply to comment #34)
> > >-static char dempty[] = "<null>";
> > >+const char *Probes::nullName = "(null)";
> > > 
> > 
> > Can this be static?
> 
> Define "static". It's already static; that's defined in class Probes:
> 
>   class Probes {
>       static const char *nullName;

Why allocate a pointer? It's never written to (I am assuming) yet not const char *const type (which would highlight that it is an unnecessary pointer). IOW you want an array, not a pointer:

const char nullName[] = "..."

Should be enough if you could get away without declaring it with a dimension. Should be able to (C lets you), right?

/be
Created attachment 470612 [details] [diff] [review]
New set of probes, object size accessor

Collided with Brendan's monster. Trivial unbitrot.
Attachment #470603 - Attachment is obsolete: true
> --- Comment #39 from Brendan Eich <brendan@mozilla.org> 2010-08-30 15:57:47 PDT ---
> (In reply to comment #35)
>> (In reply to comment #34)
>> > >-static char dempty[] = "<null>";
>> > >+const char *Probes::nullName = "(null)";
>> > >
>> >
>> > Can this be static?
>>
>> Define "static". It's already static; that's defined in class Probes:
>>
>>   class Probes {
>>       static const char *nullName;
>
> Why allocate a pointer? It's never written to (I am assuming) yet not const
> char *const type (which would highlight that it is an unnecessary pointer). IOW
> you want an array, not a pointer:
>
> const char nullName[] = "..."
>
> Should be enough if you could get away without declaring it with a dimension.
> Should be able to (C lets you), right?

Uh oh. Getting a little over my head here. I'm not sure if I follow the difference between an array and a pointer in this case -- the users of nullName want to end up with a pointer to a chunk of memory containing "blah". I agree that

  char nullName const * const

would make more sense (whenever const stuff gets complicated, I have to stop putting 'const' first or my head will explode with trying to figure out what it applies to). But unless nullName is a regular non-static data member (in which case there's a big difference; the characters are sitting right next to the adjacent data members), what's the difference between declaring it as an array or a pointer?

I can't do the initialization in the class header file, if that's what you mean.

  class Probes {
    const char nullName[] = "blah";
  }

gives

warning: ISO C++ forbids zero-size array ‘nullName’
error: ISO C++ forbids initialization of member ‘nullName’
error: making ‘nullName’ static
error: invalid in-class initialization of static data member of non-integral type ‘const char [0]’

So it's not happy to infer the static from that, and it's ignoring the initializer. But

  class Probes {
    static const char nullName[] = "blah";
  }

gives

error: invalid in-class initialization of static data member of non-integral type ‘const char []’

which seems a little odd to me. If that were outside of a class definition, it would work. Though even then, it would mean duplicating the string data once per compilation unit, possibly to be collapsed to one copy during the link (?).

If I do

  class Probes {
    static const char nullName[];
  }

and then initialize it in the .cpp with

  const char Probes::nullName[] = "(null)";

it's happy. That's better than my original, because the const-ness is attached to the right place, but I don't see why the generated code would be any different.

Educate me?
Created attachment 470641 [details] [diff] [review]
New set of probes, object size accessor

Switched to const char[].
Attachment #470612 - Attachment is obsolete: true
(In reply to comment #41)
> If I do
> 
>   class Probes {
>     static const char nullName[];
>   }
> 
> and then initialize it in the .cpp with
> 
>   const char Probes::nullName[] = "(null)";
> 
> it's happy.

Yeah, that's what I meant (what you'd do in C, with necessary changes: strip the class folderall, s/static/extern/g).

> That's better than my original, because the const-ness is attached
> to the right place, but I don't see why the generated code would be any
> different.

A pointer is a word of storage containing the address of the point-ee.

An array is a constant (link-time if not compile-time) address of the first array element. It consumes zero words of storage; it cannot be an lvalue.

C (C++, etc.) convert array names to pointers to first elements freely.

You don't need a pointer to const chars here (not even a const pointer to const chars), that means an extra load. Using nullName would compute the address of this word-sized static pointer, then load it, and only then be ready to load chars from the const string.

(A const pointer to const chars could be useful if you took its address because some API wanted a const char * const * parameter).

/be
Created attachment 470882 [details] [diff] [review]
New set of probes, object size accessor

Updating the patch because it was horribly broken by the dempty -> Probes::nullName change, which neither my build nor the try server caught because jsprobes.cpp isn't compiled unless HAVE_DTRACE is set. I fixed the breakage and set jsprobes.cpp to build by default (I had that in a later patch for bug 588537, but gal has previously ok'd it.)

This patch is for tracemonkey. I could post one for jaegermonkey instead (very minor differences.)

Thanks for the array-vs-pointer lesson; I really should've known that. I think I did, once.
Attachment #470641 - Attachment is obsolete: true

Comment 45

8 years ago
land on the tracemonkey tree, not jaegermonkey
Created attachment 470913 [details] [diff] [review]
New set of probes, object size accessor

The JM landing naturally bitrotted this patch. Here's the JM version, which now works unmodified in the TM branch.
Attachment #470882 - Attachment is obsolete: true

Updated

8 years ago
Blocks: 592518
http://hg.mozilla.org/tracemonkey/rev/bb3730430112
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
This breaks a build with --enable-dtrace for me, various errors about "invalid conversion from ‘const char*’ to ‘char*’", eg:

mozilla/js/src/jsprobes.h: In static member function ‘static void js::Probes::startExecution(JSContext*, JSScript*)’:
mozilla/js/src/jsprobes.h:173: error: invalid conversion from ‘const char*’ to ‘char*’
mozilla/js/src/jsprobes.h:173: error:   initializing argument 1 of ‘void __dtrace_probe$javascript$execute__start$v1$63686172202a$696e74(char*, int)’
mozilla/js/src/jsprobes.h: In static member function ‘static void js::Probes::stopExecution(JSContext*, JSScript*)’:
mozilla/js/src/jsprobes.h:186: error: invalid conversion from ‘const char*’ to ‘char*’
mozilla/js/src/jsprobes.h:186: error:   initializing argument 1 of ‘void __dtrace_probe$javascript$execute__done$v1$63686172202a$696e74(char*, int)’

Comment 49

8 years ago
What compiler do you use? Linux with newer gcc? Reopen or followup bug?
I'm guessing OSX. I have a dev OSX machine that I'm pretty much done configuring, so will hopefully be able to look into this today. Most likely, it's a difference between the const-ness in the headers generated by true OSX dtrace vs Linux's systemtap dtrace emulation. I probably just need to sprinkle some const-ness in javascript-trace.d, but I want to be able to test that before I submit a patch.
New patch in new bug, it's almost always the right thing.

/be
Bug 593483. It's a bug in dtrace, known since at least 2006.

Updated

8 years ago
Depends on: 596103
You need to log in before you can comment on or make changes to this bug.