43902 - while(1) i++; way slower in Mozilla than in JS shell tests

Status: VERIFIED FIXED

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

Description

[Peter "jag" Annema]

When doing some tests for bug 13350 we discovered

  while(true) i++;

takes a lot longer to execute inside Mozilla than in a JS shell test.

Comment 1

[Brendan Eich [:brendan]]

Adding Jim Nance, wondering if he can point jprof at this problem, or help us to
do it.

/be

Comment 2

[Johnny Stenback (:jst)]

We have just gotta do something about this for beta3 at least, here's some
timing stats I collected while tuning things for bug 13350, this is running
"var i=0; while(true){i++}:"

Time [us]  Iterations
479272,    4096
962961,    8192
1442295,   12288
1921476,   16384
2399275,   20480

And here's the same data for "while(1);"

2298,      4096
4652,      8192
6965,      12288
9277,      16384
11621,     20480

Quite a difference, I'd say

Comment 3

[Brendan Eich [:brendan]]

jst, can you come up with an idlc patch for everyone to try, that moves the 
GetNativeThis and CallJSScriptGetProperty invocations into the "then" part of 
the if (JSVAL_IS_INT(id)) {...}?  That might just do the trick.  It would be a 
win all around.

/be

Comment 4

[ekrock's old account (dead)]

perf.  PDT team please approve.

Comment 5

[Nisheeth Ranjan]

Marking nsbeta3+...

Comment 6

[John Bandhauer]

Some more data...

I ran the code I'll attach in 3 places with the following timings:

1) in an html page using mozilla:

global increment did 10000 iterations in 1.87 seconds.
object increment did 10000 iterations in 1.81 seconds.
local var increment did 10000 iterations in 0.91 seconds.

2) In xpcshell:

global increment did 10000 iterations in 0.12 seconds.
object increment did 10000 iterations in 0.13 seconds.
local var increment did 10000 iterations in 0.05 seconds.

3) in jsshell:

global increment did 10000 iterations in 0.04 seconds.
object increment did 10000 iterations in 0.04 seconds.
local var increment did 10000 iterations in 0.01 seconds.

The "global increment" test manipulates a global var: ++i
The "object increment" test manipulated a prop of a global var: ++obj.i
The "local var" test manipulates a local within a function.

xpcshell and jsshell are essentially the same code. The difference between '2' 
and '3' can be wholly attributed to xpcshell running against a build of js with 
JS_THREADSAFE on (using locks to serialize object accesses). This is the same js 
build used in seamonkey. The jsshell build is running with no locks at all.

The difference between '1' and '2' has to be attributed to something the DOM is 
doing. It doesn't look like resolving against the global object is all of the 
difference. Are we doing something slow too often in the infinite loop 
detection?

Comment 7

[John Bandhauer]

Attachment: my test case. copy out the inner part to run in shell

Comment 8

[John Bandhauer]

With slight modification to the test...

4) the timing in Nav 4.73:
global increment did 10000 iterations in .24 seconds. 
object increment did 10000 iterations in .16 seconds. 
local var increment did 10000 iterations in 0.09 seconds. 

Comment 9

[Brendan Eich [:brendan]]

We're doing slow stuff in the GetWindowProperty and SetWindowProperty functions
generated by DOM idlc, for sure -- all nsJSUtils::nsGetNativeThis calls should
be in the smallest block that contains uses of the native this.  I'm not sure
what can be done about nsJSUtils::nsCallJSScriptObjectGetProperty, it seems to
be necessary for non-tinyid (not predefined) properties, but it surely costs
too.

Anyone want to try hacking around with nsJSWindow.cpp (generated) or DOM idlc?

We may need to consider optimizing things to there isn't always overhead
incurred by nsJSUtils::nsCallJSScriptObjectGetProperty.

/be

Comment 10

[John Bandhauer]

Maybe I'm just adding noise, but...

The top three above were done with debug builds of the trunk

5) official PR2 release build:
global increment did 10000 iterations in 0.62 seconds. 
object increment did 10000 iterations in 0.61 seconds. 
local var increment did 10000 iterations in 0.3 seconds.

Comment 11

[John Bandhauer]

Ah, I thought my 'local var' test was avoiding global lookups. So I looked at 
the branch callback. But with my test JS was still looking up 'iterations' each 
time through the loop (even though it was declared 'const').

Changing my code to...

function loc() {
    var i = 0;
    const iter = iterations
    while(1) {
    if(++i > iter)
        break;
    }         
}         

...cuts the execution time from 0.09 to 0.01

Comment 12

[John Bandhauer]

changing only the GetWindowProperty gives me this:

global increment did 10000 iterations in 0.24 seconds. 
object increment did 10000 iterations in 0.14 seconds. 
local var increment did 10000 iterations in 0.01 seconds. 

This makes sense because "object increment" does no sets of window props. I'll 
attach my hack.

Comment 13

[John Bandhauer]

Attachment: hack to generated file showing getter speedup

Comment 14

[vidur (gone)]

The first part of jband's hack looks OK, but we've can't stop calling 
nsJSUtils::nsCallJSScriptObjectGetProperty(). At least not without figuring out 
a way to deal with the properties handled in nsGlobalWindow's GetProperty() 
method.

Comment 15

[Brendan Eich [:brendan]]

