548702 - Convert JS temp root API to be C++-based rather than C++ wrapped around macros

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Proto-patch

+++ This bug was initially created as a clone of Bug #538690 +++

That bug's getting long in the tooth and has been sidetracked into this concern, which in terms of code changes thoroughly swallows it.  Do this first, then that bug's concern can be fixed, and in about a 2K patch no less.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Er, attached the wrong proto-patch

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Base patch for workaround comparisons

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Workaround patch atop base patch

Comment 4

[Jeff Walden [:Waldo]]

In my testing of a workaround patch versus the pure C++ object-style patch on SunSpider, I find that the workaround generating fewer instructions actually benchmarks *slower* overall than the clean C++ version, by 0.6%-0.8% or so, on an otherwise non-busy buildmonkey-left.  (An unmodified tree benchmarks slightly worse than the clean C++ version and slightly better than the workaround.)  I added some extra code to each patch to double-check that the optimization expected to be present in the workaround actually functioned, and indeed it was being applied.  I added the following snippets at the very start of jsarray.cpp's EnsureCapacity, generating this assembly via |make jsarray.s|:


JSBool foopy(JSContext *cx);
{  
  SAVER_VALUE(cx, cx->runtime->NaNValue, tvr);
  foopy(cx);
}
        movq    %rdi, %r12            // save cx
        movq    %rsi, %r13            // save obj
        movl    %edx, %r14d           // save newCap
        movl    %ecx, %r15d           // save initializeAllSlots
        movq    216(%rdi), %rax       // rt = cx->runtime
        movq    600(%rax), %rdx       // nan = rt->NaNValue
        movq    856(%rdi), %rax       // tvrs = cx->tempValueRooters
        movq    %rax, -80(%rbp)       // tvr.down = tvrs
        movq    $-1, -72(%rbp)        // tvr.tag = JSVAL
        leaq    -80(%rbp), %rax       // &tvr
        movq    %rax, 856(%rdi)       // cx->tempValueRooters = &tvr
        movq    %rdx, -64(%rbp)       // tvr.val = nan
        call    __Z5foopyP9JSContext  // ...destroy the world
        movq    -80(%rbp), %rax       // previousTvrs = tvr.down
        movq    %rax, 856(%r12)       // cx->tempValueRooters = previousTvrs

JSBool foopy(JSContext *cx);
{
  JSAutoTempValueRooter tvr(cx, cx->runtime->NaNValue);
  foopy(cx);
}
        movq    %rdi, %r12            // save cx
        movq    %rsi, %r13            // save obj
        movl    %edx, %r14d           // save newCap
        movl    %ecx, %r15d           // save initializeAllSlots
        movq    216(%rdi), %rax       // rt = cx->runtime
        movq    600(%rax), %rdx       // nan = rt->NaNValue
        movq    856(%rdi), %rax       // tvrs = cx->tempValueRooters
        movq    %rax, -80(%rbp)       // tvr.down = tvrs
        movq    $-1, -72(%rbp)        // tvr.tag = JSVAL
        movq    %rdi, -64(%rbp)       // ********** save cx
        leaq    -80(%rbp), %rax       // &tvr
        movq    %rax, 856(%rdi)       // cx-tempValueRooters = &tvr
        movq    %rdx, -56(%rbp)       // tvr.val = nan
        call    __Z5foopyP9JSContext  // ...destroy the world
        movq    -80(%rbp), %rdx       // previousTvrs = tvr.down
        movq    -64(%rbp), %rax       // ********** restore cx
        movq    %rdx, 856(%rax)       // cx->tempValueRooters = previousTvrs

Based on these results, I conclude that the workaround isn't worth pursuing.  I'll complete a version of the clean C++ patch either over the weekend or on Monday; I'd really like to get this and the value array patch in so that I can get back to getOwnPropertyNames (which relies on the value array patch).

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Patch for review

Okay, this should be good to go now.

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 429601 [details] [diff] [review]
Patch for review

Drive-by nits and encouragement -- it's great to see the simpler patch prevail on perf as well as source simplicity!

