564581 - TM: avoid js_IdIsIndex() calls on trace

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

This is a follow-up from bug 564522.

dmandelin said:  js_IsIdIndex takes a lot of the time (1/3 of the time inside
js_AddProperty in my measurements). It can be specialized away at trace/PIC
compile time by checking the id. There are other opportunities for using
special cases here.

brendan said: we really should not call js_IdIsIndex from (code
jitted for) ops that access interned property names (atoms) -- that's just
silly (my bad, layering and generality == slowness). Those ids must be
lexically valid Identifiers (see the ECMA-262 lexical grammar), so cannot be
indexes.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: hacky patch

I barely understand the whole properties/scopes side of SM/TM, but I was able 
to work out the following from comment 0 and with some help from luke and
dvander on IRC:

- The relevant call-chain in access-binary-trees.js is this:

    js_IsIdIndex()
    JSScope::updateFlags()
    JSScope::extend()
    js_AddProperty()

- Certain operations, such as JSOP_SETPROP, access interned property names 
which cannot be indexes.

- So we can specialise the above call-chain for JSOP_SETPROP.

The attached patch does this in a super-dumb and super-obvious way:  it
creates js_AddProperty2(), extend2(), updateFlags2(), js_IsIdIndex2(), and 
when generating code in jstracer.cpp it calls this alternative call chain
for SETPROP.  The original call-chain is used for SETNAME and SETMETHOD.

Cachegrind tells me this successfully gets rid of all the unnecessary 
js_IdIsIndex() calls in access-binary-trees -- the instruction count drops from
82.0M to 79.4M.

SS timings show a 1.02--1.04x speedup for access-binary-trees.  This is only 
about a 0.6--1ms improvement, but it's quite repeatable on 32-bit and 64-bit
which is encouraging.

So: what now?  The patch is pretty ugly.  Would passing a bool through the 
call-chain be better?  Can the call-chain be specialized in other ways?  Have I
completely misunderstood the whole thing?

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: better patch

js_AddPropertyHelper(), and makes new functions js_AddProperty() and 
js_AddPropertyAtom() which just call js_AddPropertyHelper() with a bool set
to true/false.  This bool is then propagated through to updateFlags();  
default args are used to minimize disruption.

Interestingly enough, this version has better perf that the previous patch.
Updated instruction counts:

- tip:       81.8M
- old patch: 79.4M
- new patch: 77.7M
  
Looking at the Cachegrind output it appears that updateShape() is now being
inlined more in the new patch which accounts for the difference.  Seems odd
but I'm not complaining.  

SS timings show access-binary-trees is 2--6% faster than tip, which is up 
1.3ms better.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 444786 [details] [diff] [review]
better patch

>+js_AddPropertyHelper(JSContext* cx, JSObject* obj, JSScopeProperty* sprop, bool isDefinitelyAtom)

Just noting we have [aA]ddPropertyHelpers in jsscope.{cpp,h} and jsobj.cpp, so could use another name. Or not! C++ can cope. In that light, we don't need the js_ prefix here (static inline in jsbuiltins.cpp).

> TraceRecorder::record_SetPropHit(PropertyCacheEntry* entry, JSScopeProperty* sprop)
> {
>     jsval& r = stackval(-1);
>     jsval& l = stackval(-2);
>     LIns* v_ins;
>-    CHECK_STATUS_A(setProp(l, entry, sprop, r, v_ins));
> 
>     jsbytecode* pc = cx->fp->regs->pc;
>+
>+    bool isDefinitelyAtom = *pc == JSOP_SETPROP;

Nit: == against = is one place prevailing style over-parenthesizes (other cases involve bitwise ops -- any of 'em -- just cuz C operator precedence is broken for all bitwise, in several ways).

Thanks, good patch!

/be

Comment 4

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/48082710c12f

Comment 5

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/48082710c12f