Encapsulate the property cache using C++ best practices

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
JavaScript Engine
P1
normal
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: brendan, Assigned: jorendorff)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(5 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Updated

9 years ago
Depends on: 497789
(Reporter)

Updated

9 years ago
Priority: -- → P1
(Assignee)

Comment 1

9 years ago
Created attachment 397632 [details] [diff] [review]
moves v1

The first part is just to move the propcache-related code from file to file.
Assignee: brendan → jorendorff
(Assignee)

Comment 2

9 years ago
Created attachment 397639 [details] [diff] [review]
encapsulate v1

The second part actually changes stuff.
Attachment #397639 - Flags: review?
(Assignee)

Comment 3

9 years ago
Created attachment 397640 [details] [diff] [review]
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.
(Assignee)

Updated

9 years ago
Attachment #397639 - Flags: review? → review?(jwalden+bmo)

Updated

9 years ago
Attachment #397639 - Flags: review?(jwalden+bmo) → review+

Comment 4

9 years ago
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.
(Reporter)

Comment 5

9 years ago
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
(Assignee)

Comment 6

9 years ago
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
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 7

9 years ago
Backed out. Orange everywhere.
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 8

8 years ago
Created attachment 434307 [details] [diff] [review]
Part 1, moves - v1

This patch only moves stuff from file to file.
Attachment #397632 - Attachment is obsolete: true
Attachment #434307 - Flags: review?(brendan)
(Assignee)

Comment 9

8 years ago
Created attachment 434352 [details] [diff] [review]
Part 1, moves - v2

Oops - I moved something to the wrong file.
Attachment #434307 - Attachment is obsolete: true
Attachment #434352 - Flags: review?(brendan)
Attachment #434307 - Flags: review?(brendan)
(Assignee)

Comment 10

8 years ago
Created attachment 434354 [details] [diff] [review]
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
Attachment #434354 - Flags: review?(brendan)
(Reporter)

Comment 11

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

Wafer-thin!

/be
Attachment #434352 - Flags: review?(brendan) → review+
(Reporter)

Comment 12

8 years ago
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
Attachment #434354 - Flags: review?(brendan) → review+
(Assignee)

Comment 13

8 years ago
(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.
(Assignee)

Comment 14

8 years ago
Created attachment 434665 [details] [diff] [review]
Part 3, renaming - v1
Attachment #434665 - Flags: review?(brendan)
(Assignee)

Comment 15

8 years ago
Created attachment 434666 [details] [diff] [review]
Part 4, encapsulate pcval - v1
Attachment #434666 - Flags: review?(brendan)
(Assignee)

Comment 16

8 years ago
Created attachment 434668 [details] [diff] [review]
Part 5, make PropertyCache fields private - v1

That didn't go quite as planned, but it's ready.
Attachment #397639 - Attachment is obsolete: true
Attachment #397640 - Attachment is obsolete: true
Attachment #434668 - Flags: review?(brendan)
(Reporter)

Comment 17

8 years ago
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
Attachment #434665 - Flags: review?(brendan) → review+
(Assignee)

Comment 18

8 years ago
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.)
(Reporter)

Comment 19

8 years ago
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
Attachment #434666 - Flags: review?(brendan) → review+
(Assignee)

Updated

8 years ago
Blocks: 554996
(Reporter)

Comment 21

8 years ago
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
Attachment #434668 - Flags: review?(brendan) → review+
(Assignee)

Comment 22

8 years ago
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.
(Assignee)

Comment 23

8 years ago
Part 5: http://hg.mozilla.org/tracemonkey/rev/eb5999d8b46f
Whiteboard: fixed-in-tracemonkey

Comment 24

8 years ago
http://hg.mozilla.org/mozilla-central/rev/ba2ef20d98de
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.