478406 - jsd degrades JavaScript performance even without being used

Status: RESOLVED WORKSFORME

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[Wladimir Palant]

I was trying to understand how JSD initializes itself and was very surprised to see that all SpiderMonkey hooks are registered at application startup rather than when jsdIDebuggerService.on() is called. In fact, calling jsdIDebuggerService.on() seem unnecessary - just setting the necessary callbacks already does the job (unless you have more than one JavaScript runtime in the application which shouldn't be too common).

This obviously has an effect on performance. I used SunSpider to check out how much it degrades performance. Did four runs in Minefield build 20090212 on Windows XP, second and forth runs without JSD being initialized (removed the corresponding entries from compreg.dat manually, removing component doesn't work because it is compiled statically into xul.dll). The results:

2701.8ms +/- 2.3%
2343.2ms +/- 4.8%
2707.0ms +/- 1.4%
2328.8ms +/- 4.5%

So without JSD the results get better by 13-14%, that is everything but negligible. I see the largest difference for binary-trees and recursive, I guess those are call-heavy.

Some details: the SpiderMonkey hooks are registered in jsd_DebuggerOnForUser() (jsd_high.c). That one is being called from jsdService::OnForRuntime() (jsd_xpc.cpp) which in turn is called from jsdASObserver::Observe() (jsd_xpc.cpp). jsdASObserver happens to be an app startup observer.

In total eight hooks are registered. Of those six don't do anything at all until some extension decides to set some hooks on jsdIDebuggerService. So there is absolutely no reason why these six hooks shouldn't be registered "on demand". The other two (jsd_NewScriptHookProc and jsd_DestroyScriptHookProc) track active scripts to make sure information on them can be retrieved later (passed to hooks registered at jsdIDebuggerService, also allows script enumeration via jsdIDebuggerService.enumerateScripts()). I have to wonder whether this cannot be implemented in a way that only impacts performance when JSD is actually used. Note that SunSpider probably doesn't measure overhead from this script tracking because it has no influence on script execution time.

Comment 1

[:Gijs (out for now; he/him)]

Did you test this on a clean profile? I'm assuming you did, but just to be sure...

Comment 2

[:Gijs (out for now; he/him)]

(In reply to comment #0)
> 2701.8ms +/- 2.3%
> 2343.2ms +/- 4.8%
> 2707.0ms +/- 1.4%
> 2328.8ms +/- 4.5%

Interesting that the variance increases so much, by the way...

Comment 3

[Wladimir Palant]

(In reply to comment #1)
> Did you test this on a clean profile?

Argh... Yes, I'm an idiot. Venkman does "jsds.initAtStartup = true" when registering its own component. JSD by itself won't initialize at startup.

Not that there is no room for improvement here but that will be a different bug.

=> WORKSFORME

Comment 4

[Robert Kaiser]

Shouldn't this be changed in a way that there's a pref for it in venkman an one can have venkman installed without degrading perf right away?

Comment 5

[:Gijs (out for now; he/him)]

(In reply to comment #4)
> Shouldn't this be changed in a way that there's a pref for it in venkman an one
> can have venkman installed without degrading perf right away?

As Wladimir already wrote in his blog post, you can just use the /startup-init command to turn it on/off. That choice is dealt with using the category manager (inside JSD, that is), so that should persist. I'm not really sure what job a pref should do, except allowing the risk of things getting out of sync.

Comment 6

[Brendan Eich [:brendan]]

So long as the category setting persists, no need for a pref. But is the default wrong, then?

/be

Comment 7

[:Gijs (out for now; he/him)]

(In reply to comment #6)
> So long as the category setting persists, no need for a pref. But is the
> default wrong, then?
> 
> /be

Without bug 480765 being fixed, if the debugger is not enabled at startup, it will not be able to debug most of the chrome code you'd care about, and website code only after a full reload. I'm not really sure why you'd install Venkman if you didn't care to debug anything. So I'm not sure the default is wrong - a 10-15% perf hit on a JS benchmark is all well and good, but I somewhat doubt the performance hit on the average webpage will be as severe. I certainly never noticed it.

Comment 8

[Robert Kaiser]

My problem is that SeaMonkey ships with venkman installed by default, but giving all our users a perf penalty is not something I really want to accept.

Comment 9

[:Gijs (out for now; he/him)]

(In reply to comment #8)
> My problem is that SeaMonkey ships with venkman installed by default, but
> giving all our users a perf penalty is not something I really want to accept.

Like I said, I doubt that the perf penalty is very severe on ordinary pages. Additionally, any solution would have to address the "debugability" (for want of an actual English word there) problem as well. If you have ideas about that, then let's hear it, but right now it does not make sense to turn this off by default across the board.

I can see you might want to not turn it on by default on SeaMonkey, for which I'd be happy to take a patch, but presumably that would have to somehow enable it on the first actual run of Venkman (so that people who do actually want to use it can do so). Whichever way, that'd be a different bug.

Comment 10

[Robert Kaiser]

(In reply to comment #9)
> (In reply to comment #8)
> > My problem is that SeaMonkey ships with venkman installed by default, but
> > giving all our users a perf penalty is not something I really want to accept.
> 
> Like I said, I doubt that the perf penalty is very severe on ordinary pages.

I want same perf on websites as plain Firefox. If we can't provide that and venkman is at fault, we need to solve that in some way.

> Additionally, any solution would have to address the "debugability" (for want
> of an actual English word there) problem as well. If you have ideas about that,
> then let's hear it, but right now it does not make sense to turn this off by
> default across the board.

So, it doesn't make sense to disable it because it doesn't work correctly anyway? I either don't understand that logic or your explanation.

> I can see you might want to not turn it on by default on SeaMonkey, for which
> I'd be happy to take a patch, but presumably that would have to somehow enable
> it on the first actual run of Venkman (so that people who do actually want to
> use it can do so). Whichever way, that'd be a different bug.

I actually think it should be obvious to people that it's being turned on or off, so IMHO, it should be at a setting by default that actually doesn't turn it on but is distiguishable from permanent off by venkman, and when venkman launches and it's in that default state, it either turns on and put up a VERY visible warning what it has done, why, and how to turn it off, or it should ask the user if it should be turned on or off (explaining what it does and the possible perf degradation).

Comment 11

[:Gijs (out for now; he/him)]

(In reply to comment #10)
> (In reply to comment #9)
> > Additionally, any solution would have to address the "debugability" (for want
> > of an actual English word there) problem as well. If you have ideas about that,
> > then let's hear it, but right now it does not make sense to turn this off by
> > default across the board.
> 
> So, it doesn't make sense to disable it because it doesn't work correctly
> anyway? I either don't understand that logic or your explanation.

The logic is that when someone installs Venkman separately (which, apart from SeaMonkey, is the typical scenario), they expect to be able to use it. If this setting is not turned on, in the current circumstances (without bug 480765 fixed), that is not the case (or, to be specific, its usefulness will be severely reduced). Users will be required to figure out why, to turn on the right setting, and restart, in order to use it normally. That's not an acceptable penalty on the user, when it could just have been turned on at startup, which is fine for anyone but SeaMonkey users who have no interest in Venkman.

> > I can see you might want to not turn it on by default on SeaMonkey, for which
> > I'd be happy to take a patch, but presumably that would have to somehow enable
> > it on the first actual run of Venkman (so that people who do actually want to
> > use it can do so). Whichever way, that'd be a different bug.
> 
> I actually think it should be obvious to people that it's being turned on or
> off, so IMHO, it should be at a setting by default that actually doesn't turn
> it on but is distiguishable from permanent off by venkman, and when venkman
> launches and it's in that default state, it either turns on and put up a VERY
> visible warning what it has done, why, and how to turn it off, or it should ask
> the user if it should be turned on or off (explaining what it does and the
> possible perf degradation).

There is no way we can have this option be a tristate, and I see no need for it to be. Venkman would have to check on startup if the setting were off and if this is its first run (it keeps a counter of those anyway, for whatever reason). And like I said, implementing a SeaMonkey-specific solution is a different bug.

Comment 12

[:Gijs (out for now; he/him)]

(In reply to comment #11)
> when it could just have been turned on at startup

That should have been "turned on at initial component registration", oops.

Comment 13

[Robert Kaiser]

You're right in that this should be a different bug, so I filed bug 483282 for this followup problem. I still see this as a major problem as we are in times where bad JS perf numbers can cost SeaMonkey its very target audience, as advanced users and web devs are currently told that JS perf tests are the number one sucks/rocks meter of browser development (no matter if that's true in reality or not, too many people believe it that it matters enough).