419743 - (jsop_concatn) JSOP_CONCATN for improved chained-concat performance

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Mike Shaver (:shaver emeritus)]

A pretty common pattern on the web is to string together additions, many of which such chains are known to result in stringification for a (possibly total) suffix due to the presence of a string literal.

I propose that we gather such concatenation sequences and emit them as single JSOP_CONCATN ops, so that we can reduce the number of allocations we require, and the amount of garbage produced for temporaries.

Comment 1

[Mike Shaver (:shaver emeritus)]

Attachment: JSOP_CONCATN as described

Helps a bit on some stringy stuff, though mostly lost against TLB-churn noise, unforch.  Need to test this beyond sunspider, too.

Comment 2

[Mike Shaver (:shaver emeritus)]

Attachment: 2: teach js_DVG about JSOP_CONCATN

Passes test suite, building for mochi now.

Comment 3

[Mike Shaver (:shaver emeritus)]

Attachment: With some cosmetic fixes due to Waldo.

Have a benchmark showing the pattern I'm looking to win on here, too.

Comment 4

[Mike Shaver (:shaver emeritus)]

Attachment: buildTable.js, to measure space and speed for string construction

Trunk:

ca-222:~/src/js-src shaver$ Darwin_OPT.OBJ/js -f ../js-concatn/buildTable.js
1271
before 28413952, after 24576, break 09100000

With patch:

carnifex-4:~/src/js-concatn shaver$ Darwin_OPT.OBJ/js -f buildTable.js
774
before 14307328, after 24576, break 07b00000

Decent speed and space wins for what I _think_ is a pretty common pattern.

Comment 5

[Mike Shaver (:shaver emeritus)]

Attachment: ...and update XDR version

Got bit here myself, when backing out the patch locally to test perf.

Comment 6

[Mike Shaver (:shaver emeritus)]

I may not be awake so much when this gets all the bits it needs to commit, so people should feel free (indeed, beseeched!) to do so in my stead if I'm not around.

Comment 7

[Brendan Eich [:brendan]]

Attachment: what I'm landing

Nits (trailing whitespace, unsigned u > 0 => u != 0), JSXDR_BYTECODE_VERSION fix, and len overflow checking in the interpreter logic. Thanks for this great patch, shaver!

/be

Comment 8

[Brendan Eich [:brendan]]

Fixed:

js/src/jsemit.c 3.307
js/src/jsinterp.c 3.445
js/src/jsopcode.c 3.294
js/src/jsopcode.tbl 3.110
js/src/jsxdrapi.h 1.49

/be

Comment 9

[Brian Crowder]

+                if ((size_t)len > JSSTRING_LENGTH_MASK) {
+                    JS_ReportOutOfMemory(cx);
+                    goto error;
+                }
+            }
+
+            buf = JS_malloc(cx, (len + 1) * sizeof(*buf));

The OOM-check math doesn't match the malloc, here.

Comment 10

[Brendan Eich [:brendan]]

The OOM-check is correct and strictly more conservative than a size_t wraparound check. See js_NewString's identical check. Is there a problem?

/be

Comment 11

[Brian Crowder]

No, sorry, you're right.

Comment 12

[Brendan Eich [:brendan]]

Backed out to cure Windows orangeness afflicting all products:

js/src/jsemit.c 3.308
js/src/jsinterp.c 3.446
js/src/jsopcode.c 3.295
js/src/jsopcode.tbl 3.111
js/src/jsxdrapi.h 1.50

Shaver, you have a Windows build? Maybe vlad can help.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: picked a few nits, basis for tomorrow's landing attempt ;-)

Here's the interdiff:

