It's time. Patch defers cleanup from bug 497789. I'll stick with js-prefixed filenames: jspropcache.{h,cpp}. We can hg rename to unprefixed and less 8.3-hacked names in a separate, consolidated effort, if hg and our tools based on it allow. /be

The first part is just to move the propcache-related code from file to file.

The second part actually changes stuff.

The changes in jsops.cpp are easier to see with -b, since I reindented a bunch of the JSOP_SETPROP case.

Comment on attachment 397639 [details] [diff] [review] encapsulate v1 The jsuword consts should be uintptr_t these days, but if that's not easy to hook up don't worry about it for now. The const ints should be const unsigned, tho; they don't make any sense as signed values. I would request JS_PROPERTY_CACHE be converted to |JSPropertyCache& JSContext::propertyCache() const { return JS_THREAD_DATA(this)->propertyCache; }| except that I know doing so now rather than in a followup would be a mistake.

hg tip: ../jspropcacheinlines.h: In member function ‘bool JSPropertyCache::testForSet(JSContext*, jsbytecode*, JSObject**, JSObject**, JSPropCacheEntry**, JSAtom**)’: ../jspropcacheinlines.h:141: error: ‘cache’ was not declared in this scope ../jspropcacheinlines.h: In member function ‘bool JSPropertyCache::testForInit(JSContext*, jsbytecode*, JSObject*, JSPropCacheEntry**, JSScopeProperty**)’: ../jspropcacheinlines.h:163: error: ‘cache’ was not declared in this scope ../jsops.cpp: In function ‘JSBool js_Interpret(JSContext*)’: ../jsops.cpp:1736: error: ‘cache’ was not declared in this scope ../jsops.cpp:1772: error: ‘cache’ was not declared in this scope ../jsops.cpp:1843: error: ‘cache’ was not declared in this scope ../jsops.cpp:3549: error: ‘cache’ was not declared in this scope {standard input}:unknown:Undefined local symbol L618 etc. /be

Part 1, just moving a few functions to a new file. http://hg.mozilla.org/tracemonkey/rev/2fbd2420ef8b Part 2, refactoring. http://hg.mozilla.org/tracemonkey/rev/3f508cfdfa36 Fix JS_PROPERTY_CACHE_METERING-only build breakage. http://hg.mozilla.org/tracemonkey/rev/8abad92fd850 Change int constants to unsigned as Waldo asked in review. http://hg.mozilla.org/tracemonkey/rev/eafee0100926

Backed out. Orange everywhere.

This patch only moves stuff from file to file.

Oops - I moved something to the wrong file.

Part 3 will be to move stuff into the JSPropertyCache class where possible, in the style of the earlier patch. Part 4 will be trivial again, just rename some stuff like: JSPropertyCache -> js::PropertyCache JSPropertyCacheEntry -> js::PropertyCache::Entry

Comment on attachment 434352 [details] [diff] [review] Part 1, moves - v2 Wafer-thin! /be

Comment on attachment 434354 [details] [diff] [review] Part 2, inline functions - v2 >+inline jsuword PCVCAP_TAG(jsuword t) { >+ return t & PCVCAP_TAGMASK; >+} >+ >+inline jsuword PCVCAP_MAKE(uint32 t, unsigned int s, unsigned int p) { >+ JS_ASSERT(t < SHAPE_OVERFLOW_BIT); >+ JS_ASSERT(s <= PCVCAP_SCOPEMASK); >+ JS_ASSERT(p <= PCVCAP_PROTOMASK); >+ return (t << PCVCAP_TAGBITS) | (s << PCVCAP_PROTOBITS) | p; >+} Nits: these promulgate a third function definition style (ignoring inline methods in classes/structs): T f(a) { ... } in addition to the winning T f(a) { ... } where everything fits on one line, and the august (nay, hoary) Q T f(a) { ... } I'd say make the shorties use the one-line style, and the others use the old definition style. >+JS_ALWAYS_INLINE void >+PROPERTY_CACHE_TEST(JSContext *cx, jsbytecode *pc, JSObject *&obj, >+ JSObject *&pobj, JSPropCacheEntry *&entry, JSAtom *&atom) >+{ >+ do { Lose the do-while(0). >+ JSPropertyCache *cache_ = &JS_PROPERTY_CACHE(cx); Can lose the trailing _ all over too. Does PROPERTY_CACHE_TEST expand and optimize as well as an inline as a macro? Maybe check generated non-optimized code for quick sanity check. If that's as good or better, no worries. r=me with above addressed. /be

