419093 - property performance still worse in-browser on a few tests

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Robert Sayre]

- bitwise-and
- math-partial-sums
- string-validate-input

Comment 1

[Robert Sayre]

So, for bitwise-and, these are not misses. Just initially expensive, I guess.

Property cache stats for thread 5285312, GC 6
      fills      32211
    nofills        407
    rofills          0
   disfills          0
   oddfills         10
   modfills      27576
 brandfills       3691
 longchains          0
   recycles      19848
 pcrecycles       1512
      tests   57680232
     pchits   57640969
protopchits       9770
   settests   19204229
  addpchits        244
  setpchits   19201102
setpcmisses        467
  setmisses       2696
   idmisses      29451
   komisses       2299
   vcmisses        766
     misses      32516
    flushes          6
hit rates: pc 99.9319% (proto 0.0169382%), set 99.985%, full 99.9436%

Comment 2

[Robert Sayre]

math-partial-sums

Property cache stats for thread 5285312, GC 10
      fills     585429
    nofills        237
    rofills          0
   disfills          0
   oddfills         10
   modfills     582870
 brandfills       2718
 longchains          0
   recycles     571088
 pcrecycles       1294
      tests   16896830
     pchits   16307250
protopchits       4170
   settests    4215712
  addpchits        240
  setpchits    4212898
setpcmisses        224
  setmisses       2498
   idmisses     584533
   komisses        978
   vcmisses        255
     misses     585766
    flushes         10
hit rates: pc 96.5107% (proto 0.0246792%), set 99.9389%, full 96.5333%

Comment 3

[Robert Sayre]

string-validate-input

Property cache stats for thread 5285392, GC 9
      fills    1260750
    nofills        486
    rofills          0
   disfills          0
   oddfills         10
   modfills    1255929
 brandfills       3951
 longchains          0
   recycles    1243530
 pcrecycles       1685
      tests   11438203
     pchits   10169939
protopchits    1132165
   settests     664677
  addpchits        292
  setpchits     656978
setpcmisses        438
  setmisses       7211
   idmisses    1258135
   komisses       2333
   vcmisses        759
     misses    1261227
    flushes          9
hit rates: pc 88.912% (proto 9.8981%), set 98.8856%, full 88.9736%

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

In-browser, I'm now seeing this in bitwise-and:

Property cache stats for thread 7382496, GC 5
      fills      20467
    nofills          0
    rofills          0
   disfills          0
   oddfills         13
   modfills      14704
 brandfills       1827
 longchains          0
   recycles      12816
 pcrecycles       1145
      tests    1846538
     pchits    1820745
protopchits       5429
   initests       2883
  inipchits        316
inipcmisses       2567
   settests     602701
  addpchits        405
  setpchits     600113
setpcmisses        171
slotchanges          0
  setmisses       2075
   idmisses      16065
   komisses       1718
   vcmisses        221
     misses      18004
    flushes          4
hit rates: pc 98.6032% (proto 0.29401%), set 99.6378%, ini 10.9608%, full 99.025%

Not horrible, but a drop for sure, and strange numerology.  Did the interpreter-version fix of the global warming bug cover all the same cases?

I think these numbers are pretty clean; I used the startShark/stopShark bracketing in my own hosted copy to call JS_GC from gdb before and after the test itself.  Really not sure where we would get 16K misses, though...

Comment 5

[Brendan Eich [:brendan]]

those numbers are cumulative from startup, and count all the XUL JS that runs. We really want some native helpers (I was teasing sayrer into implementing them as an encore to his shark natives) to clear and dump the property cache meters. We could even hack things to target just one content window's context, say, if XUL JS was adding noise.

/be

Comment 6

[Damon Sicore (:damons)]

Shaver, please take a look at this (blame brendan).

Comment 7

[Brendan Eich [:brendan]]

Uh, idea was to blame me for gifting this bug to shaver. Shaver, can you take it? If not, bounce back. Thanks,

/be

Comment 8

[Johnny Stenback (:jst)]

Attachment: Make bitwiseAndValue stay out of XPConnect.

This makes us define fast expandos for undeclared global property sets as well as declared ones.

Comment 9