jband: can you hack SetWindowProperty too and see whether that makes the global
with-DOM case as fast as the global xpcshell case?  (Also, top-level consts are
not folded yet, sorry; I'm the only one likely to do that optimization in the
SpiderMonkey engine, and I haven't got around to it.)

vidur: it's crucial that the DOM either predefine well-known properties with
tinyids, so we can avoid this overhead for all others, or lazily define certain
props, again with tinyids.  If there are window properties (e.g., "location")
that are handled on every get or set via a strcmp, that's bad.  I'll help fix
this stuff, although jst should do the idlc whackage.

/be

Comment 16

[Johnny Stenback (:jst)]

I'll be glad to do the IDLC whackage once I know what the generated code should
look like...

Comment 17

[John Bandhauer]

Similar hack to SetWindowProperty gives:
increment did 10000 iterations in 0.12 seconds. 
object increment did 10000 iterations in 0.14 seconds. 
local var increment did 10000 iterations in 0.01 seconds. 

So, yes, we have speed comparable to xpcshell.

I was really doing this just to verify that the slowdown is due to these global 
object property accesses. I didn't mean to imply that the hack leads directly to 
the fix.

So here's the good part...

Without the hacks above if I just comment out the security check in the final 
'else' clause in GlobalWindowImpl::GetProperty I get:

global increment did 10000 iterations in 0.35 seconds. 
object increment did 10000 iterations in 0.3 seconds. 
local var increment did 10000 iterations in 0.01 seconds. 

The security check is associated with:
http://bugzilla.mozilla.org/show_bug.cgi?id=20469

mstoltz and I discussed this with vidur.

GlobalWindowImpl::GetProperty lives to handle 'title', 'location', and the 
seurity check. With some idlc work vidur things he can move too use tinyids for 
this stuff and avoid the native 'this' lookup and the extra calls to even get to 
GlobalWindowImpl::GetProperty. 

If we fold these into tinyids and decide to forego the security check then this 
gets us a 10x speedup on the getters.

The GlobalWindowImpl::SetProperty stuff deals with 'title', 'location', and 
special handling for 'onXXX'. I don't know how well this can be folded into 
tinyids, but this stuff is cheap compared to the security check done in the 
getter case.

Comment 18

[Brendan Eich [:brendan]]

I think we should move the GetNativeThis and CallJSScriptObjectGetProperty (and
Set) into the tinyid-specific (if (JSVAL_IS_INT(id)) {...}) block.

Oh no, Travis inflicted fibonacci indentation on GlobalWindowImpl::GetProperty. 
Hmm, we can certainly give location and title tinyids.  That leaves the else
clause that does the security check.  IIRC, we want that in general for tougher,
IE-like same-origin checks on all property accesses.  But why only for string
ids (i.e., why not also for otherWindow[0], if that names a property)?

Can we have idlc generate the security check code, so we don't have to go
through the nsCallJSScriptObjectGetProperty layer in GetWindowProperty?  We can
optimize the security check further with a fast path: if the running script's
context (cx) is the same as the window's cx, pass.

The CheckForEventListener magic in GlobalWindowImpl::SetProperty could be done
only for an exhaustive list of tinyid-identified "onXXX" properties, instead of
for every function-valued property whose name starts with "on".

While I'm mumbling, don't we need a security check in SetProperty too?  Cc'ing
mstoltz for sanity checking.

/be

Comment 19

[Johnny Stenback (:jst)]

Vidur has agreed to take this bug off my plate, thanks!

Comment 20

[vidur (gone)]

I'll move the location and title code to use tiny ids (I'm not sure why title 
was special cased, but location was to account for different setter vs. getter 
types). I can move the security check (including the fast path) into the 
generated code as well. It's just more we'll have to deal with when we phase out 
idlc.

For the setter, I'm not sure yet where to put the CheckForEventListener call, 
but I'll look into it more.

Comment 21

[Brendan Eich [:brendan]]

Vidur: couldn't the CheckForEventListener be done only for a known list of
tinyid'd "onclick", etc. names, and not for any property id beginning with "on"?

/be

Comment 22

[vidur (gone)]

I've moved all code out of the nsGlobalWindow::GetProperty/SetProperty methods. 
The glue code no longer calls them. The glue code now does a quick security 
check, calling the security manager only if the global object of the calling 
context is not identical to the window object. The location, title and event 
handler properties were switched to using tinyids. Checking performance numbers 
now.

Comment 23

[John Bandhauer]

With the patch from vidur running in mozilla.exe I see:

global increment did 10000 iterations in 0.12 seconds. 
object increment did 10000 iterations in 0.12 seconds. 
local var increment did 10000 iterations in 0.01 seconds. 

10x improvement!

Comment 24

[Brendan Eich [:brendan]]

Meesa wanna see vidur's patch!  Please attach.

/be

Comment 25

[John Bandhauer]

Attachment: patch from vidur

Comment 26

[John Bandhauer]

Attachment: new file: dom/public/base/nsIDOMWindowEventOwner.h

Comment 27

[John Bandhauer]

I attached the files that vidur sent me. Note: these are *pre* DOM stirng 
changes. For some reason when I applied the patches I didn't get the makefile 
change to expport the new .h file (though the change seems to be in the patch) 
I don't know why. I hacked by hand.

I didn't look close at the code - jst promised to review.

Comment 28

[vidur (gone)]

Fix checked in on 8/28/2000.

Comment 29

[John Bandhauer]

*** Bug 31926 has been marked as a duplicate of this bug. ***

Comment 30

[Prashant Desale]

Verified with 2000-091309. Marking verified.