Closed Bug 419743 (jsop_concatn) Opened 12 years ago Closed 10 years ago

JSOP_CONCATN for improved chained-concat performance

Categories

(Core :: JavaScript Engine, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: shaver, Assigned: luke)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(2 files, 11 obsolete files)

1.36 KB, text/plain
Details
23.99 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
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.
Attached patch JSOP_CONCATN as described (obsolete) — Splinter Review
Helps a bit on some stringy stuff, though mostly lost against TLB-churn noise, unforch.  Need to test this beyond sunspider, too.
Assignee: general → shaver
Status: NEW → ASSIGNED
Passes test suite, building for mochi now.
Attachment #305898 - Attachment is obsolete: true
Have a benchmark showing the pattern I'm looking to win on here, too.
Attachment #305905 - Attachment is obsolete: true
Attachment #305917 - Flags: review?(brendan)
Attachment #305917 - Flags: approval1.9?
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.
Attached patch ...and update XDR version (obsolete) — Splinter Review
Got bit here myself, when backing out the patch locally to test perf.
Attachment #305917 - Attachment is obsolete: true
Attachment #305919 - Flags: review?(brendan)
Attachment #305919 - Flags: approval1.9?
Attachment #305917 - Flags: review?(brendan)
Attachment #305917 - Flags: approval1.9?
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.
Attached patch what I'm landing (obsolete) — Splinter Review
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
Attachment #305919 - Attachment is obsolete: true
Attachment #305939 - Flags: review+
Attachment #305939 - Flags: approval1.9+
Attachment #305919 - Flags: review?(brendan)
Attachment #305919 - Flags: approval1.9?
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: blocking1.9+
Priority: -- → P1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta4
+                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.
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
No, sorry, you're right.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #305939 - Attachment is obsolete: true
Attachment #305949 - Flags: review?(shaver)
Confirmed that backout fixed orange (at least on SeaMonkey tree, which has the fastest windows tinderbox right now, it seems).
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.
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.
Target Milestone: mozilla1.9beta4 → mozilla1.9
Flags: tracking1.9+ → blocking1.9+
Priority: P1 → P2
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!
Attachment #305949 - Flags: review?(shaver)
Shaver should we punt this to .next?
Yeah.
Flags: blocking1.9+ → wanted-next+
Attachment #305939 - Flags: approval1.9+
Adding this to the 1.9.1 triage queue by marking this wanted1.9.1?.  
Flags: wanted1.9.1?
Priority: P2 → P3
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...
Flags: wanted1.9.1? → wanted1.9.1+
Any progress on this?
Target Milestone: mozilla1.9 → ---
Flags: blocking1.9.2?
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Assignee: shaver → lw
Attached patch updated patch, still missing JIT (obsolete) — Splinter Review
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.
Attachment #305949 - Attachment is obsolete: true
Attached patch found/fixed bug (obsolete) — Splinter Review
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...
Attachment #391185 - Attachment is obsolete: true
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #391254 - Attachment is obsolete: true
Attached patch v.2 (obsolete) — Splinter Review
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.
Attachment #392026 - Attachment is obsolete: true
Attachment #392351 - Flags: review?(jwalden+bmo)
Attached patch v.3, corrections (obsolete) — Splinter Review
Rebased and fixed the two bugs Waldo found.
Attachment #392351 - Attachment is obsolete: true
Attachment #395203 - Flags: review?(jwalden+bmo)
Attachment #392351 - Flags: review?(jwalden+bmo)
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 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...
Attachment #395203 - Flags: review?(jwalden+bmo) → review-
(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
$ 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
(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...
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
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.
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
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.
PRUint32 is a red herring. The issue is uint32 vs. uint32_t. The _t is of negative value.

/be
(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.
Attached patch v.4, spruced upSplinter Review
Attachment #395203 - Attachment is obsolete: true
Attachment #397072 - Flags: review?(jwalden+bmo)
(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?
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
Attachment #397072 - Flags: review?(jwalden+bmo) → review+
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.
http://hg.mozilla.org/mozilla-central/rev/ff66f63b2dfe
Status: REOPENED → RESOLVED
Closed: 12 years ago10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.