(In reply to comment #12) > I'd say make the shorties use the one-line style, and the others use the old > definition style. Right. Done. > Does PROPERTY_CACHE_TEST expand and optimize as well as an inline as a macro? > Maybe check generated non-optimized code for quick sanity check. If that's as > good or better, no worries. It's easier for me to diff the optimized code. There are differences. Different register allocation and order of instructions. Spills happen in different places. The generated code is "as good", no better. These are ready to roll tomorrow. Tree's pretty ugly right now.

That didn't go quite as planned, but it's ready.

Comment on attachment 434665 [details] [diff] [review] Part 3, renaming - v1 >@@ -4499,18 +4499,18 @@ js_DefineNativeProperty(JSContext *cx, J > /* XXXbe called with lock held */ > if (!AddPropertyHelper(cx, clasp, obj, scope, sprop, &value)) { > scope->removeProperty(cx, id); > goto error; > } > > if (defineHow & JSDNP_CACHE_RESULT) { > JS_ASSERT_NOT_ON_TRACE(cx); >- JSPropCacheEntry *entry; >- entry = js_FillPropertyCache(cx, obj, 0, 0, obj, sprop, added); >+ PropertyCacheEntry *entry; >+ entry = JS_PROPERTY_CACHE(cx).fill(cx, obj, 0, 0, obj, sprop, added); I don't see a goto reason for not initializing entry, and you're touching both lines, so initialize. >+ empty = JS_FALSE; Can you use false here without issue? Etc. if other such cases arise in the lines touched by this patch. r=me with above dealt with. /be

Part 1: http://hg.mozilla.org/tracemonkey/rev/ba2ef20d98de Part 2: http://hg.mozilla.org/tracemonkey/rev/fd0080b3e611 Part 3: http://hg.mozilla.org/tracemonkey/rev/23e9bcf00a86 (I made both changes mentioned in comment 17 and changed one more place where empty was being set to JS_TRUE.)

Comment on attachment 434666 [details] [diff] [review] Part 4, encapsulate pcval - v1 >+ uintptr_t v; > jsuword kshape; /* shape of direct (key) object */ > jsuword vcap; /* value capability, see above */ >- jsuword vword; /* value word, see PCVAL_* below */ >+ PCVal vword; /* value word, see PCVal above */ How about sticking to jsuword for consistency -- no difference in effect. Also getSlot should return uint32, not jsuint, and so on (slot is uint32 elsewhere -- is this suboptimal on 64-bit targets?). Can you static-assert that sizeof(PCVal) == sizeof(jsuword)? /be

Done. Part 4: http://hg.mozilla.org/tracemonkey/rev/23442b4a6e20

Comment on attachment 434668 [details] [diff] [review] Part 5, make PropertyCache fields private - v1 Looks good -- can you do a before/after optimized code size sanity check, or something better? The PropertyCache::test* API (return true for direct or similar hit, false with !atom for "full test" hit, false with atom for miss) is a bit clunky, not to say the old code was better. Mainly looking for gcc confidence-building news. The comments after some of the direct/fast hit cases say "Property cache hit: ..." and so might want to be more specific, since now those clauses have an else if (!atom) { /* slower hit */ } tail. /be

GCC jsinterp.o file size before: 80772 after: 80900 Unfortunately js_Interpret has about 3000 basic blocks, and they get shuffled when I apply this patch. I think the file size change is a result of the shuffling, but it's hard to tell. So much for confidence-building at this low level. But I did run sunspider (without -j, the better to bang on JSPropertyCache) and found no change. Also: njn's cachegrinding in bug 554996 used an omnibus patch that actually included this change (parts 4 and 5 in this bug), and the results there were all positive.

Part 5: http://hg.mozilla.org/tracemonkey/rev/eb5999d8b46f