454530 - TM: misc trace abort fixes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: add some misc abort avoidance

Fixes cmp with JSOP_NULL; also adds js_str_concat with 1,2,3 strings.

Comment 1

[Brendan Eich [:brendan]]

Comment on attachment 337826 [details] [diff] [review]
add some misc abort avoidance

> BUILTIN3(String_p_concat_1int,  LO, LO, LO, P,  JSString*, JSContest*, JSString*, jsint, 1, 1)

JSContest typo noted on IRC.

>+JSString* FASTCALL
>+js_String_p_concat_1str(JSContext* cx, JSString* str, JSString* a)
>+{
>+    str = js_ConcatStrings(cx, str, a);
>+    return str;

Nit: just return js_ConcatStrings(...) here.

>+JSString* FASTCALL
>+js_String_p_concat_3str(JSContext* cx, JSString* str, JSString* a, JSString* b, JSString* c)
>+{
>+    str = js_ConcatStrings(cx, str, a);
>+    if (str)
>+        str = js_ConcatStrings(cx, str, b);
>+    if (str)
>+        str = js_ConcatStrings(cx, str, c);

If str is null after the first js_ConcatStrings call, no point in the third attempt, so push the final if(str)str=js_ConcatStrings(cx,str,c) over and brace the first if's then clause.

>@@ -3118,6 +3118,10 @@
>             cond = (l == r) ^ negate;
>             break;
>         }
>+    } else if (l == JSVAL_NULL) {
>+        x = lir->ins_eq0(get(&r));
>+    } else if (r == JSVAL_NULL) {
>+        x = lir->ins_eq0(get(&l));

This isn't right, see bug 454529 comment 1 -- null is object type so type stability guarantee does not ensure null (on trace, we could see a non-null object ref, so you have to guard on null).

Can you leave the null == "string" stuff for bug 454529? This bug is all about concat. Thanks,

/be

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated

Updated patch.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 338035 [details] [diff] [review]
updated

Nice catch on the F_ConcatStrings already being there -- that kind of builtin reuse has happened before (F_NumberToString, F_FastNewObject).

>+js_String_p_concat_2str(JSContext* cx, JSString* str, JSString* a, JSString* b)
>+{
>+    str = js_ConcatStrings(cx, str, a);
>+    if (str)
>+        str = js_ConcatStrings(cx, str, b);
>+    return str;

Good forward-predicted and pre-fetched code order -- no PGO required.

>+}
>+
>+JSString* FASTCALL
>+js_String_p_concat_3str(JSContext* cx, JSString* str, JSString* a, JSString* b, JSString* c)
>+{
>+    str = js_ConcatStrings(cx, str, a);
>+    if (!str)
>+        return NULL;
>+
>+    str = js_ConcatStrings(cx, str, b);
>+    if (!str)
>+        return NULL;
>+
>+    return js_ConcatStrings(cx, str, c);

A bit of PGO help needed here, since failure is rare to non-existent. It's not traditionaly JS style (we historically prefer to over-indent the error or abnormal cases, as you do here) but it would match the concat_2str case and look leaner-and-meaner to my icache-line-prefetching eyes. Your call.

r=me, thanks a bunch. Onward!

/be

Comment 4

[Andreas Gal :gal]

Is this ready to go into the tree?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yeah, I have an updated patch here.. I'll check in later today after my talk.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

19110[tip]   2637d4852218   2008-09-15 17:30 -0400   vladimir
  b=454530; misc trace abort fixes (trace String.concat); r=brendan