568863 - Performance measurement utility

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Zack Weinberg (:zwol)]

Attachment: 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.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: 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.)

Comment 2

[Zack Weinberg (:zwol)]

Attachment: 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.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are these numbers from our process only, or from all processes, or what?

Why go through an XPCOM interface here with virtual calls?

Comment 4

[Zack Weinberg (:zwol)]

(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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Note, one way to provide JS access could be to use JS-ctypes.

Comment 7

[Zack Weinberg (:zwol)]

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 8

[Zack Weinberg (:zwol)]

cc:ing Vlad for possible insight into how to implement this on Windows and OSX

Comment 9

[Benjamin Smedberg]

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).

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 11

[Zack Weinberg (:zwol)]

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?

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 13

[Zack Weinberg (:zwol)]

Attachment: 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.

Comment 14

[Zack Weinberg (:zwol)]

Attachment: 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.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 452413 [details] [diff] [review]
re-revised: native JSAPI class, accessible from chrome via JSM

This is hot.

>diff --git a/js/src/perf/jsperf.h b/js/src/perf/jsperf.h
>new file mode 100644
>--- /dev/null
>+++ b/js/src/perf/jsperf.h
...
>+class JS_FRIEND_API(PerfMeasurement) {
...
>+  /**
>+   * 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.

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.  I'd like to do
something like
http://hg.mozilla.org/mozilla-central/file/e11c0cb8fbb2/ipc/ipdl/test/cxx/TestLatency.cpp#l32
with your new class, for example.

>+  /**
>+   * Stop timing and set counter values.
>+   */
>+  void Stop();
>+};
>+
>+/**
>+ * RegisterPerfMeasurement injects a Javascript wrapper around the
>+ * above C++ class into the Javascript object passed as an argument
>+ * (this will normally be a global object).
>+ *
>+ * The only visible differences between the C++ class and the
>+ * Javascript wrapper are: in the Javascript wrapper, the counter
>+ * attributes are read-only, and the Start() and Stop() methods are
>+ * lowercased (start() and stop()).

Style note: since you're in js/src, you don't need to follow silly
Gecko rules, 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.

r- just because I'd like to review a version with the interface changes.

Comment 16

[Zack Weinberg (:zwol)]

(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.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #16)
> > 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/?

https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style

/be

Comment 19

[Zack Weinberg (:zwol)]

Attachment: 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.

Comment 20

[Zack Weinberg (:zwol)]

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.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 453239 [details] [diff] [review]
revised x3: cjones' requests, style fixes, add unwrapper for use with jsvals

>diff --git a/js/src/perf/pm_linux.cpp b/js/src/perf/pm_linux.cpp
>new file mode 100644
>--- /dev/null
>+++ b/js/src/perf/pm_linux.cpp
[snip]
>+EventMask
>+Impl::init(EventMask toMeasure)
>+{
>+    JS_ASSERT(group_leader == -1);
>+    if (!toMeasure)
>+        return EventMask(0);
>+
>+    EventMask measured = EventMask(0);
>+    struct perf_event_attr attr;
>+    for (int i = 0; i < PerfMeasurement::NUM_MEASURABLE_EVENTS; i++) {
>+        if (!(toMeasure & kSlots[i].bit))
>+            continue;
>+
>+        memset(&attr, 0, sizeof(attr));
>+        attr.size = sizeof(attr);
>+
>+        // Set the type and config fields to indicate the counter we
>+        // want to enable.  We want read format 0, and we're not using
>+        // sampling, so leave those fields unset.
>+        attr.type = kSlots[i].type;
>+        attr.config = kSlots[i].config;
>+
>+        // If this will be the group leader it should start off
>+        // disabled.  Otherwise it should start off enabled (but blocked
>+        // on the group leader).
>+        if (group_leader == -1)
>+            attr.disabled = 1;
>+
>+        // The rest of the bit fields are really poorly documented.
>+        // F'rinstance, I have *no idea* whether we should be setting
>+        // the inherit, inherit_stat, or task flags.  I'm pretty sure
>+        // we do want to set mmap and comm, and not any of the ones I
>+        // haven't mentioned.
>+        attr.mmap = 1;
>+        attr.comm = 1;
>+

I'm pretty sure that we don't want |inherit|.  It might be nice to
expose the user/kernel/hv/idle flags somehow, but that's definitely
material for a followup.

>+        int fd = syscall(__NR_perf_event_open, &attr,
>+                         0 /* trace self */, -1 /* on any cpu */,
>+                         group_leader, 0);

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.

