TM: misc trace abort fixes

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: vlad, Assigned: vlad)

Tracking

Trunk
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

Fixes cmp with JSOP_NULL; also adds js_str_concat with 1,2,3 strings.
Attachment #337826 - Flags: review?(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
Summary: misc trace abort fixes → TM: misc trace abort fixes
Created attachment 338035 [details] [diff] [review]
updated

Updated patch.
Assignee: general → vladimir
Attachment #337826 - Attachment is obsolete: true
Attachment #338035 - Flags: superreview?(brendan)
Attachment #337826 - Flags: review?(brendan)

Updated

10 years ago
Attachment #338035 - Flags: superreview?(brendan) → review+
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

10 years ago
Is this ready to go into the tree?
Yeah, I have an updated patch here.. I'll check in later today after my talk.
19110[tip]   2637d4852218   2008-09-15 17:30 -0400   vladimir
  b=454530; misc trace abort fixes (trace String.concat); r=brendan
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.