Closed
Bug 564581
Opened 13 years ago
Closed 13 years ago
TM: avoid js_IdIsIndex() calls on trace
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
10.48 KB,
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•13 years ago
|
||
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?
Attachment #444604 -
Flags: feedback?(brendan)
![]() |
Assignee | |
Comment 2•13 years ago
|
||
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.
Attachment #444604 -
Attachment is obsolete: true
Attachment #444786 -
Flags: review?(brendan)
Attachment #444604 -
Flags: feedback?(brendan)
Comment 3•13 years ago
|
||
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
Attachment #444786 -
Flags: review?(brendan) → review+
![]() |
Assignee | |
Comment 4•13 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/48082710c12f
Whiteboard: fixed-in-tracemonkey
Comment 5•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/48082710c12f
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•