501230 - TM: fast path for String.toString

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Comment 1

[Andreas Gal :gal]

Attachment: patch

This does GetMethod twice potentially, but only if the object is of StringClass, which should be safe.

Comment 2

[Andreas Gal :gal]

This path is hit heavily in dromaeo JS tests >> string tests >> charAt test.

Comment 3

[Andreas Gal :gal]

100x that Dromeo test

without the patch: 643
with the patch: 414

(smaller is better)

Comment 4

[Andreas Gal :gal]

The 643 is with Bug 501254 applied. Without that we get 1488, which is slower than interpretation (1202).

Comment 5

[Andreas Gal :gal]

"100x that dromeo test" => running that test 100 times (it runs too short originally to properly measure it)

Comment 6

[Brendan Eich [:brendan]]

You can't argue about getMethod called twice being "safe" in the presence of user-defined getters. By the book, toString should be got only once.

Suggest pushing the optimization down into js_DefaultValue.

/be

Comment 7

[Andreas Gal :gal]

I thought its safe because of the js_StringClass check. Could you briefly explain why thats not the case? (I believe you its not, I just don't understand why not ;)

I will push this further down and eliminate the redundant getMethod.

Comment 8

[Brendan Eich [:brendan]]

js> save = String.prototype.toString                                
function toString() {
    [native code]
}
js> String.prototype.__defineGetter__('toString', function(){++effectCount; return save.apply(this, arguments)})
js> effectCount=1
1
js> s = new String('hi')
hi
js> s
hi
js> effectCount
2

You don't know what the getter might do. It could have arbitrary effects. The spec says toString is tried once. The example above shows this. With your patch if I'm not mistaken the effectCount would be 4 at the end.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: proposed alterna-patch

Comment 10

[Brendan Eich [:brendan]]

Attachment: proposed alterna-patch, with null proto defense

Someone could set String.prototype.__proto__ = null or s = new String("hi"); s.__proto__ = null -- that would be just mean, but it's allowed.

/be

Comment 11

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/9a626a99c67c

/be

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Nice, what's the win like?

(Wouldn't we rather just emit a tiny bit of LIR here?  Guard on shape, deref a couple of pointers, Bob's your screaming CPU pipeline?)

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> Nice, what's the win like?
> 
> (Wouldn't we rather just emit a tiny bit of LIR here?  Guard on shape, deref a
> couple of pointers, Bob's your screaming CPU pipeline?)

There are lotsa ways to reach OBJ_DEFAULT_VALUE that won't go through an imacro being recorded for JITting. String objects are rare birds outside of Dromaeo, so any JIT-only optimizations should go in a later bug, if we feel the need.

Why is Dromaeo creating String wrappers? Dunno. Anyone?

/be

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #12)
> Nice, what's the win like?

Andreas said strictly better than the win measured in comment 3 but I didn't hear actual numbers.

/be

Comment 15

[Brendan Eich [:brendan]]

John, see comment 13 para 2. Thanks,

/be

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/9a626a99c67c