[Johnny Stenback (:jst)]

Taking bug.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 307067 [details] [diff] [review]
Make bitwiseAndValue stay out of XPConnect.

Nit about oldobjp name -- the p at the end (flouting decades of LISP tradition ;-) connotes double indirection. So oldobj would work for me better.

>+  JSObject *realObj, *proto;
>+  wrapper->GetJSObject(&realObj);

Push this cost down by splitting the && chain below:

>+
>+  // Make a fast expando if we're assigning to (not declaring) a new
>+  // undefined property that's not already defined on our prototype
>+  // chain. This way we can access this expando w/o ever getting back
>+  // into XPConnect.
>+  if ((flags & (JSRESOLVE_ASSIGNING)) &&
>+      win->IsInnerWindow() 

Here:

>+... && obj == realObj &&

Makes for if-...-if nesting one deeper but no big.

>+      (proto = STOBJ_GET_PROTO(obj))) {

Don't you want to do the fast expando even (especially) if proto is null here? The proto-not-null guard should apply to the JS_HasUCProperty test only.

>+    JSString *str = JSVAL_TO_STRING(id);
>+    jschar *chars = ::JS_GetStringChars(str);
>+    size_t len = ::JS_GetStringLength(str);
>+
>+    JSBool hasProp;
>+    if (!::JS_HasUCProperty(cx, proto, chars, len, &hasProp)) {
>+      *_retval = JS_FALSE;
>+
>+      return NS_OK;
>+    }

JS_Has*Property will resolve, but this newresolve hook invocation (the one we are in) won't know to communicate that back to the JS engine. That seems like trouble. Do you want to use OBJ_LOOKUP_PROPERTY here instead? I think so (hairy, but there it is). That way you can pass &pobj and return it if prop. Don't forget OBJ_DROP_PROPERTY if no error and prop non-null.

/be

Comment 11

[Brendan Eich [:brendan]]

jst asked over IRC, then had to run: I didn't mean that resolving on the proto is bad, clearly we need that. What's missing is doing that but not setting *objp to the object on which JS_HasUCProperty resolved. Calling OBJ_LOOKUP_PROPERTY is the way to go.

/be

Comment 12

[Johnny Stenback (:jst)]

Attachment: Updated per brendan's comments.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 307082 [details] [diff] [review]
Updated per brendan's comments.

>+  JSObject *oldobjp = *objp;
>+  rv = nsEventReceiverSH::NewResolve(wrapper, cx, obj, id, flags, objp,
>+                                     _retval);
>+
>+  if (NS_FAILED(rv) || *objp != oldobjp) {

My nit about oldobj > oldobjp may have been missed, I put it up high without any code excerpt.

>+      JSObject *proto = STOBJ_GET_PROTO(obj);
>+      jsid interned_id;
>+      JSProperty *prop = nsnull;
>+      if (proto &&
>+          (!::JS_ValueToId(cx, id, &interned_id) ||
>+           !OBJ_LOOKUP_PROPERTY(cx, proto, interned_id, objp, &prop))) {
>+        *_retval = JS_FALSE;
>+
>+        return NS_OK;
>+      }
>+
>+      if (prop) {
>+        // A property was found on the prototype chain, and *objp is
>+        // already set to point to the prototype where the property
>+        // was found.
>+        OBJ_DROP_PROPERTY(cx, proto, prop);
>+
>+        return NS_OK;
>+      }

At this point, with the case analysis so far, it seems better to me to make the logic look like this:

  if (proto) {
    JSProperty *prop;

    if (!value-to-id || !OBJ_LOOKUP_PROP) {
      FALSE/OK
    }
    if (prop) {
      DROP/OK
    }
  }

No need to initialize prop to nsnull this way -- we know that control can't flow to if (prop) without OBJ_LOOKUP_PROPERTY succeeding, in which case it must have set prop.

Null proto is kind of an edge case, not concerned about cycle costs so much as clarity.

r/a=me with that.

/be

Comment 14

[Johnny Stenback (:jst)]

Attachment: Fix that landed.

Comment 15

[Johnny Stenback (:jst)]

Fix checked in! File new bugs on remaining issues if any.