diff -u jsinterp.c jsinterp.c
--- jsinterp.c  27 Feb 2008 05:43:55 -0000
+++ jsinterp.c  27 Feb 2008 07:02:38 -0000
@@ -6581,11 +6581,11 @@
             argc = GET_ARGC(pc);
             len = 0;
             for (vp = sp - argc; vp < sp; vp++) {
-                lval = *vp;
-                if (JSVAL_IS_STRING(lval)) {
-                    len += JSSTRING_LENGTH(JSVAL_TO_STRING(lval));
+                rval = *vp;
+                if (JSVAL_IS_STRING(rval)) {
+                    len += JSSTRING_LENGTH(JSVAL_TO_STRING(rval));
                 } else {
-                    str = js_ValueToString(cx, lval);
+                    str = js_ValueToString(cx, rval);
                     if (!str)
                         goto error;
                     *vp = STRING_TO_JSVAL(str);
@@ -6608,9 +6608,9 @@
 
             len = 0;
             for (vp = sp - argc; vp < sp; vp++) {
-                JS_ASSERT(JSVAL_IS_STRING(*vp));
-                lval = *vp;
-                str = JSVAL_TO_STRING(lval);
+                rval = *vp;
+                JS_ASSERT(JSVAL_IS_STRING(rval));
+                str = JSVAL_TO_STRING(rval);
                 js_strncpy(buf + len, JSSTRING_CHARS(str),
                            JSSTRING_LENGTH(str));
                 len += JSSTRING_LENGTH(str);

Those stacked values are r-values in C jargon, not l-values, and may as well use the value loaded from *vp in the assertion.

r? shaver just cuz.

/be

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Confirmed that backout fixed orange (at least on SeaMonkey tree, which has the fastest windows tinderbox right now, it seems).

Comment 15

[Mike Shaver (:shaver emeritus)]

Thanks, guys.  Don't suppose anyone has a stack for those oranges?  I don't have a Windows development env, might try to impose on Vlad tomorrow.

Comment 16

[Mike Shaver (:shaver emeritus)]

We're tripping this assertion in js_Invoke:

    JS_ASSERT(vp + 2 + argc <= (jsval *) cx->stackPool.current->avail);

on this line in FeedConverter.js's registerSelf:

  registerSelf: function M_registerSelf(cm, file, location, type) {
    var cr = cm.QueryInterface(Ci.nsIComponentRegistrar);
    
    cr.registerFactoryLocation(FS_CLASSID, FS_CLASSNAME, FS_CONTRACTID,
                               file, location, type);
    cr.registerFactoryLocation(FPH_CLASSID, FPH_CLASSNAME, FPH_CONTRACTID,
                               file, location, type);
    cr.registerFactoryLocation(PCPH_CLASSID, PCPH_CLASSNAME, PCPH_CONTRACTID,
                               file, location, type);

    // The feed converter is always attached, since parsing must be done to 
    // determine whether or not auto-handling can occur. 
    const converterPrefix = "@mozilla.org/streamconv;1?from=";
    var converterContractID = 
        converterPrefix + TYPE_MAYBE_FEED + "&to=" + TYPE_ANY;
    cr.registerFactoryLocation(FC_CLASSID, FC_CLASSNAME, converterContractID,
                               file, location, type); // ***HERE***

    converterContractID = 
        converterPrefix + TYPE_MAYBE_VIDEO_FEED + "&to=" + TYPE_ANY;
    cr.registerFactoryLocation(FC_CLASSID, FC_CLASSNAME, converterContractID,
                               file, location, type);

    converterContractID = 
        converterPrefix + TYPE_MAYBE_AUDIO_FEED + "&to=" + TYPE_ANY;
    cr.registerFactoryLocation(FC_CLASSID, FC_CLASSNAME, converterContractID,
                               file, location, type);
    },

I couldn't get it to repro in the shell with variants of this, but I'll come back to it after I spend a bit of time digging into bug bug 419878.

Comment 17

[Mike Shaver (:shaver emeritus)]

Comment on attachment 305949 [details] [diff] [review]
picked a few nits, basis for tomorrow's landing attempt ;-)

I like the nit-picking; need to figure out the stack depth thing still!

Comment 18

[Mike Schroepfer]

Shaver should we punt this to .next?

Comment 19

[Mike Shaver (:shaver emeritus)]

Yeah.

Comment 20

[Damon Sicore (:damons)]

Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.  

Comment 21

[Mike Shaver (:shaver emeritus)]

We saw the assertion from comment 16, again on Windows only, when landing 260106.  That also adds an opcode JSOP_NEWARRAY that pops a variable number of items from the stack and then produces one.  Other ops also consume N and produce 1, but don't seem to trigger this behaviour (CALL, SETCALL, EVAL, NEW; POPN produces 0).

I'm still digging...

Comment 22

[Alfred Kayser]

Any progress on this?

Comment 23

[Luke Wagner [:luke]]

Attachment: updated patch, still missing JIT

Rebased the patch and ran it on the try server and all green (yipee!).  Still need to trace JSOP_CONCATN.

I also swapped in the brand-spankin' new JSTempVector into the JSOP_CONCATN implementation in jsinterp.cpp.  In addition to getting shorter, this is faster when it avoids temporary JSStrings created when a conversion is necessary.  For example, in the attached benchmark, if you add two more uses of 'results[t].score' to the CONCATN in the loop body, the timing is:

trunk: 1203
original patch: 784
new patch: 581

Incidentally, because of the two intermediate function calls added by my changes, when there are no conversions, the JSTempVector version is a few ms slower.  If I manually inline the JSVAL_IS_STRING case of js_ValueToCharBuffer and inline js_NewStringFromCharBuffer, the slowdown goes away.  I suspect this isn't worth the redundancy, so I left it as is.

Comment 24

[Luke Wagner [:luke]]

Attachment: found/fixed bug

I think I found a bug in the interpreted CONCATN.  The impl needs to perform the ToPrimitive conversion (step 5,6 in 11.6.1) before the ToString conversion (step 12,13).  Otherwise,

o = { valueOf:function() { return 1 } };
print("a" + o + o);

prints "a11" without CONCATN and "a[object Object][object Object]" with.

Still working towards JIT...

Comment 25

[Luke Wagner [:luke]]

Attachment: WIP

Progress.  I still need to test the heck out of it, but this patch includes JITing.  Quite some trickery is required to call N imacros to convert the arguments to builtins without manually hard-coded unrolling.

Comment 26

[Luke Wagner [:luke]]

Attachment: v.2

End result, for attached buildTable.js:

trunk:       760ms
trunk (jit): 678ms
patch:       491ms
patch (jit): 392ms

To wit, the JITed code does not use JSTempVector for building the string in place (no clean/efficient way to do it), so if you tweak buildTable.js by adding a few more (here, 4) uses of non-string fields that need to be converted using temporaries, the interpreted code actually beats the JIT:

trunk:       2008ms
trunk (jit): 1724ms
patch:       703ms
patch (jit): 853ms

This is still a big win over trunk, so probably not worth taking any further.

Comment 27

[Luke Wagner [:luke]]

Attachment: v.3, corrections

Rebased and fixed the two bugs Waldo found.

Comment 28

[Luke Wagner [:luke]]

Note on corrections in comment 27: because of global scope weirdness, I had to disable JSOP_CONCATN when emitting in the global scope.  After a short talk with Brendan on why this is so, I filed bug 511256.  But for now, don't expect any speedup if your benchmark is in the global scope.

Also, ready for review Waldo.

Comment 29

[Jeff Walden [:Waldo]]

Comment on attachment 395203 [details] [diff] [review]
v.3, corrections

int32_t, uint32_t, etc. throughout


>diff --git a/js/src/imacros.jsasm b/js/src/imacros.jsasm

>+    .imacro string                                  # [xN] obj N
>+        swap                                        # [xN] N obj
>+        dup                                         # [xN] N obj obj
>+        dup                                         # [xN] N obj obj obj
>+        getprop valueOf                             # [xN] N obj obj valueOf
>+        ifprimtop 1                                 # [xN] N obj obj valueOf
>+        swap                                        # [xN] N obj valueOf obj
>+        string void                                 # [xN] N obj valueOf obj "void"
>+        call 1                                      # [xN] N obj val
>+        ifprimtop 3                                 # [xN] N obj val
>+        pop                                         # [xN] N obj
>+        dup                                         # [xN] N obj obj
>+        goto 2
>+1:      pop                                         # [xN] N obj obj
>+2:      callprop toString                           # [xN] N obj toString obj
>+        call 0                                      # [xN] N obj val
>+        primtop (JSTYPE_VOID)                       # [xN] N obj val
>+3:      swap                                        # [xN] N val obj
>+        pop                                         # [xN] N val
>+        swap                                        # [xN] val N
>+        imacop                                      # catstr
>+        stop
>+    .end

This seems a little clearer to me:

  # [arg1..argN] argI I


>diff --git a/js/src/jsbuiltins.cpp b/js/src/jsbuiltins.cpp

>+JSString* FASTCALL
>+js_ConcatN(JSContext *cx, JSString *pretend, int32 size)
>+{
>+    /* There is no JSString** type, so use a cast. */
>+    JSString **strArray = reinterpret_cast<JSString **>(pretend);

MY EYES!!!

If you're willing to spend some time grokking jsbuiltins.h, I'd much prefer if this were a void* argument; at least there it's more obviously a static_cast for reasons of type-incompetence than casting between entirely unrelated types.  (Then again, if you're willing to go that far it's probably not much effort to add a JSString** type.)


>+    /* Calculate total size. */
>+    size_t numChar = 1;
>+    for (int32 i = 0; i < size; ++i) {
>+        size_t before = numChar;
>+        numChar += strArray[i]->length();
>+        if (numChar < before)
>+            return NULL;
>+    }

If |size| can't be size_t (another epic add to jsbuiltins.h?), assert that it's non-negative (positive?).


>+    JSString *str = js_NewString(cx, buf, numChar - 1);
>+    if (!str) {
>+        cx->free(buf);
>+        return NULL;
>+    }
>+
>+    return str;
>+}

|return NULL| above is superfluous; just let control flow to |return str|.


>diff --git a/js/src/jsopcode.cpp b/js/src/jsopcode.cpp

>+              case JSOP_CONCATN:
>+                argc = GET_UINT16(pc);
>+                argv = (char **)JS_malloc(cx, (size_t)argc * sizeof *argv);

C++-style cast here on argc


>+                ok = JS_TRUE;
>+                for (i = argc - 1; i >= 0; i--) {
>+                    argv[i] = JS_strdup(cx, POP_STR());
>+                    if (!argv[i]) {
>+                        ok = JS_FALSE;
>+                        break;
>+                    }
>+                }
>+
>+                todo = Sprint(&ss->sprinter, "%s", argv[0]);
>+                if (todo < 0)
>+                    ok = JS_FALSE;
>+                for (i = 1; i < argc; i++) {
>+                    if (Sprint(&ss->sprinter, " + %s", argv[i]) < 0) {
>+                        ok = JS_FALSE;
>+                        break;
>+                    }
>+                }

Suppose OOM on JS_strdup; argv[0...i - 1] are arbitrary values, then passed to Sprint (and worse, later to JS_free).  Seems like you need an |if (ok) { }| surrounding todo =, if, for statements.

Convention now may be to use js_free and js_malloc here; I don't remember whether that patch has landed or exactly what the JS-wrapper methods were named, off the top of my head.


>diff --git a/js/src/jsopcode.tbl b/js/src/jsopcode.tbl

>+/*
>+ * Concatenate multiple values, coercing to string if necessary.
>+ */
>+OPDEF(JSOP_CONCATN,       234,"concatn",       NULL,  3, -1,  1,  0,  JOF_UINT16|JOF_TMPSLOT2)

This could use more description of the stack layout, maybe like so:

Concatenate multiple values, coercing to string if necessary.  Stack setup is, bottom to top, [arg1, arg2, ..., argN] initially; when an imacro is being recorded the setup is [primarg1, ..., primarg_{i-1}, argi, ..., argN, argi, i] for i=1 to N, as args are converted to primitives prior to final consumption of all primargs to get a final concatenated string.

Other suggestions welcome; exactly how this works is...complicated at best.


>diff --git a/js/src/jsops.cpp b/js/src/jsops.cpp

>+          BEGIN_CASE(JSOP_CONCATN)
>+          {
>+            JS_ASSERT_IF(fp->imacpc, *fp->imacpc == JSOP_CONCATN &&
>+                                     *regs.pc == JSOP_IMACOP);

Readability would be enhanced if the second argument to the assertion were on its own line, not half on the condition-line and half not.

>+            JS_ASSERT_IF(fp->imacpc,
>+                         *fp->imacpc == JSOP_CONCATN && *regs.pc == JSOP_IMACOP);


>+            bool imacro = !!fp->imacpc;
>+            bool recording = !!TRACE_RECORDER(cx);

This suggests |bool JSContext::recording()|, as a followup method.

I would prefer |fp->imacpc != NULL| to avoid the double-negative of !!; my mind parses the former faster than the latter.


>+            if (imacro) {
>+                /* END_CASE does pc += CONCATN_LENGTH. (IMACOP YOU IDIOT!) */
>+                regs.pc -= JSOP_CONCATN_LENGTH - JSOP_IMACOP_LENGTH;
>+            }

Hm; this is distinctly unsatisfying.  I wish I had a better idea that didn't require every tail-calling imacro-op to include a hack like this.


>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp

>+        /* Dereference is safe since imacros are JSOP_STOP-terminated. */
>+        if (*(regs->pc + js_CodeSpec[*regs->pc].length) != JSOP_STOP)

More conventional style would be regs->pc[js_CodeSpec[*regs->pc].length].


>+#define MAX_CONCATN_SIZE 32

static const uint32_t


>+jsval *
>+js_ConcatPostImacroStackCleanup(uint32 argc, JSFrameRegs &regs,
>+                                TraceRecorder *recorder)
>+{
>+    JS_ASSERT(*regs.pc == JSOP_IMACOP);
>+
>+    /* Pop the argument offset and imacro return value. */
>+    JS_ASSERT(JSVAL_IS_INT(*(regs.sp - 1)));
>+    jsint offset = JSVAL_TO_INT(*--regs.sp);

JSVAL_TO_INT includes this assertion already these days, probably since you posted this patch.


>+    jsval *imacroResult = --regs.sp;
>+
>+    /* Replace non-primitive argument with new primitive argument. */
>+    jsval *vp = regs.sp - offset;
>+    JS_ASSERT((regs.sp - argc) <= vp && vp < regs.sp);

Don't overparenthesize here.


>+ * like to support largeish N), daisy chain the imacros using 'imacop'.

Nit: daisy-chain with hyphen


>+    /* Set in each branch below. */
>+    uint32 argc;
>+    jsval *loopStart;
>+
>+    /*
>+     * If we are in an imacro, we must have just finished a call to
>+     * defvalue.string.  Continue where we left off last time.
>+     */

Move the argc/loopStart declarations down beneath this comment, nix the /* Set in */ comment.


>+             * In addition to the jsval we want the imacro to primitize, we

"convert to primitive", please.


>diff --git a/js/src/jstracer.h b/js/src/jstracer.h

>+    friend jsval *js_ConcatPostImacroStackCleanup(uint32, JSFrameRegs &, TraceRecorder *);

Name the arguments here, please.

r- at least for the decompilation botch, and the rest of the issues are not entirely de minimis themselves...

Comment 30

[Brendan Eich [:brendan]]

(In reply to comment #29)
> (From update of attachment 395203 [details] [diff] [review])
> int32_t, uint32_t, etc. throughout

We aren't doing this yet, or ever -- we have API commitments to the _t-less forms and the _t adds no value.

/be

Comment 31

[Brendan Eich [:brendan]]

$ grep -w 'u*int[12346]*_t' *.cpp
jsarray.cpp:    uint32_t length = (uint32_t) obj->fslots[JSSLOT_ARRAY_LENGTH];
jstracer.cpp:    uint32_t bits;
jstracer.cpp:    int32_t ci = cf > 0x7fffffff ? uint32_t(cf) : int32_t(cf);
jstracer.cpp:IsConst(LIns* i, int32_t c)
jstracer.cpp:    int32_t& hits = c->hits();
jstracer.cpp:    int32_t bs = state.builtinStatus;
jstracer.cpp:                uint32_t hwcap = aux.a_un.a_val;
jstracer.cpp:static JS_ALWAYS_INLINE int32_t
jstracer.cpp:        native_rval_ins = lir->ins2i(LIR_piadd, invokevp_ins, int32_t((vplen - 1) * sizeof(jsval)));
jstracer.cpp:        args[1] = lir->ins2i(LIR_piadd, invokevp_ins, int32_t(2 * sizeof(jsval)));
jstracer.cpp:    uint32_t slot = GET_UINT16(cx->fp->regs->pc);

All jstracer.cpp except for that first one :-/.

/be

Comment 32

[Jeff Walden [:Waldo]]

(In reply to comment #30)
> We aren't doing this yet, or ever -- we have API commitments to the _t-less
> forms and the _t adds no value.

This is all internal API, and we added jsstdint.h precisely so we could transition to more-standard code.  I thought we had this discussion a long time ago and decided we were fine making the switch...

Comment 33

[Brendan Eich [:brendan]]

No, and I think we're in a world where it's better to change all at once, or not at all -- as with T* p vs. T *p. And we've got real work to do that is much higher priority than either of these changes (or similar window-dressing ones). But I do not agree to mixing window drape styles. That is a recipe for perpetual mixing and confusion.

/be

Comment 34

[Jeff Walden [:Waldo]]

We've done a lot of other "window-dressing" work, particularly the macro->inline function changes.  We have reaped strong benefits from doing so, and we will continue to reap strong benefits from doing so.

But this is all aside from this bug; keep it as uint32 for now if that's what matters to get this fixed now.  In the long run, I believe we should and will switch.  Making up our own names for standard types has real consequences that we should avoid outside of stable APIs.

Comment 35

[Brendan Eich [:brendan]]

Inlines beat macros by being more readable and avoiding multiple evaluation with side effects hazard.

The _t tax is pure name bloat, whose standardosity (I'm a kernel kid, I grew up on pid_t, proc_t, etc.) is not compelling given the predominant use of int32, etc.

/be

Comment 36

[Jeff Walden [:Waldo]]

Predominant use doesn't mean it's more approachable for new readers, which is the main reason to make the change, and which "tradition" cannot invalidate.  Every little bit counts.  Do you really want to see PRUint32 forevermore as well?  That's preposterous.

Comment 37

[Brendan Eich [:brendan]]

PRUint32 is a red herring. The issue is uint32 vs. uint32_t. The _t is of negative value.

/be

Comment 38

[Luke Wagner [:luke]]

(In reply to comment #29)
> >+JSString* FASTCALL
> >+js_ConcatN(JSContext *cx, JSString *pretend, int32 size)
> >+{
> >+    /* There is no JSString** type, so use a cast. */
> >+    JSString **strArray = reinterpret_cast<JSString **>(pretend);
> 
> MY EYES!!!

Hah!  I either missed _JS_CTYPE_STRINGPTR or it was added recently.  No cast.

> If |size| can't be size_t (another epic add to jsbuiltins.h?), assert that it's
> non-negative (positive?).

Again, I totally missed _JS_CTYPE_UINT32.  Either way, its 16-bit unsigned value.

> Other suggestions welcome; exactly how this works is...complicated at best.

I agree about said complication, but it seems like the abstract opcode semantics should not include the imacro hackery, since this is tracer-specific.  The record_JSOP_CONCATN includes such a description, so perhaps thats the right place?

> >+            bool imacro = !!fp->imacpc;
> >+            bool recording = !!TRACE_RECORDER(cx);
> 
> This suggests |bool JSContext::recording()|, as a followup method.

Good suggestion, and perhaps this is what you mean by "followup method", but it seems like this would be a separate patch that replaces all use of TRACE_RECORDER(cx) {!|}= NULL as the "are we recording?" discriminant at once that way there was no confusion of whether the two tests were equivalent.

> Hm; this is distinctly unsatisfying.  I wish I had a better idea that didn't
> require every tail-calling imacro-op to include a hack like this.

Eventually I'd like to replace *all* imacros with the mechanism to support self-hosting in bug 460904.

Comment 39

[Luke Wagner [:luke]]

Attachment: v.4, spruced up

Comment 40

[Jeff Walden [:Waldo]]

(In reply to comment #37)
> PRUint32 is a red herring.

I'm just applying logic from one domain to another that for this purpose is extremely similar.  Why doesn't your argument apply both places?

Comment 41

[Brendan Eich [:brendan]]

Because my argument is not just mindless standing on precedent:

0. API commitments keep uint32 forever, mixing uint32_t internally with uint32 "externally" still mixes, and mixing is complexity and likely cross-contamination over time (see 2 below).

1. uint32 is better than uint32_t (e.g.) as a type name. This is not separable from my argument in full. PRUint32 sucks but it's not our problem in JS-land.

2. Independent of 0 and 1, it's better not to mix old and new in non-API code.

Yes, I know stdint is a standard. So is the local standard in our code that avoids the _t tax. Among all the barriers to new developers grokking our code, the uint32 without _t is dead last, if it's an actual problem for anyone.

I'm probably not alone in disliking the _t tax on shifted keystroke typing and reading, but if most current developers really want stdint names internally, then we should hash this out in the newsgroup.

/be

Comment 42

[Jeff Walden [:Waldo]]

Comment on attachment 397072 [details] [diff] [review]
v.4, spruced up

(In reply to comment #38)
> I agree about said complication, but it seems like the abstract opcode
> semantics should not include the imacro hackery, since this is tracer-specific.
>  The record_JSOP_CONCATN includes such a description, so perhaps thats the
> right place?

Perhaps -- but the description there doesn't really describe the stack layout, at least not to the level of detail I'd like.  Let's figure out something before pushing.


> > >+            bool imacro = !!fp->imacpc;
> > >+            bool recording = !!TRACE_RECORDER(cx);
> > 
> > This suggests |bool JSContext::recording()|, as a followup method.
> 
> Good suggestion, and perhaps this is what you mean by "followup method", but it
> seems like this would be a separate patch that replaces all use of
> TRACE_RECORDER(cx) {!|}= NULL as the "are we recording?" discriminant at once
> that way there was no confusion of whether the two tests were equivalent.

Precisely, no need for anything now/here except maybe filing that bug.


>diff --git a/js/src/jsopcode.tbl b/js/src/jsopcode.tbl

>+ * Concatenate multiple values, coercing to string if necessary.

Hmm, at least note that the recording version of this is a bit different with a pointer to the description of the tracing mechanics.

Comment 43

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/ff66f63b2dfe

Comment 44

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 45

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9a1e9c9305b7