[snip]
>+namespace JS {
>+
>+#define initCtr(flag) ((eventsMeasured & flag) ? 0 : -1)
>+
>+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.

Looks good.  Off to Waldo for JSAPI review.

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 453239 [details] [diff] [review]
revised x3: cjones' requests, style fixes, add unwrapper for use with jsvals

>diff --git a/js/src/Makefile.in b/js/src/Makefile.in

>+# PerfMeasurement is available regardless of low-level support for it,
>+# it just doesn't necessarily do anything useful.  There is one

Run-on here, semicolon or "but" or something.


>diff --git a/js/src/perf/jsperf.cpp b/js/src/perf/jsperf.cpp

>+#define PM_FATTRS \
>+    (JSPROP_READONLY|JSPROP_PERMANENT|JSPROP_SHARED)

const uint8 for this these days.


>+// 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 said, no obviously better solution comes immediately to mind.  :-\


>+#define PM_PATTRS \
>+    (JSPROP_ENUMERATE|JSPROP_READONLY|JSPROP_PERMANENT|JSPROP_SHARED)

More const uint8.


>+static JSPropertySpec pm_consts[] = {
>+    { "CPU_CYCLES",             -1, PM_PATTRS, pm_getconst, 0 },
>+    { "INSTRUCTIONS",           -2, PM_PATTRS, pm_getconst, 0 },
>+    { "CACHE_REFERENCES",       -3, PM_PATTRS, pm_getconst, 0 },
>+    { "CACHE_MISSES",           -4, PM_PATTRS, pm_getconst, 0 },
>+    { "BRANCH_INSTRUCTIONS",    -5, PM_PATTRS, pm_getconst, 0 },
>+    { "BRANCH_MISSES",          -6, PM_PATTRS, pm_getconst, 0 },
>+    { "BUS_CYCLES",             -7, PM_PATTRS, pm_getconst, 0 },
>+    { "PAGE_FAULTS",            -8, PM_PATTRS, pm_getconst, 0 },
>+    { "MAJOR_PAGE_FAULTS",      -9, PM_PATTRS, pm_getconst, 0 },
>+    { "CONTEXT_SWITCHES",      -10, PM_PATTRS, pm_getconst, 0 },
>+    { "CPU_MIGRATIONS",        -11, PM_PATTRS, pm_getconst, 0 },
>+    { "ALL",                   -12, PM_PATTRS, pm_getconst, 0 },
>+    { "NUM_MEASURABLE_EVENTS", -13, PM_PATTRS, pm_getconst, 0 },
>+    {0,0,0,0,0}
>+};

These properties would be better defined directly as values, rather than by using a getter function.  That would also let you get rid of the JSPROP_SHARED here as well, sticking to standard ES5isms (if sometimes with different names and/or inverted meaning) for more clarity.


>+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.  In this context at this time it'll always be threadsafe, but that might not always be the case.  But...

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. 


>+static JSBool
>+pm_construct(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+{
>+    uint32 mask;
>+    if (!JS_ConvertArguments(cx, argc, argv, "u", &mask))
>+        return JS_FALSE;
>+
>+    if (!JS_SealObject(cx, obj, JS_FALSE))
>+        return JS_FALSE;
>+
>+    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).


>+static void
>+pm_finalize(JSContext *cx, JSObject *obj)
>+{
>+    PerfMeasurement *p = (PerfMeasurement*) JS_GetPrivate(cx, obj);
>+    delete p;
>+}

No need for a local variable here.


>+static JSBool
>+pm_getmask(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
>+{
>+    PerfMeasurement *p = GetPM(cx, obj, "eventsMeasured");
>+    if (!p) return JS_FALSE;

Two lines here, for easier debugging.


>+    if (!JSVAL_IS_INT(id))
>+        return JS_TRUE; // unknown property
>+
>+    int tinyid = -JSVAL_TO_INT(id) - 1;
>+    if (tinyid < 0 || tinyid >= (int)JS_ARRAY_LENGTH(masks))
>+        return JS_TRUE; // unknown property

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.


>+// Calls
>+
>+static JSBool
>+pm_start(JSContext *cx, uintN argc, jsval *vp)
>+{
>+    PerfMeasurement *p = GetPM(cx, JS_THIS_OBJECT(cx, vp), "start");
>+    if (!p)
>+        return JS_FALSE;

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.)


>+namespace JS {

namespace js


>+PerfMeasurement*
>+ExtractPerfMeasurement(jsval wrapper)
>+{
>+    if (!JSVAL_IS_OBJECT(wrapper))
>+        return 0;
>+
>+    JSObject *obj = JSVAL_TO_OBJECT(wrapper);
>+
>+    // This is what JS_GetInstancePrivate does internally.  We can't
>+    // call JS_anything from here, because we don't have a JSContext.
>+    if (!obj || obj->getClass() != &pm_class)
>+        return 0;

May be slightly more pleasant here to hide the null-check behind JSVAL_IS_PRIMITIVE, up to you.


>diff --git a/js/src/perf/jsperf.h b/js/src/perf/jsperf.h

>+namespace JS {

namespace js


>+/**

JS doesn't javadoc-ify its comments with double-*, for whatever reason.


>+ * To use this API, create a PerfMeasurement object, passing its
>+ * constructor a bitmask indicating which events you are interested
>+ * in.  Thereafter, Start() zeros all counters and starts timing;

I think "zeroes" is the more common spelling.


>+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?  I think we want to avoid it if possible (no idea whether it is or not, seems to me having the class definition would suffice, but I could be wrong).

Class brace goes on new line:

class PerfMeasurement
{


>diff --git a/js/src/perf/pm_linux.cpp b/js/src/perf/pm_linux.cpp

>+Impl::Impl()
>+    : f_cpu_cycles(-1),
>+      f_instructions(-1),
>+      f_cache_references(-1),
>+      f_cache_misses(-1),
>+      f_branch_instructions(-1),
>+      f_branch_misses(-1),
>+      f_bus_cycles(-1),
>+      f_page_faults(-1),
>+      f_major_page_faults(-1),
>+      f_context_switches(-1),
>+      f_cpu_migrations(-1),
>+      group_leader(-1),
>+      running(false)

Two-space indent for these:

Impl::Impl()
  : f_cpu_cycles(-1),
    f_instructions(-1),
    ...


>diff --git a/toolkit/components/perf/Module.cpp b/toolkit/components/perf/Module.cpp

>+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.  :-)  Basically, assume any JSAPI action could trigger a GC, which kills anything that's not rooted -- and note that pointers on the stack don't count as roots.  (This is changing really soon, but it has not changed yet, the worse for you now.)  This function gets used with things which could conceivably be overwritten and not be guaranteed to have exact values, so you do have to worry about this.  (Well, technically you don't because this is a privileged API, but I don't want to see us rely on that, possibly leading some future reader astray.

Basically, anywhere you have a jsval, replace it with js::AutoValueRooter and use the value() and addr() methods to interact with it.  Anywhere you have a JSObject*, replace it with a js::AutoObjectRooter and use object() and addr() in basically the same way.  The GC knows about the values hidden inside rooters and will trace over them during garbage collection, keeping whatever they have inside them alive through the GC.


>+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.

Comment 23

[Brendan Eich [:brendan]]

(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

Comment 24

[Zack Weinberg (:zwol)]

Attachment: 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.

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 457205 [details] [diff] [review]
revised x4: most of waldo's change requests

Just re-skimmed based on comment 15 and comment 21 ...

(In reply to comment #21)
> Comment on attachment 453239 [details] [diff] [review]
> >+        int fd = syscall(__NR_perf_event_open, &attr,
> >+                         0 /* trace self */, -1 /* on any cpu */,
> >+                         group_leader, 0);
> 
> 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.

> [snip]
> >+namespace JS {
> >+
> >+#define initCtr(flag) ((eventsMeasured & flag) ? 0 : -1)
> >+
> >+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.

Comment 26

[Zack Weinberg (:zwol)]

Attachment: 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 27

[Jeff Walden [:Waldo]]

(In reply to comment #25)
> > 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.

Correctomundo.

Comment 28

[Jeff Walden [:Waldo]]

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.

Comment 29

[Zack Weinberg (:zwol)]

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...

Comment 30

[Brendan Eich [:brendan]]

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

Comment 31

[Jeff Walden [:Waldo]]

(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.

Comment 32

[Luke Wagner [:luke]]

If calling ExtractPerfmeasurement is performance-sensitive at all, it would be slightly faster, for the sake of 32-bit, to pass by const jsval &.

Comment 33

[Jeff Walden [:Waldo]]

(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.

Comment 34

[Johnny Stenback (:jst)]

Comment on attachment 457957 [details] [diff] [review]
r5: OOM & syscall wrappers in pm_linux.cpp

+Module::Call(nsIXPConnectWrappedNative* wrapper,
+             JSContext* cx,
+             JSObject* obj,
+             PRUint32 argc,
+             jsval* argv,
+             jsval* vp,
+             PRBool* _retval)
+{
+  JSObject* global = JS_GetGlobalObject(cx);

Duh, I completely missed the questions in this bug that were directed directly at me. Waldo is right, you should use JS_GetGlobalForObject(cx, JS_GetScopeChain(cx)) in Module::Call(), and throw if JS_GetScopeChain(cx) returns null for some reason (which it really shouldn't here).

r=jst with that changed.

Comment 35

[Brendan Eich [:brendan]]

In comment 33, Waldo is referring to a "map macro" technique I've started using instead of the .tbl-file method (jsopcode.tbl) for practicing DRY in C or C++. Worth a second look.

/be

Comment 36

[Zack Weinberg (:zwol)]

Attachment: 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.

Comment 37

[Mitchell Field [:Mitch]]

Comment on attachment 461362 [details] [diff] [review]
r6: against tracemonkey, more tweaks[Checkin: comment 39]

Cautious r+ from me. I'm not an OSX user, but these changes look okay in general. Changes can be backed out if need be.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 461362 [details] [diff] [review]
r6: against tracemonkey, more tweaks[Checkin: comment 39]

a=bzbarsky

Comment 39

[Zack Weinberg (:zwol)]

Landed http://hg.mozilla.org/tracemonkey/rev/0406c2681957

Comment 40

[Zack Weinberg (:zwol)]

I've written up some rough documentation here:

https://developer.mozilla.org/en/JS::PerfMeasurement
https://developer.mozilla.org/en/JavaScript/Code_modules/PerfMeasurement.jsm

It needs a whole lot of polish.  I'm happy to answer questions, but I think I have taken it as far as I can.

Comment 41

[Robert Kaiser]

This seems to have broken static builds of SeaMonkey and Thunderbird, see e.g. http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1280647800.1280654989.21938.gz

nsStaticComponents.o: In function `global constructors keyed to kPStaticModules':
/builds/slave/comm-central-trunk-linux-nightly/build/objdir/suite/app/nsStaticComponents.cpp.in:71: undefined reference to `perf_NSModule'
/usr/bin/ld: seamonkey-bin: hidden symbol `perf_NSModule' isn't defined

Comment 42

[Zack Weinberg (:zwol)]

I'm not at all familiar with the differences between moz-central and comm-central; having added a directory below toolkit/components in the former, what needs to be done for the latter to pick it up?

Comment 43

[Johnny Stenback (:jst)]

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?

Comment 44

[Robert Kaiser]

(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.

Comment 45

[Zack Weinberg (:zwol)]

Attachment: 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.)

Comment 46

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 461955 [details] [diff] [review]
fix seamonkey build failures

Looks reasonable enough.

Comment 47

[Zack Weinberg (:zwol)]

I'm not able to watch the tree till tomorrow morning. If anyone would like to go ahead and land this before then please feel free.

Comment 48

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/d57ef967d07e

Comment 49

[(not currently active) Ted Mielczarek]

(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.

Comment 50

[Zack Weinberg (:zwol)]

Thanks for pushing the seamonkey fix, kyle.

Comment 51

[Steve Scott (pxbugz)]

I think this patch might be a candidate for causing a regression on http://demos.hacks.mozilla.org/openweb/CSSMAKESUSICK/ as it uses JS for manipulating images.

The images are low res when moving the mouse or in the centre of the screen. Move mouse near an edge and they switch to hi-res until you move the mouse again.

I checked hourlies and 1280718690 is fine, next one is 1280722154 which exhibits this issue.

Patch range between those builds http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2010-08-01+20%3A31&enddate=2010-08-01+22%3A33
I can't see any other candidates there for this regression.

I posted on mozillazine with screenshots http://forums.mozillazine.org/viewtopic.php?f=23&t=1961573&start=26

Comment 52

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

None of those patches should have caused any change in the Firefox binary.

Comment 53

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Though, if we expand your range slightly, there is http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=b1d534754b62 which is full of all sorts of graphics patches.  I would take a look at the layers for 2D transforms one.

Comment 54

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Please file a new bug for that and CC me

Comment 55

[Zack Weinberg (:zwol)]

Attachment: follow-up fix: add reset() to JS-accessible API

Minor oversight: reset() never got added to the JavaScript wrapper interface.

Comment 56

[Zack Weinberg (:zwol)]

Follow-up landed: http://hg.mozilla.org/mozilla-central/rev/e103582a72ca

Comment 57

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0406c2681957

Comment 58

[Eric Shepherd [:sheppy]]

This was mostly written already, but I cleaned it up, reorganized it, and added bits and pieces. Also added a new top-level "Performance" page to link to performance-related articles from; this page will be built out over the next little bit.

https://developer.mozilla.org/en/Performance
https://developer.mozilla.org/en/JavaScript_code_modules/PerfMeasurement.jsm
https://developer.mozilla.org/en/Performance/Measuring_performance_using_the_PerfMeasurement.jsm_code_module
https://developer.mozilla.org/en/Performance/JS::PerfMeasurement

Also linked to from Fx4 for developers.