Created attachment 448041[details][diff][review]
patch: adds performance measurement interface
This patch adds to XPCOM an interface to system-provided detailed performance counters. I have been able to use this to do precise benchmarking of my CSS parser performance improvements - PR_Now() based measurements had enormous variance even with the computer otherwise completely idle; this interface provides much more stable numbers. It also lets you see things like cache statistics and context switches, which can be very helpful. Currently there is only an implementation for Linux >=2.6.31, but I expect both OSX and Windows have similar things.
To motivate the interface, I'll also attach the benchmarking harness that I've been using, but I don't propose that part for inclusion.
I am not attached to the names, or even the structure of the implementation. It's possible that this would be better living over in js/ than xpcom/, for instance.

Created attachment 448042[details][diff][review]
not for inclusion: example use of new interface to benchmark parser (part 1)
Here's part 1 of the promised demo - it adds microbenchmarks of the CSS parser to nsIDOMWindowUtils. (May not compile against current trunk, I have a lot of pending API revisions in my queue.)

Created attachment 448043[details]
not for inclusion: example use of new interface to benchmark parser (part 2)
And this is part 2 - I've labeled it text/plain, but it's actually privileged-Javascript-in-HTML that runs benchmarks using the two previous attachments.

(In reply to comment #3)
> Are these numbers from our process only, or from all processes, or what?
Our process only. That's one of the ways it's more stable than wall-clock measurements.
> Why go through an XPCOM interface here with virtual calls?
If you know another way to make it accessible from Javascript I am all ears.

Make a reasonable C++ interface in nsPerformanceMeasurement.h, have each platform have its own implementation of nsPerformanceMeasurement, and add an XPCOM service wrapper over nsPerformanceMeasurement.h for JS users, or ask the JS people how they would like it exposed.
We need get away from the mentality that interfaces need to be slow and bloated just because they might be accessed via JS.

It was coded the way it is, for the record, because I needed something that was callable from JS on day one (see attachment 448043[details]) and I already know how to write .idl files. And I still wasted two hours fighting with XPCOM, so I am definitely on board with your desire to get rid of that.
dwitte, could you comment on better ways to implement?

Comment on attachment 448041[details][diff][review]
patch: adds performance measurement interface
There's some incorrect whitespace stuff here, although that's trivial. I'm delegating this to cjones since I know he had thoughts on this (and has touched TimeStamp which is related).

This is an immensely useful bit of code, thanks Zack! Squeezing maximum benefit out of it looks to be an interesting design problem. Here are the use cases I can think of:
(1) Direct, lightweight use from C++. I'd like to be able to do
#include "mozilla/PerformanceMeasurer.h"
void MeasureX() {
PerformanceMeasurer p;
p.Start();
X();
p.Stop();
// dump, whatever
}
I would bet folding money that jseng hackers would want the same.
(2) Use from chrome-privileged JS. It'd be nice to do the ~same as above,
function measureX() {
p = new PerformanceMeasurer();
// ...
}
(3) Use from jsshell and xpcshell.
(4) Use from content JS. I'm sure web app authors would love an instrumentation interface like this.
So, (1) seems to imply (if jseng people are interested in this tool) that the C++ should live in js/src. The design problem is (2)/(3)/(4). The options for this seem to be
(A) XPCOM wrapper (basically what Zack has now). Precludes jsshell use.
(B) ctypes with a C++-to-C shim. Precludes use from web content (?). Is ctypes available in both jsshell and xpcshell?
(C) JS class wrapped around C++ class (something like Date?)
dwitte, jorendorff, any advice on best practices here? jorendorff, would jseng folks use this class?

There is also use case 2a: chrome-privileged JS creates a measurement object and hands it to C++ functions which use it to time some, but not all, of their execution; results are read out from JS. That's what I did in the "example use" attachments.
I tried to do a ctypes shim, at roc's recommendation, and I ran into two apparently-insuperable obstacles. Most seriously is that js-ctypes still has no notion of "look this symbol up in the current runtime image, I don't care which library defines it" (i.e. the semantics of passing RTLD_DEFAULT as the first argument to dlsym()). Almost every "instead of XPCOM" use of js-ctypes that I can think of, requires this. (I thought there was a bug already for this but I couldn't find it.)
The other obstacle is that a C++ XPCOM object whose sole reference is from Javascript will get its destructor called when the garbage collector releases that reference, but there appears to be no way to mimic that with a JS object wrapped around ctypes functions. You have to put explicit destroy-method calls into the calling Javascript, which is ugly and unsafe. The low-level system interface, at least on Linux, involves holding open a bunch of file descriptors. I really don't want to publish even a privileged-only interface that can leak file descriptors if someone forgets a destroy.
I'd be happy to code this as a native JS class implemented in C++ (your (c)) if that would get us around the above problems and not make use case 2a impossible, but I don't know how to do it. Can anyone point me to instructions?

(In reply to comment #11)
> There is also use case 2a: chrome-privileged JS creates a measurement object
> and hands it to C++ functions which use it to time some, but not all, of their
> execution; results are read out from JS. That's what I did in the "example
> use" attachments.
Oh interesting. I don't think ctypes would support that case well (or should!), since that would entail a stable C++ API for unwrapping ctypes objects.
Sounds like XPCOM is the way to go for now for JS access, pending guidance from JS folk. Do you mind modifying your patch per roc's suggestion in comment 5? I have some code outside XPCOM-land where I'd like to use this instrumentation.

Created attachment 452402[details][diff][review]
revised: native JSAPI class instead of XPCOM
This revised patch implements the same (or nearly so) C++ interface, but with no XPCOM overhead, and a native JSAPI wrapper. In the present version, it is only made available from the JS shell. I'll look into exposing it in the browser on Monday - I was thinking of doing it the same way as ctypes, i.e.
Components.utils.import("PerfMeasurement.jsm");
gets you a PerfMeasurement object in the current scope. Security checks may be missing - I do not think this capability should be exposed to content JS, but I don't know if exposing it only via Cu.import is enough to block that, or if I need to add explicit security checks to the code in jsperf.cpp.
One big hole in the present code is what happens when you try to hand the JS wrapper to a function implemented in C++ - ideally, the unwrapped C++ object would appear on the other side, but there is no code to do that as yet, and I'm unclear about whether it's even possible.

Created attachment 452413[details][diff][review]
re-revised: native JSAPI class, accessible from chrome via JSM
Turns out to be quite simple to do the .jsm dance, so here we go. Now it can be used from chrome as well as js shell. I don't know if xpcshell will pick it up, though.

(In reply to comment #15)
> >+ /**
> >+ * Zero all counters and begin timing.
> >+ */
> >+ void Start();
> >+
>
> Per IRC chat, seems useful to accumulate multiple trials, so Start()
> shouldn't zero. A separate reset() method for clearing would be nice.
Ok.
> I'd additionally like a
>
> static bool hasNontrivialImpl();
>
> interface, or something to that effect. If it's easy to expose to JS,
> that'd be cool, but I don't particularly care there.
You can get this by creating an instance and then seeing whether its eventsMeasured property is nonzero - that's what the tests I added do.
I can certainly add a static method as well, though.
> Style note: since you're in js/src, you don't need to follow silly
> Gecko rules
Izzat really so? Are there other rules I should be following in js/?
> so I'd prefer start()/stop() et al. method names in your
> C++ class. That would remove the second clause from the last sentence
> above.
Can do.

(In reply to comment #16)
> (In reply to comment #15)
> > I'd additionally like a
> >
> > static bool hasNontrivialImpl();
> >
> > interface, or something to that effect. If it's easy to expose to JS,
> > that'd be cool, but I don't particularly care there.
>
> You can get this by creating an instance and then seeing whether its
> eventsMeasured property is nonzero - that's what the tests I added do.
> I can certainly add a static method as well, though.
Ah OK, I missed that. I think I'd still prefer the static method, if you don't mind.

Created attachment 453239[details][diff][review]
revised x3: cjones' requests, style fixes, add unwrapper for use with jsvals
I believe this revision addresses all of cjones' review requests, and also brings the coding style into line with SpiderMonkey preferred style. The patch is now relative to tracemonkey tip, because I've added a helper function for use with the feature added in bug 560643 that allows you to pass a jsval through an xpconnect interface. Nothing *uses* that capability yet. I wanted to add a test for that feature to the code in toolkit/components/jsperf, but I'm not sure how to code it up - that directory is totally ripped off of components/jsctypes and I don't know what I would have to do to add an IDL interface to the C++ module.

Also worth mention is that the helper function inlines the internals of JS_GetInstancePrivate, because it doesn't have a JSContext with which to call JS_GetInstancePrivate. Please tell me how to do this properly.

(In reply to comment #22)
> >+// There is one read-only JS-level attribute for each counter
> >+// supported at the C++ level. We use negated indices into this array
> >+// for the attributes' tinyids; therefore this table and the next must
> >+// be kept precisely in sync (except for the entry for eventsMeasured,
> >+// which uses a different accessor)
>
> This seems quite fragile. Further, we're trying to avoid using tinyids these
> days; they have some unfortunate side effects both within the engine and
> outside it (some of these become magically visible as negative indexes on such
> objects, e.g. pm[-3] or such, exact details escape my memory).
That is not an issue here, as the patch uses JS_PropertyStub for pm_class's getProperty and setProperty. The only issues are:
1. Avoid tinyids in aid of some day removing them from the JS API. That is a long way off.
2. The fragile table indexing. This can be made as safe as anything like it that we do elsewhere in the codebase by adding some static assertions.
> That said, no
> obviously better solution comes immediately to mind. :-\
Fire for effect only when on target :-P.
/be

Created attachment 457205[details][diff][review]
revised x4: most of waldo's change requests
Chris: I mixed up patches and started making waldo's requested changes on top of v2 instead of v3, and then had to merge the v3 changes back in, so could you have a look through this again please and make sure I didn't lose any of your requests?
jst: Please advise re this piece (unchanged in this revision):
> >+NS_IMETHODIMP
> >+Module::Call(nsIXPConnectWrappedNative* wrapper,
> >+ JSContext* cx,
> >+ JSObject* obj,
> >+ PRUint32 argc,
> >+ jsval* argv,
> >+ jsval* vp,
> >+ PRBool* _retval)
> >+{
> >+ JSObject* global = JS_GetGlobalObject(cx);
> >+ *_retval = InitAndSealPerfMeasurementClass(cx, global);
> >+ return NS_OK;
>
> Actually, I think you want JS_GetGlobalForObject(cx, JS_GetScopeChain(cx))
> here (with some error-checking). The global object for the context isn't
> actually what you want here, but I could be wrong -- ask jst or somebody
> who does DOM stuff to recite the incantation to be used here.
waldo:
(In reply to comment #22)
> Run-on here, semicolon or "but" or something.
Fixed.
> const uint8 for this these days.
Ok.
> >+// There is one read-only JS-level attribute for each counter
> >+// supported at the C++ level. We use negated indices into this array
> >+// for the attributes' tinyids; therefore this table and the next must
> >+// be kept precisely in sync (except for the entry for eventsMeasured,
> >+// which uses a different accessor)
>
> This seems quite fragile. Further, we're trying to avoid using tinyids these
> days
Even though Brendan seems to think we could get away with it here, I changed this to use a specialized (macro-generated) getter for every attribute.
> >+static JSPropertySpec pm_consts[] = {
> >+ { "CPU_CYCLES", -1, PM_PATTRS, pm_getconst, 0 },
...
> These properties would be better defined directly as values, rather than by
> using a getter function.
I believe I have done this (JS_DefineProperty has a few too many knobs)
> >+static PerfMeasurement*
> >+GetPM(JSContext *cx, JSObject *obj, const char *fname)
> >+{
> >+ PerfMeasurement *p = (PerfMeasurement*)
> >+ JS_GetInstancePrivate(cx, obj, &pm_class, 0);
> >+ if (p)
> >+ return p;
> >+
> >+ JS_ReportErrorNumber(cx, js_GetErrorMessage, 0, JSMSG_INCOMPATIBLE_PROTO,
> >+ pm_class.name, fname, JS_GetClass(cx, obj)->name);
> >+ return 0;
> >+}
>
> Use JS_GET_CLASS(cx, obj), the reason being that this expands into the
> appropriate call depending on JS_THREADSAFE.
Done.
> JS_GetInstancePrivate will set an exception if it fails (when the class of the
> object doesn't match the provided one), so you should be able to just return
> J_GIP(...) directly.
Unfortunately this is only true if you pass an argv as its last argument. As I have no argv to pass here, I can't rely on that.
It would be great if I *could*, this is one of the few places where I have to use a non-public API.
> >+ PerfMeasurement *p = new PerfMeasurement(PerfMeasurement::EventMask(mask));
> >+ if (!p) {
> >+ JS_ReportOutOfMemory(cx);
> >+ return JS_FALSE;
> >+ }
>
> This, incidentally, is correct use of ReportOOM: when an allocation not
> tracked by the JS engine fails (or would fail, in certain cases).
I was wondering, can we rely on infallible malloc in here?
> No need for a local variable here.
Fixed.
> Two lines here, for easier debugging.
Ok.
> This bit is repeated a bunch of times; it really should be pushed into a
> helper method of some sort since it seems typo-prone.
Tinyids no longer used, so this bit went away entirely.
> JS_THIS_OBJECT is fallible, and if you pass it here like this a failure will
> cause an error to be reported twice. Calculate the this object, check for
> failure, then make the GetPM call. (Repeated a bunch of times.)
Fixed (with a helper function to reduce repetition).
> >+namespace JS {
> namespace js
Unchanged; https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style specifically says use namespace JS for public stuff. (Although I see I am the only user in the tree, so maybe that's out of date?)
> May be slightly more pleasant here to hide the null-check behind
> JSVAL_IS_PRIMITIVE, up to you.
I did make this change.
> JS doesn't javadoc-ify its comments with double-*, for whatever reason.
Removed extra stars.
> I think "zeroes" is the more common spelling.
Fixed.
> >+class JS_FRIEND_API(PerfMeasurement) {
>
> Is it necessary to wrap the class name in a JS_FRIEND_API, or is it just a
> helpful documentation hint?
It is required to avoid link errors over in toolkit/components.
> Class brace goes on new line:
Fixed.
> Two-space indent for these:
>
> Impl::Impl()
> : f_cpu_cycles(-1),
> f_instructions(-1),
> ...
Fixed.
> >+static JSBool
> >+SealObjectAndPrototype(JSContext* cx, JSObject* parent, const char* name)
> >+{
> >+ jsval prop;
> >+ if (!JS_GetProperty(cx, parent, name, &prop))
> >+ return false;
> >+
> >+ JSObject* obj = JSVAL_TO_OBJECT(prop);
> >+ if (!JS_GetProperty(cx, obj, "prototype", &prop))
> >+ return false;
> >+
> >+ JSObject* prototype = JSVAL_TO_OBJECT(prop);
> >+ return JS_SealObject(cx, obj, JS_FALSE) &&
> >+ JS_SealObject(cx, prototype, JS_FALSE);
> >+}
>
> ...and now you get to learn about the momentary fun that is rooting
> garbage-collected JS values. [...]
I don't understand what this is doing or why it needs to do most of what it does, and am happy to change it as you see fit, but ... This file is copied from toolkit/components/ctypes and only the names have been changed. If this is wrong, so is that. Therefore, I left this alone in the present revision.

Created attachment 457957[details][diff][review]
r5: OOM & syscall wrappers in pm_linux.cpp
Quick respin to address these:
(In reply to comment #25)
> > Could you please wrap this in a |sys_perf_event_open()| helper or
> > somesuch? It'd be nice to get a little documentation and type
> > checking here. Probably also worth noting that this code might run on
> > a kernel that doesn't support perf_event_open() (<2.6.31 AFAICT, which
> > is a lot still), but syscall() will harmlessly return -1.
>
> Would still like this to be addressed.
Done, sorry about that, I missed it on the first go-round.
> > >+PerfMeasurement::PerfMeasurement(PerfMeasurement::EventMask toMeasure)
> > >+ : impl(new Impl),
> >
> > I'm not sure what the JS OOM conventions are; this might not fly.
> > Waldo should clarify.
>
> Judging by comment 22, I think this code needs to change.
I've attempted to fix this. It does not produce a JS exception if this allocation goes OOM, because there's no way to trigger that from a constructor; instead, it produces a valid object whose eventsMeasured bitmask is zero, and whose start/stop methods do nothing.
jst: Thanks for the r+, but I still need an answer to the question I asked you in comment 24.

Comment on attachment 457957[details][diff][review]
r5: OOM & syscall wrappers in pm_linux.cpp
(In reply to comment #24)
> I changed this to use a specialized (macro-generated) getter for every attribute.
This works. Bonus points if you were to make a perfevents.tbl file, similar in spirit to js/src/jsopcode.tbl and various other such things, so as to not rely upon multiple source locations coordinating ordering of array elements correctly -- but not necessary, since this is unlikely to be modified often. (Still, worth keeping in mind when writing future patches.)
> I believe I have done this (JS_DefineProperty has a few too many knobs)
Looks good. (I roughly agree with your parenthetical.)
> It would be great if I *could*, this is one of the few places where I have to
> use a non-public API.
Here you should be able to use JS_GET_CLASS and JS_GetPrivate (still with the latter's cast) to avoid using internal stuff. But is it necessary? I'd thought, with this being part of the JS engine by source location, that it wasn't particularly important that this be the case. (We could probably "go crazy" with internalizing uses of APIs, but it seems unnecessary and more trouble than it's worth for someone not already aware of them.)
> > >+ PerfMeasurement *p = new PerfMeasurement(PerfMeasurement::EventMask(mask));
> > >+ if (!p) {
> > >+ JS_ReportOutOfMemory(cx);
> > >+ return JS_FALSE;
> > >+ }
> >
> > This, incidentally, is correct use of ReportOOM: when an allocation not
> > tracked by the JS engine fails (or would fail, in certain cases).
>
> I was wondering, can we rely on infallible malloc in here?
You're part of the JS engine, so you shouldn't.
> > >+namespace JS {
> > namespace js
>
> Unchanged;
> https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style
> specifically says use namespace JS for public stuff. (Although I see I am the
> only user in the tree, so maybe that's out of date?)
The future is now! Wasn't even thinking of this, indeed keep as is.
> > JS doesn't javadoc-ify its comments with double-*, for whatever reason.
>
> Removed extra stars.
One step forward, one step backward:
/* One-liners can be braced this way. */
/*
* Or this way, if really desired for some reason.
*/
/*
* Major comments that cover multiple lines should be
* braced this way.
*/
Sorry for not being absolutely clear on this last time.
> It is required to avoid link errors over in toolkit/components.
*grmbl*
>diff --git a/js/src/perf/jsperf.cpp b/js/src/perf/jsperf.cpp>+ if (!JS_SealObject(cx, obj, JS_FALSE)) {
>+ return JS_FALSE;
>+ }
Don't brace single-line ifs (or single-line elses, if the if was also single-line) (but do brace if the condition takes up more than one line). (See a note below.)
>+#define GETTER(name) \
>+ static JSBool \
>+ pm_get_##name(JSContext* cx, JSObject* obj, jsval /*unused*/, jsval* vp) \
This should be |jsid id|, neither |jsval| nor |/*unused*/|. The type signature's changed in the TM branch (you're racing it), and either it changes now or sayrer changes it on that merge.
>+const uint8 PM_FATTRS = JSPROP_READONLY|JSPROP_PERMANENT|JSPROP_SHARED;
Spaces around binary operators (one other time if I'm not mistaken).
>+static PerfMeasurement*
>+GetPMFromThis(JSContext* cx, jsval* vp)
>+{
>+ JSObject* this_ = JS_THIS_OBJECT(cx, vp);
>+ if (!this_)
>+ return 0;
>+ return (PerfMeasurement*)
>+ JS_GetInstancePrivate(cx, this_, &pm_class, JS_ARGV(cx, vp));
Canonical would be |obj|, but no worries if you don't change.
>+ if (!JS_SealObject(cx, prototype, JS_FALSE) ||
>+ !JS_SealObject(cx, ctor, JS_FALSE))
>+ return 0;
Here's a case where you should brace, because the condition spills over into two lines (note the unfortunate alignment of condition and body). Alternately, if (eyeballing sez yes) it fits in the JS standard line limit of 99 characters (you may be using 79, which was the old standard -- leave as-is if you aren't in the mood to change, js/src is a haphazard mix of the two at the moment), keep the condition all on one line.
>+PerfMeasurement*
>+ExtractPerfMeasurement(jsval wrapper)
>+{
>+ if (JSVAL_IS_PRIMITIVE(wrapper))
>+ return 0;
>+
>+ // This is what JS_GetInstancePrivate does internally. We can't
>+ // call JS_anything from here, because we don't have a JSContext.
>+ JSObject *obj = JSVAL_TO_OBJECT(wrapper);
>+ if (obj->getClass() != &pm_class)
>+ return 0;
>+
>+ return (PerfMeasurement*) obj->getPrivate();
>+}
Why can't you make this method take a context, and then you can also have it report errors (if you wanted, for most predictability, or just have it return bool and perhaps rename to HasPerfMeasurement or something like that, whatever you feel like if you prefer this)? This would be the best solution.
>diff --git a/js/src/perf/pm_linux.cpp b/js/src/perf/pm_linux.cpp>+// Mapping from our event bitmask to codes passed into the kernel, and
>+// to fields in the PerfMeasurement and PerfMeasurement::impl structures.
>+const struct
>+{
>+ EventMask bit;
>+ uint32 type;
>+ uint32 config;
>+ uint64 PerfMeasurement::* counter;
>+ int Impl::* fd;
>+} kSlots[PerfMeasurement::NUM_MEASURABLE_EVENTS] = {
I think you accidentally lost a static here while moving the brace.
>+bool
>+PerfMeasurement::canMeasureSomething()
>+{
>+ return true;
>+}
Completely afield from what I was reviewing, but maybe this should inform about whether the system is a Linux system with a new enough kernel, rather than lying on old ones? Just throwing it out since I saw it.
Looks good with a last couple nits above, shouldn't be necessary to see exactly how you resolve them.

Responding now to a couple small pieces of this - more later:
> Why can't you make [ExtractPerfMeasurement] take a context, and
> then you can also have it report errors
Its callers will not have a context to provide. Remember that the use case for this is the C++ side of an XPCOM interface that was passed the boxed object via a [jsval] parameter. I'm expecting to use this over in nsIDOMWindowUtils (although not in patches that will be committed), which it's already enough of a layering violation to have that know about jsvals *at all*, don't you think?
> maybe this should inform about whether the system is a Linux system
> with a new enough kernel, rather than lying on old ones?
I could do that, would a uname() check be good enough or should I try to call sys_perf_event_open() and see if it returns ENOSYS?
> The type signature's changed in the TM branch (you're racing it),
> and either it changes now or sayrer changes it on that merge.
I'd be happy to change it and land it on TM (assuming there's going to be a TM->mc merge in the near future?) but it's still going to be unused...

jsperf.h extern JS_FRIEND_API prototypes should not indent the declarator and params (the second line).
XPConnect by design allows C++ to call JS to call C++ to call JS -- which requires a thread-local stack of JSContexts so the inner JS can use the same context as the outer, and propagate exceptions using it. If the inner C++ is meant to call ExtractPerfMeasurement and propagate exceptions to the outer JS, then it needs to get that context from the thread-local stack. Cc'ing mrbkap.
/be

(In reply to comment #29)
> Remember that the use case for this is the C++ side of an XPCOM interface
> that was passed the boxed object via a [jsval] parameter. I'm expecting to
> use this over in nsIDOMWindowUtils (although not in patches that will be
> committed), which it's already enough of a layering violation to have that
> know about jsvals *at all*, don't you think?
Hum, right. *sadfaces*
> > maybe this should inform about whether the system is a Linux system
> > with a new enough kernel, rather than lying on old ones?
>
> I could do that, would a uname() check be good enough or should I try to call
> sys_perf_event_open() and see if it returns ENOSYS?
I would think the latter is preferable (seems like something distros might backport perhaps), but this really is beyond what I want to speak authoritatively about. :-)
> > The type signature's changed in the TM branch (you're racing it),
> > and either it changes now or sayrer changes it on that merge.
>
> I'd be happy to change it and land it on TM (assuming there's going to be a
> TM->mc merge in the near future?) but it's still going to be unused...
Unused is fine, and understandable, and unnamed-without-comment versus named isn't a huge difference; the type for it is the most important part.

(In reply to comment #28)
> Bonus points if you were to make a perfevents.tbl file, similar in
> spirit to js/src/jsopcode.tbl and various other such things, so as to not rely
> upon multiple source locations coordinating ordering of array elements
> correctly -- but not necessary, since this is unlikely to be modified often.
> (Still, worth keeping in mind when writing future patches.)
Actually, even better here might be the pattern demonstrated here, since the list of things here isn't used very many places:
https://bugzilla.mozilla.org/attachment.cgi?id=458220&action=diff&headers=0#a/js/src/jscntxt.h_sec1
But again, still not a necessary change, just one to think about using in the future.

Created attachment 461362[details][diff][review]
r6: against tracemonkey, more tweaks[Checkin: comment 39]
This should be the final version of this patch. It sounds to me like neither cjones nor waldo wants to review again, but I did hit an OSX-specific build system problem that I *think* I have fixed correctly but am not 100% sure of, so I'd like ted to check me on that. The deal is, both t/c/ctypes and t/c/perf were generating object files named Module.o. They were going into different .a libraries, but on OSX only, toolkit/library explodes those into their individual object files again, so we got two copies of the ctypes Module.o and the link failed. I fixed this by renaming both ctypes' and perf's Module.cpp.
Also:
* Rediffed on top of tracemonkey rather than m-c.
* The Linux version of ::canMeasureSomething now tests whether sys_perf_event_open does something useful, rather than being hardwired to return true.
* Fixed some formatting nits pointed out by waldo.
* Fixed the Module::Call() code per jst in comment 34; filed bug 583099 on making the analogous fix to ctypes.
* After consultation with jst and luke IRL, I believe t/c/perf does *not* need to be sprinkled with AutoObjectRooters anymore (as originally requested in comment 22); thus this is also not a bug in ctypes anymore.
* After consultation with sayrer IRL, I believe the current ExtractPerfMeasurement code (that doesn't attempt to dig a JSContext out of xpconnect's data structures, or require its caller to do so) is the least bad option.
* I have not revised this code to use a perfevents.tbl file or PERF_EVENTS_LIST macro as suggested. It looked finicky, and potentially very inconvenient for whoever writes the OSX or Windows back ends if those OSes don't support the full set of events. Maybe in a follow-up, after we know what the other-OS support code will have to look like.
* I pushed this to try server and got a bunch of orange, but it looks like tracemonkey itself gets a lot of orange right now, and I don't see how the addition of this code could have caused the failures I'm seeing.
Therefore, I'm requesting formal approval to land this on the tracemonkey tree after ted checks the build logic.

One of the differences between Firefox and the rest (SeaMonkey, Thunderbird, etc) is that Firefox is build with libxul, the rest are not (yet). That might be the reason for the failure here. One way to test would be to build Firefox with --disable-libxul and --disable-ipc.
Robert, when it SeaMonkey switching to libxul builds?

(In reply to comment #43)
> One of the differences between Firefox and the rest (SeaMonkey, Thunderbird,
> etc) is that Firefox is build with libxul, the rest are not (yet). That might
> be the reason for the failure here. One way to test would be to build Firefox
> with --disable-libxul and --disable-ipc.
And --enable-static possibly, which is where we're seeing this happening, but it might already be blocked from being used with Firefox.
> Robert, when it SeaMonkey switching to libxul builds?
As soon as the mailnews folks have fixed their code to build with it (I can't wait, personally, and am about as unhappy about it as you guys).
And this affects Thunderbird the same, btw, it's not just SeaMonkey.

Created attachment 461955[details][diff][review]
fix seamonkey build failures
I managed to reproduce the problem on my home computer, and this change makes the link failure go away, but I am not confident that it doesn't break anything else. (I'm pretty confident that $(MODULE_NAME) is only used to generate the list of modules that go into nsStaticComponents.cpp, and that that needs to match the name in the MODULE_DEFN line in PerfMeasurement.cpp - what I don't understand is why we also have $(MODULE), what that might need to match, or under what circumstances it might be necessary for MODULE and MODULE_NAME to be different.)

(In reply to comment #36)
> Created attachment 461362[details][diff][review]
> r6: against tracemonkey, more tweaks[Checkin: comment 39]
>
> This should be the final version of this patch. It sounds to me like neither
> cjones nor waldo wants to review again, but I did hit an OSX-specific build
> system problem that I *think* I have fixed correctly but am not 100% sure of,
> so I'd like ted to check me on that. The deal is, both t/c/ctypes and t/c/perf
> were generating object files named Module.o. They were going into different .a
> libraries, but on OSX only, toolkit/library explodes those into their
> individual object files again, so we got two copies of the ctypes Module.o and
> the link failed. I fixed this by renaming both ctypes' and perf's Module.cpp.
Yeah, the Mac build is stupid like that. Your fix sounds reasonable.