243488 - [FIXr]Consider ways to speed up documentNeedsSecurityCheck

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

To be precise, does the new stuff brendan adds in the patches in bug 213946 make
it possible to speed up this code a bit?  It looks to me like it maybe should....

See http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMClassInfo.cpp#5086
and especially the loop at
http://lxr.mozilla.org/seamonkey/source/dom/src/base/nsDOMClassInfo.cpp#5121

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, it looks like that code only returns principals, whereas what we want here
is the global object associated to whatever function object is on the stack....

I still wonder whether it's worth a JS api change to speed this up -- it's done
for all sorts of access of properties of document objects, so it may help some
DHTML stuff along a tad...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that needsSecurityCheck() has similar issues/code.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Make the cache work, actually

I bet this will help!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Actual patch

Comment 5

[Brendan Eich [:brendan]]

bz: any perf numbers or profiling before/after?

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

I think it sped up the "many document.write calls" testcase by about 1% or so;
I'd need to retest to make sure...  too many patches hanging out in my build at
this point to tell off the top of my head.

If you need more specifics, let me know and I'll reprofile.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 148547 [details] [diff] [review]
Actual patch

 static inline PRBool
 needsSecurityCheck(JSContext *cx, nsIXPConnectWrappedNative *wrapper)
 {
-  // Cache a pointer to a wrapper and a context and set these pointers
-  // to point to the wrapper and context that doesn't need a security
-  // check, thus we avoid doing all this work to find out if we need
-  // to do the security check, in most cases this check would end up
-  // being two pointer compares.
+  // We cache a pointer to a wrapper and a context that we've last vetted
+  // and cache what the verdict was.

   // First, compare the context and wrapper with the cached ones
-  if (cx != cached_cx || wrapper != cached_wrapper) {
-    cached_cx = nsnull;
-    cached_wrapper = nsnull;
+  if (cx == cached_win_cx && wrapper == cached_win_wrapper) {
+    return !cached_win_skip_check;

Since this cached_win_skip_check is used in a method that checks whether or not
we *need* to do a security check, maybe you should name that new static
variable cached_win_needs_check? If you'd do that, you can remove some reverse
logic as well.

r=jst

Comment 8

[Boris Zbarsky [:bzbarsky]]

I did it to avoid static PR_TRUE, but you're right -- the reversed logic is not
worth it.  I'll rename accordingly.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated patch

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 149567 [details] [diff] [review]
Updated patch

Brendan, would you sr?

One other thing.  I've been running into these functions in some DHTML profiles
for things that use chained setTimeout() calls (setting a timeout from within a
timeout).  I assume that's because the JSContext will be different for each
timeout firing?  If so, we may want to still reduce the overhead here....

Comment 11

[Brendan Eich [:brendan]]

> One other thing.  I've been running into these functions in some DHTML profiles
> for things that use chained setTimeout() calls (setting a timeout from within a
> timeout).  I assume that's because the JSContext will be different for each
> timeout firing?

No, that shouldn't be happening if the setTimeout is on the current window.  Can
you dig into why the cache is being missed?  Something's not quite right here.

/be

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ok.  I'll check on it (need to recall which profile this was...).

Probably when I come back from my trip.

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK, I can't reproduce the cache misses on any of the profiles I tried today...
if I see them again, I'll look into it.  Brendan, are you happy enough with the
patch as-is to sr?

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 149567 [details] [diff] [review]
Updated patch

Sure.

/be

Comment 15

[Boris Zbarsky [:bzbarsky]]

Checked in.