605317 - TM: Special case 'function(a){return b[a]}' in String.replace (and friends?)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

The meat of the deminifiers in string-unpack-code are calls to String.replace(...,k[c]), where k[c] is 'function(e){return d[e]}' (d is a hashmap-style object).  For an HTML benchmark which deminifies mochikit 20 times:

Opera: 86
FF4 (nightly): 122
Safari: 125
Chrome: 112

If I modify the function at k[c] to also bump a global counter, I get these times:

Opera: 210
FF4 (nightly): 123
Safari: 128
Chrome: 116

Similar perturbations that affect the function passed to replace (e.g. add a 'var a = 0' to the function) have a similar effect; Opera seems to have a fragile special case for calls to replace on functions returning an element.  To do this ourselves (which seems a good idea, deminification is something web browsers should do fast), replace could inline the behavior of such functions and avoid making a call except when:

(a) the function is accessing a hole in the object and a proto has a scripted getter.
(b) the function is accessing a value in the object which has a scripted toString.

Comment 1

[David Mandelin [:dmandelin]]

string-unpack-code has been holding us back for a long time--this sounds like a great fix.

Comment 2

[Brian Hackett [Laid off!]]

I'll work on this.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch

This saves me 9ms on string-unpack-code (31ms -> 22ms).  In the shell, the test case used in comment 0 goes from 128ms to 87ms.

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 484229 [details] [diff] [review]
patch

First of all, sorry for the slow turnaround.

I think this is a cool hack/optimization!  Initially, I was concerned about restricting it to JSOP_GETFCSLOT, but on further consideration (and testing what the compiler considers flat), this seems like it would cover many of the cases that otherwise match.

Although I don't see anything wrong, I think the correctness of the patch depends on knowing exactly what the various property lookup/access functions do, which I'm really not qualified to judge.  Perhaps you get could jorendorff or waldo to review it?

My only nit is that it seems like 'if' starting with 'match.isObject()' could be stuck inside the then-branch of the preceding 'if' as well as the decl of 'match'.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 484229 [details] [diff] [review]
patch

That is one goofy hack you've got there. Looks correct to me, with a nit or
two.

In jsstr.cpp, FindReplaceLength:
>+        JSObject *holder;
>+        JSProperty *prop = NULL;
>+        if (!js_LookupProperty(cx, base, id, &holder, &prop))
>+            return false;

Here, call js_LookupPropertyWithFlags instead, passing JSRESOLVE_QUALIFIED as
the flags.

(Otherwise we could get the wrong flags, due to some really, really disgusting
stuff we do in resolve hooks. We probably have *plenty* of bugs in that area
and one more wouldn't have hurt us much, but since we're here, might as well
fix it.)

In js::str_replace:
>+                Value match = UndefinedValue();

Heh, match is a crummy name for this variable. Choices: map, lookupTable,
table. I vote for calling it d, in honor of the code you're optimizing for.

>+                    if (clasp->isNative() &&
>+                        clasp->getProperty == PropertyStub &&
>+                        !clasp->ops.lookupProperty &&
>+                        !clasp->ops.getProperty) {

You don't need the second condition (clasp->getProperty == PropertyStub) as
that only comes into play when the property isn't found.

>-        rdata.lambda = NULL;
>+        rdata.lambda = rdata.elembase = NULL;

Micro-nit: Two separate lines, please.

In js_NewDependentString:
>     if (length == 1 && *chars < UNIT_STRING_LIMIT)
>         return const_cast<JSString *>(&JSString::unitStringTable[*chars]);
> 
>+    if (length == 2) {
>+        if (JSString::fitsInSmallChar(chars[0]) &&
>+            JSString::fitsInSmallChar(chars[1])) {
>+            return const_cast<JSString *>(JSString::length2String(chars[0], chars[1]));
>+        }
>+    }

Hmm. Is there any reason not to check for intStrings here too? Why not common
this up with the code in js_AtomizeString? Does the return type of
length2String have to be const, or can we put the const_cast in there? Should
we use the same code in other places where we're about to allocate a string?

If the answer is just "I am trying to optimize for a specific benchmark", OK.

In the test:
>+Object.prototype.__defineGetter__("b", function() { return "what"; });

Use the standard, since you can:

Object.defineProperty(Object.prototype, "b", {get: function() { return "what"; }});

Comment 6

[Brian Hackett [Laid off!]]

Attachment: commoning patch

This commons up the logic in AtomizeString and NewDependentString to use a JSString::lookupStaticString helper.  I don't know whether other string allocation points should use this helper; I think it's best to wait until we see hot paths which would benefit from the static strings (as I did with the first version of this patch).

Comment 7

[Brian Hackett [Laid off!]]

http://hg.mozilla.org/tracemonkey/rev/0ba7d07b1660

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/0ba7d07b1660