500431 - Encapsulate the property cache using C++ best practices

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

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

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: moves v1

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

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: encapsulate v1

The second part actually changes stuff.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: encapsulate v1 - diff -b patch for jsops.cpp

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

Comment 4

[Jeff Walden [:Waldo]]

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.

Comment 5

[Brendan Eich [:brendan]]

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

Comment 6

[Jason Orendorff [:jorendorff]]

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

Comment 7

[Jason Orendorff [:jorendorff]]

Backed out. Orange everywhere.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Part 1, moves - v1

This patch only moves stuff from file to file.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Part 1, moves - v2

Oops - I moved something to the wrong file.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Part 2, inline functions - v2

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 11

[Brendan Eich [:brendan]]

Comment on attachment 434352 [details] [diff] [review]
Part 1, moves - v2

Wafer-thin!

/be

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Jason Orendorff [:jorendorff]]

(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.

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: Part 3, renaming - v1

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: Part 4, encapsulate pcval - v1

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: Part 5, make PropertyCache fields private - v1

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

Comment 17

[Brendan Eich [:brendan]]

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

Comment 18

[Jason Orendorff [:jorendorff]]

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 19

[Brendan Eich [:brendan]]

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

Comment 20

[Jason Orendorff [:jorendorff]]

Done.

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

Comment 21

[Brendan Eich [:brendan]]

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

Comment 22

[Jason Orendorff [:jorendorff]]

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.

Comment 23

[Jason Orendorff [:jorendorff]]

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

Comment 24

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ba2ef20d98de