>+      out:
>+        /*
>+         * XXX This is unfortunately necessary due to multiply-nested loops.
>+         *     Eventually we should change vec to be a js::Vector, at which
>+         *     point we can make it a local to this block, and it'll clean
>+         *     itself up automatically.  AutoValueArray is another possibility,
>+         *     but it may not lend itself to the very low-level count-fiddling
>+         *     currently used here.
>+         */
>+        ;

Please use FIXME: bug nnnnnn instead of XXX comments. Here, the "This" referring to the labeled empty statement is a little late -- you could specify the reference ("The out label is unfortunately necessary...") but that is better written up in the bug to cite via the FIXME, if this is gonna get fixed (I bet you are right to suspect the count-fiddling being a hindrance to goto elimination, but is the count-fiddling really needed for perf? You are on a roll, keep going! ;-)

>+class TempValueRooter {
>+  public:
>+    TempValueRooter(JSContext *cx, ptrdiff_t tag)
>+      : down(cx->tempValueRooters), tag(tag), context(cx)
>+    {

Note prevailing style observed here, both for : overflow-line indentation and { on its own line due to extends-clause overflow.

>+    enum {
>+        /* jsval */
>+        JSVAL = -1,
>+        /* JSAutoScopePropertyTreeRooter */
>+        SPROP = -2,
>+        /* AutoSaveWeakRoots */
>+        WEAKROOTS = -3,
>+        /* JSCompiler */
>+        COMPILER = -4,
>+        /* JSAutoScriptRooter */
>+        SCRIPT = -5,
>+        /* JSAutoEnumStateRooter */
>+        ENUMERATOR = -6,
>+        /* JSAutoIdArray */
>+        IDARRAY = -7,
>+        /* AutoDescriptorArray */
>+        DESCRIPTORS = -8,
>+        /* AutoNamespaceArray */
>+        NAMESPACES = -9,
>+        /* JSAutoXML */
>+        XML = -10

This would read a lot better with = indented to same column and comments on the right of each enumerator. As it is, the ragged-middle-margin for = and the unrelated comment-lines obscure the definitions.

>+/* FIXME(bug 332648): Move this into a public header. */
>+class JSAutoTempValueRooter : private js::TempValueRooter
>+{
>+  public:
>     explicit JSAutoTempValueRooter(JSContext *cx, jsval v = JSVAL_NULL
>                                    JS_GUARD_OBJECT_NOTIFIER_PARAM)
>-        : mContext(cx) {
>+        : js::TempValueRooter(cx, JSVAL), val(v)

Indentation off by two for the : overflow line.

>+class JSAutoObjectRooter : private JSAutoTempValueRooter {
>+  public:
>+    JSAutoObjectRooter(JSContext *cx, JSObject *obj = NULL
>+                       JS_GUARD_OBJECT_NOTIFIER_PARAM)
>+        : JSAutoTempValueRooter(cx, obj) {

Both : indentation and { not on own line nits here and several more times below.

/be

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Updated, another auto object, no funky labeling

Style nits addressed, also became motivated enough to replace manual vec-freeing with an auto object, which meant I could kill a lot of gotos and boolean sets -- lost a bit over a dozen lines from that change.  :-)

Comment 8

[Igor Bukanov]

(In reply to comment #7)
> Created an attachment (id=429907) [details]
> Updated, another auto object, no funky labeling
> 
> Style nits addressed, also became motivated enough to replace manual
> vec-freeing with an auto object, which meant I could kill a lot of gotos and
> boolean sets -- lost a bit over a dozen lines from that change.  :-)

Could you attach diff -b version of the patch? It contains quite a few indentation-only changes.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Without whitespace changes

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Er, without ASCII coloring

...but if you want, you can pipe the other into |less -R| and still use it.

Comment 11

[Igor Bukanov]

> diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp
...
> @@ -668,39 +665,34 @@ array_length_setter(JSContext *cx, JSObj
> -            ok = (JS_CHECK_OPERATION_LIMIT(cx) &&
> -                  JS_NextProperty(cx, iter, &id));
> -            if (!ok)
> -                break;
> +            if (!JS_CHECK_OPERATION_LIMIT(cx) || !JS_NextProperty(cx, iter, &id))
> +                return JS_FALSE;
> 
Nit: do not introduce a new return JS_FALSE. Instead chnage all return JS_FALSE|TRUE in a method that you touch to false/true.
> -        JS_POP_TEMP_ROOT(cx, &tvr);
> -        if (!ok)
> -            return JS_FALSE;
>      }
>  
>      obj->fslots[JSSLOT_ARRAY_LENGTH] = newlen;
>      return JS_TRUE;
> 

That should be "return true" per nit above.

> @@ -1713,17 +1705,17 @@ InitArrayElements(JSContext *cx, JSObjec
>      JS_ASSERT(start == MAXINDEX);
>      jsval tmp[2] = {JSVAL_NULL, JSVAL_NULL};
> -    JSAutoTempValueRooter tvr(cx, JS_ARRAY_LENGTH(tmp), tmp);
> +    JSAutoArrayRooter tvr(cx, JS_ARRAY_LENGTH(tmp), tmp);
> 

I would prefer to have a separated auto-router for fixed-length arrays that would initialize the values to JSVAL_NULL and provide a convinience operator[] to accessing the elements. But that can be done in a separated patch or separated bug.

>  static JSBool
>  array_sort(JSContext *cx, uintN argc, jsval *vp)
>  {
> -    JS_PUSH_TEMP_ROOT(cx, 0, vec, &tvr);
> +    {
...
> +        JSAutoArrayRooter tvr(cx, 0, vec);
>  

We should add JSAutoRootedVector here that grows as necessary. That would provide a better way to deal with sparse arrays than relying on OS not to commit memory for a huge malloc immediately. But again, that can go to a separated bug.

> -        ok = JS_CHECK_OPERATION_LIMIT(cx);
> -        if (!ok)
> -            goto out;
> +            if (!JS_CHECK_OPERATION_LIMIT(cx))
> +                return JS_FALSE;
> 
> 

Nit: As before, convert all boolean return to use false/true

> diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h
...
> +static inline void
> +TRACE_JSVALS(JSTracer *trc, size_t len, jsval *vec, const char *name)
> 

Nit why ALL-CAPS name? The patch size increase of changing it to js_TraceValues should be minimal. Also this function belongs to jsgc.h, not here. And make it just inline.

> -class JSAutoTempIdRooter
> +class JSAutoObjectRooter : private JSAutoTempValueRooter {
...
> +    JSObject ** addr() {
> +        JS_ASSERT(JSVAL_OBJECT == 0);
> +        return reinterpret_cast<JSObject **>(JSAutoTempValueRooter::addr());
> 
Hm, don't that add an alising warning with GCC? A better approach would be to add separated OBJECT tag and just use a JSObject* field here.

> +class JSAutoTempIdRooter : private JSAutoTempValueRooter
...
> -    jsid id() { return (jsid) mTvr.u.value; }
> -    jsid * addr() { return (jsid *) &mTvr.u.value; }
> +    jsid * addr() {
> +        return reinterpret_cast<jsid *>(JSAutoTempValueRooter::addr());
> +    }
> 

Again, the casts are ugly here. Replace it with a special ID tag and proper jsid field.

> diff --git a/js/src/jsexn.cpp b/js/src/jsexn.cpp
> --- a/js/src/jsexn.cpp
> +++ b/js/src/jsexn.cpp
> @@ -819,62 +819,56 @@ exn_toString(JSContext *cx, uintN argc, 
> -    JS_PUSH_TEMP_ROOT(cx, 3, localroots, &tvr);
> +    {
> +        JSAutoArrayRooter tvr(cx, JS_ARRAY_LENGTH(localroots), localroots);

A special fixed-size auto array would be helpful here as well.

>  
>  #ifdef __GNUC__
>      message = filename = NULL;
>  #endif
> -    ok = JS_GetProperty(cx, obj, js_message_str, &localroots[0]) &&
> -         (message = js_ValueToSource(cx, localroots[0]));
> -    if (!ok)
> -        goto out;
> +        if (!JS_GetProperty(cx, obj, js_message_str, &localroots[0]) ||
> +            !(message = js_ValueToSource(cx, localroots[0]))) {
> +            return JS_FALSE;

Nit: use return boolean here and everywhere in the function.

> @@ -1246,61 +1230,48 @@ js_ReportUncaughtException(JSContext *cx
> -        if (!bytes) {
> -            ok = JS_FALSE;
> -            goto out;
> +        if (!bytes)
> +            return JS_FALSE;

Nit: use return boolean here and everywhere in the function.

> diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>      if (!JS_XDRUint32(xdr, &localsword) ||
>          !JS_XDRUint32(xdr, &flagsword)) {
> -        goto bad;
> +        return JS_FALSE;

Nit: use return boolean here and everywhere in the function.

> diff --git a/js/src/jsiter.cpp b/js/src/jsiter.cpp
> --- a/js/src/jsiter.cpp
> +++ b/js/src/jsiter.cpp
> @@ -210,28 +210,21 @@ Iterator(JSContext *cx, JSObject *iterob
>  
>      *rval = argv[0];
>      return js_ValueToIterator(cx, flags, rval);
>  }
>  
>  static JSBool
>  NewKeyValuePair(JSContext *cx, jsid key, jsval val, jsval *rval)
>  {
> -    jsval vec[2];
> -    JSTempValueRooter tvr;
> -    JSObject *aobj;
> +    jsval vec[2] = { ID_TO_VALUE(key), val };
> +    JSAutoArrayRooter tvr(cx, 2, vec);

Consider fixed-size array rooter here.

> +    JSAutoTempValueRooter tvr(cx);
> @@ -380,71 +373,64 @@ js_ValueToIterator(JSContext *cx, uintN 
>          } else {
>              obj = js_ValueToNonNullObject(cx, *vp);
>              if (!obj)
>                  return JS_FALSE;
>          }
>      }
>  
>      JS_ASSERT(obj);
> -    JS_PUSH_TEMP_ROOT_OBJECT(cx, obj, &tvr);
> +    *tvr.addr() = OBJECT_TO_JSVAL(obj);

I guess you have tried to refector this to avoid an extra tvr.addr(), but a jump to the default label prevents it, right?

>  
>      clasp = OBJ_GET_CLASS(cx, obj);
>      if ((clasp->flags & JSCLASS_IS_EXTENDED) &&
>          (xclasp = (JSExtendedClass *) clasp)->iteratorObject) {
>          iterobj = xclasp->iteratorObject(cx, obj, !(flags & JSITER_FOREACH));
>          if (!iterobj)
> -            goto bad;
> +            return JS_FALSE;

Nit: use return boolean here and everywhere in the function.

> diff --git a/js/src/json.cpp b/js/src/json.cpp
> @@ -253,17 +250,17 @@ static JSBool
>  JO(JSContext *cx, jsval *vp, StringifyContext *scx)
>  {
>      JSObject *obj = JSVAL_TO_OBJECT(*vp);
>  
>      if (!scx->cb.append('{'))
>          return JS_FALSE;
>  
>      jsval vec[3] = {JSVAL_NULL, JSVAL_NULL, JSVAL_NULL};
> -    JSAutoTempValueRooter tvr(cx, 3, vec);
> +    JSAutoArrayRooter tvr(cx, JS_ARRAY_LENGTH(vec), vec);

Consider fixed-size array rooter here.

> --- a/js/src/xpconnect/src/xpcquickstubs.h
> +++ b/js/src/xpconnect/src/xpcquickstubs.h
> @@ -316,17 +316,17 @@ struct xpc_qsSelfRef
>  template<size_t N>
>  struct xpc_qsArgValArray
>  {
>      xpc_qsArgValArray(JSContext *cx) : tvr(cx, N, array)
>      {
>          memset(array, 0, N * sizeof(jsval));
>      }
>  
> -    JSAutoTempValueRooter tvr;
> +    JSAutoArrayRooter tvr;
>      jsval array[N];

This could be replaced with a fixed size array router.
> diff --git a/modules/plugin/base/src/nsJSNPRuntime.cpp b/modules/plugin/base/src/nsJSNPRuntime.cpp
> --- a/modules/plugin/base/src/nsJSNPRuntime.cpp
> +++ b/modules/plugin/base/src/nsJSNPRuntime.cpp
> +  {
> +    JSAutoArrayRooter tvr(cx, 0, jsargs);

This router is similar to the router in the merge sort.

I will quickly review a patch addressing this quickly.

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #11)
> Nit: do not introduce a new return JS_FALSE. Instead chnage all return
> JS_FALSE|TRUE in a method that you touch to false/true.

I've registered this dislike on IRC, and even if everyone else disagrees, I still want to restate it: I don't like interchanging true/false and JS_TRUE/JS_FALSE.  I don't think this:

long canFrobble() { return true; }

looks at all natural, and I don't think this:

JSBool canFrobble() { return true; }

makes it any different.  JSBool and bool are not the same type, even if they often interconvert.


> > @@ -1713,17 +1705,17 @@ InitArrayElements(JSContext *cx, JSObjec
> >      JS_ASSERT(start == MAXINDEX);
> >      jsval tmp[2] = {JSVAL_NULL, JSVAL_NULL};
> > -    JSAutoTempValueRooter tvr(cx, JS_ARRAY_LENGTH(tmp), tmp);
> > +    JSAutoArrayRooter tvr(cx, JS_ARRAY_LENGTH(tmp), tmp);
> > 
> 
> I would prefer to have a separated auto-router for fixed-length arrays that
> would initialize the values to JSVAL_NULL and provide a convinience operator[]
> to accessing the elements. But that can be done in a separated patch or
> separated bug.

Separate, yes.


> >  static JSBool
> >  array_sort(JSContext *cx, uintN argc, jsval *vp)
> >  {
> > -    JS_PUSH_TEMP_ROOT(cx, 0, vec, &tvr);
> > +    {
> ...
> > +        JSAutoArrayRooter tvr(cx, 0, vec);
> >  
> 
> We should add JSAutoRootedVector here that grows as necessary. That would
> provide a better way to deal with sparse arrays than relying on OS not to
> commit memory for a huge malloc immediately. But again, that can go to a
> separated bug.

This is what bug 538690 was originally about!  But folding in all these changes here made the patch way too big, so the cleanup got split into this bug.  There's also a good part of me that's uncertain that this bit actually can be meaningfully cleaned up into some sort of non-memory-touching abstraction, but I've intentionally not tried to implement anything better here, so maybe my intuition is wrong.


> > +static inline void
> > +TRACE_JSVALS(JSTracer *trc, size_t len, jsval *vec, const char *name)
> > 
> 
> Nit why ALL-CAPS name? The patch size increase of changing it to js_TraceValues
> should be minimal. Also this function belongs to jsgc.h, not here. And make it
> just inline.

That's what the macro was named, and I was trying to make changes minimal, not maximal.  But I guess I can do this.


> > +    JSObject ** addr() {
> > +        JS_ASSERT(JSVAL_OBJECT == 0);
> > +        return reinterpret_cast<JSObject **>(JSAutoTempValueRooter::addr());
> > 
> Hm, don't that add an alising warning with GCC? A better approach would be to
> add separated OBJECT tag and just use a JSObject* field here.

Wasn't for me, simpler than letting a thousand tags bloom, but I can change.


> > +    JSAutoTempValueRooter tvr(cx);
> > @@ -380,71 +373,64 @@ js_ValueToIterator(JSContext *cx, uintN 
> >          } else {
> >              obj = js_ValueToNonNullObject(cx, *vp);
> >              if (!obj)
> >                  return JS_FALSE;
> >          }
> >      }
> >  
> >      JS_ASSERT(obj);
> > -    JS_PUSH_TEMP_ROOT_OBJECT(cx, obj, &tvr);
> > +    *tvr.addr() = OBJECT_TO_JSVAL(obj);
> 
> I guess you have tried to refector this to avoid an extra tvr.addr(), but a
> jump to the default label prevents it, right?

I tried to keep the patch minimal, except when a change clearly and completely invalidated the need for existing gotos.

Comment 13

[Brendan Eich [:brendan]]

The better reason besides reduced eyestrain to switch to true and false even if the return type cannot change yet from JSBool to bool is because we make progress in many steps, and we've explicitly favored this change on the road to JSBool elimination (or equation with bool -- ABI issues may hurt, see jsbuiltins.cpp comment, but perhaps it'll all just work).

A worse reason may be eyestrain but it counts.

In no case is "type purity" a good reason to overdo it, in C or C++ prevailing style. This is long-standing and not worth fighting over. Stay on target, fry the bigger fish!

/be

Comment 14

[Brendan Eich [:brendan]]

On IRC I told Waldo I'd back him in a bool-not-JSBool metabug. Cleaning up as we go in unrelated patches has been helpful, since there's so much to do, and of course JS_TRUE and JS_FALSE strain eyes, but we'll need some focused bool-only patches too.

/be

Comment 15

[Brendan Eich [:brendan]]

> Nit why ALL-CAPS name? The patch size increase of changing it to js_TraceValues

Must we use "js_" these days? I say no! Certainly not in namespace js {...}, but even if only in a private .h file for an inline, old-school js_ prefixing is not the thing we do in other such cases.

/be

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Nits (all?) fixed

Will get a -b patch to post next...

Comment 17

[Jeff Walden [:Waldo]]

Attachment: With whitespace ignored

Comment 18

[Igor Bukanov]

Comment on attachment 430689 [details] [diff] [review]
Nits (all?) fixed

>-class JSAutoTempValueRooter
>+namespace js {
>+
>+class TempValueRooter {

Nit: lets use the consistent naming since we are touching all these code and rename:

1. TempValueRooter -> AutoGCRooter

TempValueRooter has not been rooting jsval exclusively almost fromtnhe beginning. 


2. JSAutoTempValueRooter -> JSAutoValueRooter
   JSAutoXML -> JSAutoXMLRooter

The word "temp" was natural in C code but C++ style is to use Auto.

>diff --git a/js/src/jsparse.h b/js/src/jsparse.h
>--- a/js/src/jsparse.h
>+++ b/js/src/jsparse.h
>@@ -832,8 +832,10 @@ struct JSFunctionBoxQueue {
> 
>+struct JSCompiler : private js::TempValueRooter {
>+    JSContext           * const context; /* FIXME Use the context in TempValueRooter */

Nit: file a bug and cite it in the FIXME comments.

r+ with this addressed.

Comment 19

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/e7065853ef79 :-)
http://hg.mozilla.org/tracemonkey/rev/54ce0245d095 :-|
http://hg.mozilla.org/tracemonkey/rev/ecdf1586c148 :-|
http://hg.mozilla.org/tracemonkey/rev/dfcbd76281c0 :-\
http://hg.mozilla.org/tracemonkey/rev/2a6dbc1782c9 :-(

Cited bug 551291 in FIXME, renamed everything, made things even more hugeous than before.  Still need to file bug for fixed-size arrays kept alive by rooters, a shame we can't (I think) parametrize except manually...

Comment 20

[Jeff Walden [:Waldo]]

So, this apparently broke ARM in an inscrutable manner (sole output: Segmentation fault), and I didn't notice because the ARMboxen aren't on tbpl (boo-urns).  More immediate recognition and I'd have just backed out, but even suggesting it in the JS pit now resulted in howls of protest, so I'm going to work at figuring out the failure as soon as possible and just hope we can hold off on ->m-c merges for a little bit until I can figure out what's wrong.

Comment 21

[Jeff Walden [:Waldo]]

Hoped to get the ARM issue fixt by today, ran into troubles getting an ARM box set up and reproducing it -- regrettably, look for this on Monday.  :-\

Comment 22

[Jeff Walden [:Waldo]]

Haven't been able to reproduce problems on n900; so far n810 hasn't worked, but that's been entirely due to toolchain/build-config issues (seems development is focusing on n900 now, so n810 is kind of dying/dead, or such is my limited understanding), and whether or not something actually reproduces as not working is yet to be seen.

Comment 23

[Jeff Walden [:Waldo]]

So...as best as I can tell, something inexplicable is happening.  We're failing to load libxpcom, which causes XPCOMGlueLoad to fail, which brings things to a crashing halt.  According to dlerror(), the problem is:

/media/mmc2/fennec/xulrunner/libxpcom.so: undefined symbol: NS_CycleCollectorSuspect_P

I am absolutely puzzled as to how this would have been affected by any of the changes here.

However, the failure seems to start (or so I'd guess) a few lines earlier in that function when libxul fails to load, with this error message:

/usr/lib/libstdc++.so.6: version `GLIBCXX_3.4.5' not found (required by /media/mmc2/fennec/xulrunner/libxul.so)

This one suggests a toolchain mismatch of some sort, except this doesn't seem to make much sense, because mfinkle says he's seeing the same failure to load XPCOM, and his n810 build environment isn't newly set up like mine is, so it should have demonstrated such a problem much earlier.

I'm not sure what's up here yet, but at least I have a little more visibility into it.

Comment 24

[Julian Seward [:jseward]]

TM tip 39514:550c5bd4fbae built with gcc-4.4.1 on Ubuntu 9.10 (arm)
starts up (Fx) and runs OK, also with no obvious valgrindage problems.
So it doesn't seem like a problem with the code per se.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #24)
> TM tip 39514:550c5bd4fbae built with gcc-4.4.1 on Ubuntu 9.10 (arm)
> starts up (Fx) and runs OK, also with no obvious valgrindage problems.
> So it doesn't seem like a problem with the code per se.

Indeed. The exact same build that fails to start on the N810, works fine on the N900.

Comment 26

[Jeff Walden [:Waldo]]

Long time no comment...

I attempted to debug this on-device for quite awhile.  I tried setting up a build environment, and while I got one that produced a debug build, the build wouldn't run on the device for reasons which elude me.  I changed the tree to force-build debug with symbols on n810, to get a functioning build with symbols to debug; the resulting build would run on the device, but it frequently hung or slowed to a crawl, and I couldn't get it to reproduce the problem with a debugger hooked up to determine where things were going bad.  Eventually, after two weeks of effort I gave up, backed out, and started incrementally repushing -- keeping JSTempValueRooter and js::AutoGCRooter side by side, switching users from one to the other file by file.  That finished up today, and now everything but the changes to nsJSEnvironment.{cpp,h} are in.  Those changes, I've determined via tinderbox cycles, are what caused the widespread n810 orange.  These two diffs somehow cause all n810s to turn orange:

http://hg.mozilla.org/tracemonkey/diff/79fd90e2dd87/dom/base/nsJSEnvironment.cpp
http://hg.mozilla.org/tracemonkey/diff/79fd90e2dd87/dom/base/nsJSEnvironment.h

So, something in there is causing the failure.  I'm guessing it's the LazilyConstructed use/change that's causing problems just on suspicion of complexity (and the other being relatively straightforward).  If so, I'm not sure at the moment what can or should be done, given that I still don't know exactly what's causing problems or how exactly things are going awry.  But that's a task for tomorrow -- late enough tonight already...

Comment 27

[Jeff Walden [:Waldo]]

Turns out the problem was alignment issues with js::LazilyConstructed -- with those fixed everything's good, and JSTempValueRooter is no more in TM.  Whee!  Fully fixed without issue in TM starting with this revision:

http://hg.mozilla.org/tracemonkey/rev/993c318b58ef

Comment 28

[Robert Sayre]

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

Comment 29

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pretty sure this caused bug 558675.