We need to be sure that normal use of SeaMonkey isn't impacted by having venkman installed or not, any perf or other degradation that is impended on users without the connected feature being used regularly is unacceptable and must be stopped. Bug 478406 shows that venkman actually makes users pay a price in terms of perf for having this extension installed at all, independent if it's used or not. This must be fixed if we want to ship venkman with SeaMonkey 2. Actually, I am convinced that such an impact is unacceptable even for people who only use it every now and then, unless they are prominently pointed to that fact when they use it first. Nobody should pay a such a price for just having installed an extension for testing, even a debugger should not have any impact when one isn't actively debugging anything. If someone thinks that it's just in SunSpider and nobody sees this on normal websites, I don't believe this unless I see solid numbers about this. And then, SunSpider and similar JS tests currently get the number 1 sucks/rocks meter for browsers, esp. among the web dev and advanced user crowd that SeaMonkey is targeting, so no chance we accept any penalty in comparison to Firefox when it comes to normal use of the browser without actually debugging JS. I'd rather ship our suite without this JS debugger than to make us look bad in that testing, as bad JS perf numbers in the current stage of browser competition could cost us our very target audience and the whole project with it. I really want to ship with venkman available to our users, so some solution along the lines of what I pointed out in bug 478406 comment #10 or whatever else can get us to not degrading numbers for casual browsing would be appreciated a lot.

(In reply to comment #0) > Bug 478406 shows that venkman actually makes users pay a price in terms of > perf for having this extension installed at all, independent if it's used > or not. I'm not sure what Wladimir's numbers are based upon - what does this SunSpider test? Chrome, content, both? Is this the penalty imposed by turning Tracemonkey off as soon as a debugger comes into play? Do we have a similar impact on the 1.8 branch (SeaMonkey 1.x)? > If someone thinks that it's just in SunSpider and nobody sees this on normal > websites, I don't believe this unless I see solid numbers about this. Erm, I don't know SunSpider, but shouldn't we first make certain that there *is* a problem? > I really want to ship with venkman available to our users, so some solution > along the lines of what I pointed out in bug 478406 comment #10 or whatever > else can get us to not degrading numbers for casual browsing would be > appreciated a lot. Venkman in SeaMonkey is usually started via its menuitem, so there's a single point of interaction we could use... But first we need to know *exactly* what is happening and why!

Sunspider is a performance benchmark for content... It is intended to emulate (in a visible way) what a person would encounter in terms of JS script constructs in the wild. We don't have talos available for SeaMonkey, but if you need real numbers, install Venkman to Firefox, and run 10 talos runs with Venkman, and 10 without (see if we can get the "assistance" to do it). We'll have very similar results with those tests, and that is actually base-worthy. I'm included to agree with the assessment based on this, and we either need this turned off by default (at --least-- for seamonkey) or venkman not installed (I hear some users clammoring for Firebug/Chromebug instead of Venkman anyway) Do we need more proof than a respected community member encountering this problem with venkman installed, and figuring out "why"?

(In reply to comment #1) > I'm not sure what Wladimir's numbers are based upon - what does this SunSpider > test? Chrome, content, both? Pure content. It's an in-browser JS benchmark test, see http://www2.webkit.org/perf/sunspider-0.9/sunspider.html - and it's probably the most popular JS benchmark suite right now. > Is this the penalty imposed by turning Tracemonkey > off as soon as a debugger comes into play? I think that's either all or most of it. The original bug says 'Venkman does "jsds.initAtStartup = true" when registering its own component. JSD by itself won't initialize at startup.' and points to that as the problem, which indicates that the init is at fault, and as soon as it happens, we don't JIT, which is a problem for cases when we don't debug. > Do we have a similar impact on the > 1.8 branch (SeaMonkey 1.x)? Probably not. Test runs wanted! > Erm, I don't know SunSpider, but shouldn't we first make certain that there > *is* a problem? There is. Turning off JIT for normal use is not an option. That will be true even more once we turn on JIT for chrome by default. > Venkman in SeaMonkey is usually started via its menuitem, so there's a single > point of interaction we could use... Not completely. People might want to debug stuff that was already loaded when venkman was started, and I think I have heard that we run into a problem there.

AFAICT, based on the fix for bug 480765, the perf hit would be because spidermonkey needs to create jsdscripts for all the scripts. Because sunspider loads 26 tests 5 times, all of which use inline scripts (for which I believe jsdscripts are not cached), that boils down to 130 script creations and equally many destructions in the course of 2.7 - 2.3 = 0.4 seconds, which would amount to some ~3ms per jsdscript that's created (I'm assuming destructions are cheap, that is 0 cost, in comparison). Compared with the average webpage where pages will largely use external JS files (as they are reused among various pages), which can be cached, and where this large number of create/destroy sequences obviously won't occur in such a short timespan (unless you're suggesting you can actually manually navigate 130 webpages with absolutely no scripts in common in less than 3 seconds). So yeah, I am skeptical at best on the universality of the benchmark's perf hit in terms of average user webbrowsing. That being said, as was pointed out in the earlier comments, real numbers would be best. Given that we can't have those (unless someone knows of a "representative" tests for average browsing?), maybe we should at least tone down the hysteria. I don't think JIT-ing has anything to do with it, because (a) I assume Wladimir would have had the sense to turn that off anyway (isn't it off by default regardless?) and (b) I would expect the perf hit for turning that off to be much bigger, based on what I remember from all the sparkly perf graphs when we got JIT-ing, and (c) I think jsd only forces that to be turned off if the user actually uses a debugger, not just if jsd itself is enabled - though I'm only basing that statement on recollections of conversations with people at FOSDEM. (In reply to comment #3) > > Is this the penalty imposed by turning Tracemonkey > > off as soon as a debugger comes into play? > > I think that's either all or most of it. The original bug says 'Venkman does > "jsds.initAtStartup = true" when registering its own component. JSD by itself > won't initialize at startup.' and points to that as the problem, which > indicates that the init is at fault, and as soon as it happens, we don't JIT, > which is a problem for cases when we don't debug. Do you have a source link for that? As in, how did you verify this? Because (see my paragraph above) I don't believe that's true. > > Venkman in SeaMonkey is usually started via its menuitem, so there's a single > > point of interaction we could use... > > Not completely. People might want to debug stuff that was already loaded when > venkman was started, and I think I have heard that we run into a problem there. Correct.

JFTR: I ran SunSpider with SM 1.1.14, both with Venkman's /start-init set to "on" and "off", and it shows the same >10% perf hit there. So JIT isn't (a significant) part of the problem here. But, like Gijs, I'm not sure that SunSpider's results are as related to the webbrowsing experience as such. Not even SunSpider itself does claim that! | This benchmark tests the core JavaScript language only, | not the DOM or other browser APIs. If there is a significant perf hit here we should address it, cold-blooded and with a clear mind, without hysteria. (In reply to comment #3) > > Venkman in SeaMonkey is usually started via its menuitem, so there's a > > single point of interaction we could use... > > Not completely. People might want to debug stuff that was already loaded when > venkman was started, What I meant was: if we can turn off that perf hit by turning off Venkman initially, we here have a single point where we can display a confirmation dialog about turning in it on automatically in future. And we can have pref panel for it. (This is not so much about Venkman itself, but about its SM integration.) That said, I think the most important part to fix is Wladimir's bug 480765 comment 2, once 480765 is checked in.

(In reply to comment #5) > JFTR: I ran SunSpider with SM 1.1.14, both with Venkman's /start-init set to > "on" and "off", and it shows the same >10% perf hit there. So JIT isn't (a > significant) part of the problem here. Did you restart between those runs? That'd be required to test it properly...

So, I just did a number of SunSpider runs on my machine (see attached file for URLs that list detailed results), and my testing disagrees with what Karsten said here: 1.1.16pre (1.8 tree as of today, self-compiled): default config: 15719.8ms +/- 1.5% venkman "/startup-init off" config: 15806.2ms +/- 1.4% (no significant difference) 2.0b1pre (comm-central/1.9.1 tree as of today, self-compiled): default config: 2289.8ms +/- 10.7% venkman disabled: 1954.8ms +/- 9.4% JIT.content disabled + venkman disabled: 2751.6ms +/- 2.1% I found out that executing "/startup-init off" in venkman in 2.0b1pre doesn't stick after restart, is that a different bug? In any case, the tests verify that we don't disable JIT completely, but contrary to what Gijs states, JIT is on for content by default in the 1.9.1 tree. It's interesting that, on the same machine, there's still a very large speedup from 1.8.1 to 1.9.1 even with JIT disabled. As a conclusion, we really have a large gap of 17% slowdown of having venkman enabled vs. having it disabled, and I strongly believe we should not ship with that much of a penalty for a feature that the majority of our users never use, and which those who do use it only use for a very small fragment of their SeaMonkey usage.

(In reply to comment #7) > I found out that executing "/startup-init off" in venkman in 2.0b1pre doesn't > stick after restart, is that a different bug? Can you elaborate on what you mean by this? If it really didn't have any effect after restart (JSD was still loaded), that'd mean that JSD was loaded all the time anyway, which would make your results all the more strange but would also mean we don't have a clue what was going on. Finally, if you selfcompiled (why not test with stock builds?) then what buildconfig did you use, and is that representative, and did you use a clean profile? (I know these might seem like stupid questions, I don't mean to insult your intelligence, but after what happened in the previous bug (not a clean profile), I'd like to be sure...)

(In reply to comment #8) > (In reply to comment #7) > > I found out that executing "/startup-init off" in venkman in 2.0b1pre doesn't > > stick after restart, is that a different bug? > > Can you elaborate on what you mean by this? If it really didn't have any effect > after restart (JSD was still loaded), that'd mean that JSD was loaded all the > time anyway, which would make your results all the more strange but would also > mean we don't have a clue what was going on. Well, the good thing is that the new toolkit allows us to deactivate extensions so that nothing of them is loaded at all, and that's what I used in the "venkman disabled" cases, so JSD should be off there. But yes, any time I entered "/startup-init off" in venkman in the 2.0b1pre build, restarted it, opened venkman again and entered "/startup-init" to display the state, it was on again. In 1.1.16pre, it works as expected and is off on a restart. (Of course, I didn't run the test after having opened venkman in the same session, just as a note.) > Finally, if you selfcompiled (why not test with stock builds?) then what > buildconfig did you use, and is that representative, and did you use a clean > profile? The buildconfig is pretty default, just with some additional compile optimization for my processor enabled, and with Lightning enabled, which should be orthogonal - esp. as 1.1.16pre has the same compiler optimizations as 2.0b1pre, and the most important thing is the comparisons of venkman enabled/disabled (and jit enabled/disabled to compare its disabling cost with the cost of enabling venkman). The profiles are not completely clean, but fairly clean, I'm only using it for testing, and it's the same profile for all tests with the same SeaMonkey version (but 1.1.x and 2.x need different profiles). In any case, the test conditions should be good enough to give comparable results for those things I tested. Apparently, enabling JIT makes results vary a bit more, as you see with the larger +/- margins. Still, the differences are significant enough, and they are consistent if I run the tests multiple times. BTW, I just was pointed to http://www.legitreviews.com/article/929/1/ which features a test of various browsers on Windows, and also shows SeaMonkey 2.0a3 about 20% slower than Firefox 3.1b3 though both of those ship very similar Gecko and JS engine versions. They used the test at http://service.futuremark.com/peacekeeper/index.action and I ran this on the same 2.0b1pre build with both venkman enabled and disabled. In the default config with venkman enabled, I scored 491 Points (larger is better), with just venkman disabled, I scored 529 Points, which again seems to prove a penalty, even if it looks smaller here on my build. I don't want to discuss the latter benchmark here a lot, I just wanted to point out that reports like that are what gives us bad press and makes us lose users if we can't hold up with Firefox numbers.

(In reply to comment #9) > Well, the good thing is that the new toolkit allows us to deactivate extensions > so that nothing of them is loaded at all, and that's what I used in the > "venkman disabled" cases, so JSD should be off there. > But yes, any time I entered "/startup-init off" in venkman in the 2.0b1pre > build, restarted it, opened venkman again and entered "/startup-init" to > display the state, it was on again. In 1.1.16pre, it works as expected and is > off on a restart. (Of course, I didn't run the test after having opened venkman > in the same session, just as a note.) Seeing as 1.1.x isn't new toolkit, I presume that you used /startup-init there? It'd be interesting to see if that makes any difference on those builds compared to disabling it entirely (ie removing the component idl and xpt, cleaning up compreg.dat and so on - or just not installing it). That way we can establish if the entire perf regression could be addressed within Venkman* - or if there is something more complicated going on still. I hope not, but being sure would prevent us from being disappointed. * by not initialising jsd on SeaMonkey by default, and then enabling it with user permission if/when they open Venkman for the first time > > Finally, if you selfcompiled (why not test with stock builds?) then what > > buildconfig did you use, and is that representative, and did you use a clean > > profile? > > The buildconfig is pretty default, just with some additional compile > optimization for my processor enabled, and with Lightning enabled, which should > be orthogonal - esp. as 1.1.16pre has the same compiler optimizations as > 2.0b1pre, and the most important thing is the comparisons of venkman > enabled/disabled (and jit enabled/disabled to compare its disabling cost with > the cost of enabling venkman). > The profiles are not completely clean, but fairly clean, I'm only using it for > testing, and it's the same profile for all tests with the same SeaMonkey > version (but 1.1.x and 2.x need different profiles). > In any case, the test conditions should be good enough to give comparable > results for those things I tested. Apparently, enabling JIT makes results vary > a bit more, as you see with the larger +/- margins. Still, the differences are > significant enough, and they are consistent if I run the tests multiple times. OK. Did you install the same (trunk/latest-release) version of Venkman on both 1.1.x and 2.x? The reason I'm asking is that if /startup-init is broken, we need to figure out if that's an issue in core or a regression in Venkman itself... And to be clear, as far as I can tell now, I think that can stay more or less in this bug, as it is instrumental in solving the problem described in the summary of this bug. So you've definitely established (as I attempted to hypothesise in comment #4) that the difference is not due to JIT being disabled, but we still need to figure out what exactly *is* causing the problem. Are you in a position to profile this in some way? Your benchmark results are more severe than those reported by Wladimir, so it should be easier for you to pick up what actually causes the perf regression... As for the Windows test, for now I don't have easy access to a Windows machine where I can test this stuff, should be better when I'm home in two weeks. For now I will note the following: First, I fully understand your concerns on benchmark results, even though I continue to remain skeptical as to how well they reflect ordinary browsing reality. I will be happy to try to help minimise Venkman's impact on SeaMonkey's benchmark results in this bug and in any others that might crop up. However, I was also rather annoyed by the tenet of your blogpost and your initial reactions on bugzilla that give the impression that Venkman somehow randomly impacts perf without *any* reason. I have repeatedly tried to explain that there *is* a reason for why it is enabled at startup, why this is not usually an issue, and why the naive alternative (never enabling it by default) is extremely user-unfriendly. I didn't have the feeling my comments were read or understood, from the further comments that have been made as well as the blogpost just now. A more balanced view rather than just pointing fingers at Venkman would have been appreciated. Finally, I'd like to apologise if I was/am overly defensive about the matter. I hope we can fix this as soon as possible. Which leads me to the next apology, namely that I'm not more proactive here - sadly my time is limited as the end of term is nearing and for me that includes 2x 20 page research papers and MSc exams to prepare for, followed by a thesis to work on during summer...

(In reply to comment #10) > Seeing as 1.1.x isn't new toolkit, I presume that you used /startup-init there? Yes. > It'd be interesting to see if that makes any difference on those builds > compared to disabling it entirely (ie removing the component idl and xpt, > cleaning up compreg.dat and so on - or just not installing it). I agree that would be nice, but I'm somewhat short on time and that would need some additional fiddling with the build, I wonder if doing that with 1.1.x is really that helpful for anything else than satisfying curiosity. > OK. Did you install the same (trunk/latest-release) version of Venkman on both > 1.1.x and 2.x? No, I used whatever's the pre-installed default on both SeaMonkey builds. > The reason I'm asking is that if /startup-init is broken, we > need to figure out if that's an issue in core or a regression in Venkman > itself... I understand. It might be either from this testing, I just stumbled over it. > So you've definitely established (as I attempted to hypothesise in comment #4) > that the difference is not due to JIT being disabled, but we still need to > figure out what exactly *is* causing the problem. Exactly. If disabling /startup-init would have worked, I would have provided that as an additional data point, but unfortunately I uncovered that additional bug :( > Are you in a position to > profile this in some way? Not sure, both for that I don't know the tools, don't know how to interpret their data, and additionally for not knowing how easy I find the time for it. That said, it is of high enough priority to me to spend some time on it, for sure (as I did today as well), but unfortunately I always have some amount of high-priority tasks lined up (don't ask how long low-priority stuff takes to be picked up - if at all). > First, I fully understand your concerns on benchmark results, even though I > continue to remain skeptical as to how well they reflect ordinary browsing > reality. I think we're on the same page here. > However, I was also rather annoyed by the tenet of your blogpost and your > initial reactions on bugzilla that give the impression that Venkman somehow > randomly impacts perf without *any* reason. I think you got the wrong impression, maybe I miscommunicated things. I fully understand that there are reasons for venkman doing what it does, I am just worried that people who are not using venkman at all or just not at the moment might have a visible perf penalty to pay, and I'm also quite worried about getting bad press because of including a feature that we surely find useful to include but is actually only used by a smaller fraction of our users. The ideal feature is invisible (in multiple ways) to those who don't need it, or at least has a very small profile for them. That said, sorry if what I said sounded harsh, being busy and overworked as well as someone who doesn't understand much of venkman and who has never used it himself and then being concerned about marketing are all factors that probably play together to make such statements come out in my communications. I fully understand your argument about limited time, I guess there's quite a few people around in our community that we'd love to have cloned so they can work multiple things at once ;-)

(In reply to comment #6) > Did you restart between those runs? That'd be required to test it properly... Well, is it really? Trying to make the /startup-init setting stick in SM 1.1.14(!) completely fails for me, it's always off after a restart. So I can't restart between test runs. My test ran like this: 1. Create a new profile and start it 2. Don't open Venkman 3. Run SunSpider: 26676 ms +/- 0.5% 4. Run SunSpider: 26434 ms +/- 0.7% 5. Open Venkman; /startup-init says it's "off" 6. Run SunSpider: 31357 ms +/- 0.3% 7. Run SunSpider: 31351 ms +/- 0.3% 8. Close Venkman 9. Run SunSpider: 24729 ms +/- 0.7% 10. Run SunSpider: 24837 ms +/- 1.3%

(In reply to comment #12) > (In reply to comment #6) > > Did you restart between those runs? That'd be required to test it properly... > > Well, is it really? > > Trying to make the /startup-init setting stick in SM 1.1.14(!) completely fails > for me, it's always off after a restart. So I can't restart between test runs. That's well weird. KaiRo doesn't seem to have had any problems turning it on and off. Are you passing it a toggle parameter? I mean, does it tell you it's "on" after you change it, and then you restart, and then it tells you it's back to "off"? Because that would be weird. :-)

(In reply to comment #13) > That's well weird. KaiRo doesn't seem to have had any problems turning it on > and off. Are you passing it a toggle parameter? I mean, does it tell you it's > "on" after you change it, and then you restart, and then it tells you it's back > to "off"? Because that would be weird. :-) I start SM, open Venkman, enter "/startup-init" and it says "off". I say "/startup-init on" and it says "on". I restart SM, open Venkman, enter "/startup-init" and it says "off"... I now killed all venkman and jsd stuff under SM1.1.14's /components and removed all respective entries in its compreg.dat and ran SunSpider: 24303 ms +/- 0.4% with closed Venkman, 24603 ms +/- 0.7% with open (but of course broken) Venkman, so I'm quite convinced that my numbers in comment 12 are consistent. I'm going to test that whole stuff in my debug trunk build now.

Heh, I have it say "on" all the time in 2.0a1pre :P Karsten, are you using the venkman version from 1.8.1 as I did, or are you using a newer one? If the latter, that might point to a venkman problem in newer versions, IMHO.

My SM 1.1.14 tests were all made with the "onboard" Venkman, which claims to be "Venkman version 0.9.83".

OK, good news in terms of finding the cause of the problem: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b4pre) Gecko/20090315 Lightning/1.0pre SeaMonkey/2.0b1pre venkman disabled: 1892.2ms +/- 12.2% venkman enabled: 2376.2ms +/- 13.8% venkman enabled, venkman-service.js line 570 changed to "jsds.initAtStartup = false;": 1889.6ms +/- 8.7% (note that I needed to delete compreg and the fastload files in the profile to make the venkman-service.js apply and venkman to really tell that /startup-init was off, as AFAIK XPC fastload caches the component code somehow) With the same change on the 1.1.16pre build, I get 14122.2ms +/- 4.8% - which also is some improvement to 15719.8ms +/- 1.5% This seems to indicate (to me) that jsds.initAtStartup is really the cause of the slowdown, and as this feature is known to probably have some overhead, I think we have the smoking gun we were looking for. It also looks reasonable that the percentage impact on 1.9.1-based builds is significantly higher than on 1.8.1-based ones, as the stuff it's doing probably take similar absolute time but with the rest of the engine having been sped up as much as it was, this gets much more visible. So, for one thing, we need to make /startup-init to be correctly remembered across restarts, and in addition, I think we really should try to find a solution that only impacts speed when a user actually wants to debug back to startup - IMHO, that's a good idea independent of the host application venkman is running in.

http://hg.mozilla.org/venkman/file/d119d68f96c7/js/venkman-service.js#l570 is the line I changed and inverted to false to actually make startup-init be disabled in SeaMonkey 2.0b1pre, by the way.

From bug 480765 comment #2, this shouldn't be hard to fix.

(In reply to comment #19) > From bug 480765 comment #2, this shouldn't be hard to fix. And from the other 19 comments on this bug, it's obvious that it's not that clear at all. Additionally, it's not clear from bug 480765 comment #2 in how many places Venkman would need to be adapted to take care of this, that could easily not be as straightforward as it may seem from that comment. If you still think it's trivial, feel free to post a patch, but be sure you take the effort Robert and the others have taken to fully test its effects to make sure it fixes the issue at hand, as well as verifying it doesn't break debugging (something which these benchmark tests obviously haven't done, which is not a problem in itself but would obviously have to be checked before any patches go in). Finally, I'd be uncomfortable at best with a solution which only works on trunk. Which further complicates matters.

(In reply to comment #17) > (note that I needed to delete compreg and the fastload files in the profile to > make the venkman-service.js apply and venkman to really tell that /startup-init > was off, as AFAIK XPC fastload caches the component code somehow) The line in question should only be run on XPCOM registration, so once for any Venkman install. Which would indeed suggest you'd need to remove those files. > This seems to indicate (to me) that jsds.initAtStartup is really the cause of > the slowdown, and as this feature is known to probably have some overhead, I > think we have the smoking gun we were looking for. It also looks reasonable > that the percentage impact on 1.9.1-based builds is significantly higher than > on 1.8.1-based ones, as the stuff it's doing probably take similar absolute > time but with the rest of the engine having been sped up as much as it was, > this gets much more visible. Right, that seems quite plausible. As a side issue, we should probably also look into if we can optimise this code some more, but that'd be icing on the cake for now... > So, for one thing, we need to make /startup-init to be correctly remembered > across restarts, and in addition, I think we really should try to find a > solution that only impacts speed when a user actually wants to debug back to > startup - IMHO, that's a good idea independent of the host application venkman > is running in. It's not so much about debugging "back to startup", it's about debugging files that were *loaded* (and maybe still are - definitely are for eg. browser/navigator.js and friends) before jsd is started. I'd bet that because of XUL Cache, there's actually no way you're going to get a JSDScript instance in the before-bug-480765 state of the world if you didn't watch the jsscript being created. You're absolutely right, of course, that we need to make sure the value is correctly remembered, and we should probably turn it off by default on SeaMonkey, and prompt to have it changed on first Venkman startup. So, to get things straight, Robert, for you on 1.1.16pre, things work in terms of remembering changes to it, and for Karsten they don't work on 1.1.14? Can you try each other's builds (ie Karsten trying 1.1.16pre and Robert 1.1.14), and/or actual clean profiles? Because I have absolutely no explanation at this point as to what could cause it to not be remembered correctly, and this is the biggest anomaly I can see in terms of reproducability, so hopefully it can give us a clue as to what's wrong...

(In reply to comment #21) > (In reply to comment #17) > > (note that I needed to delete compreg and the fastload files in the > > profile to make the venkman-service.js apply and venkman to really tell > > that /startup-init was off, as AFAIK XPC fastload caches the component > > code somehow) > > The line in question should only be run on XPCOM registration, so once for > any Venkman install. Which would indeed suggest you'd need to remove those > files. I debugged this tonight on trunk and found that Venkman is severely hit by bug 364864 - Neither AddCategoryEntry nor DeleteCategoryEntry states are persisted at all!

Sorry about the very late reply. (In reply to comment #22) > I debugged this tonight on trunk and found that Venkman is severely hit by bug > 364864 - Neither AddCategoryEntry nor DeleteCategoryEntry states are persisted > at all! That has nothing to do with bug 364864. I probably should have been more explicit in my blog post: the Venkman XPCOM component sets "jsds.initAtStartup = true" every time component registration runs. Consequently, /startup-init command persists only until the next component registration. (In reply to comment #1) > Is this the penalty imposed by turning Tracemonkey > off as soon as a debugger comes into play? Not sure whether Tracemonkey is really disabled when debugger service is switched on. However, the debugger service itself creates quite an overhead. This is mostly due to jsdscripts being created but as I already said in the other bug - I doubt SunSpider is even measuring this, it measures only execution time and not compilation time. In addition to this, debugger service executes some operations in function _interpreterTrace() for each JavaScript statement executed - this one and similar hooks should be creating the slowdown that SunSpider is measuring.

(In reply to comment #23) > > I debugged this tonight on trunk and found that Venkman is severely hit by > > bug 364864 - Neither AddCategoryEntry nor DeleteCategoryEntry states are > > persisted at all! > > That has nothing to do with bug 364864. "That" being what? > the Venkman XPCOM component sets "jsds.initAtStartup = true" every time > component registration runs. Consequently, /startup-init > command persists only until the next component registration. Sure, and that's already bug of itself. But what I mean in comment 22 is: it doesn't even persist until the next component registration but only until program shutdown - at least for me, under Linux.

(In reply to comment #23) > That has nothing to do with bug 364864. I probably should have been more > explicit in my blog post: the Venkman XPCOM component sets "jsds.initAtStartup > = true" every time component registration runs. Consequently, /startup-init > command persists only until the next component registration. Could it be that component registration for extensions is being run on every startup or so, effectively making this code run on every startup? In any case, I think venkman needs to store the /startup-init setting in a pref and make component registration set whatever the pref is set to (and just do nothing if the pref is not found, probably). This pref should IMHO default to off and venkman should put up a warning when it's being launched with it off and tell the user about this, what it implies, and that he can toggle it with /startup-init. If users are warned every time they launch venkman with this off, they should get well-educated about that feature, including to turn it off when they don't want its negative effects on normal non-debugging operation. And those who never use venkman don't feel an impact caused by it.

(In reply to comment #25) > Could it be that component registration for extensions is being run on every > startup or so, effectively making this code run on every startup? No. I specifically tested that (on trunk). > In any case, I think venkman needs to store the /startup-init setting in a > pref and make component registration set whatever the pref is set to (and > just do nothing if the pref is not found, probably). Unsure. Using a pref there will mean we have (at least) two places (prefs + catman) where we store this setting, which is bad. > This pref should IMHO default to off and venkman should put up a warning > when it's being launched with it off and tell the user about this, what it > implies, and that he can toggle it with /startup-init. IMO, that setting is too important to hide it behind the /startup-init switch, but that'd be a Venkman UI issue and is probably out of scope here.

(In reply to comment #26) > (In reply to comment #25) > > Could it be that component registration for extensions is being run on every > > startup or so, effectively making this code run on every startup? > > No. I specifically tested that (on trunk). Out of curiosity, how did you test that? Did you actually debug this, or is there some easier way that I'm missing? As for the /startup-init and so on... when JSD starts is not only a Venkman issue. We need to be mindful of the other add-ons that use it, and the fact that what you're really configuring is JSD. So I think it ought to be a JSD pref, which it should observe itself, so that no matter how it is changed (about:config / pref panel for Venkman/Firebug/Whatever), it always corresponds to what is actually going on. I would still argue that Venkman should turn it on automatically if it were off (which should be the default) *when it is explicitly installed by the user*. Loads of add-ons do first-run pages and so on, so I presume Venkman could use a similar system (post-install event that it can observe or something?). I am a bit confused about the category manager thing. Is there a workaround for bug 364864? From the looks of that bug it's not getting fixed soon, and it seems to me that as long as it doesn't work, we can't actually make sure we do/don't succesfully start on startup unless we start on startup and add/remove ourselves with the categorymanager at that point? I hope I'm missing something, because that seems like a bit of a Catch 22. Mook?

(In reply to comment #27) > I am a bit confused about the category manager thing. Is there a workaround for > bug 364864? From the looks of that bug it's not getting fixed soon, and it > seems to me that as long as it doesn't work, we can't actually make sure we > do/don't succesfully start on startup unless we start on startup and add/remove > ourselves with the categorymanager at that point? I hope I'm missing something, > because that seems like a bit of a Catch 22. Mook? No, you only get to poke it when you register new components. The only workaround is pretty much make whatever gets called by the category manager to disable itself instead (i.e. it's always called, but doesn't do anything when it ought to be disabled).

(In reply to comment #27) > So I think it ought to be a JSD > pref, which it should observe itself, so that no matter how it is changed > (about:config / pref panel for Venkman/Firebug/Whatever), it always corresponds > to what is actually going on. Hmm, JSD should already be remembering its state by itself, right? If not, it's it own problem to do that, I agree on that. > I would still argue that Venkman should turn it > on automatically if it were off (which should be the default) *when it is > explicitly installed by the user*. 1) I think exactly that is what we need a venkman pref for, and 2) I think this should NOT be "when it's installed" but "when the users decides to have it turned on", and venkman should warn the user on launch when it's not turned on, tell him why he wants to turn it on (and how he can turn it off again and why he might want to do that), and give him an easy way to actually do this turning on. When the user explicitely installs venkman (e.g. into Firefox), and he really wants to use it (right, why would he have installed it else), then he will launch it. And there he has his warning that he really wants this auto-turn-on activated that venkman can do. We'd tell him that debugging anything loaded before the venkman window only works with it, but that it degrades performance of the whole application somewhat. We'd give him a button or so to turn it on and tell him he obviously needs to restart for it to actually apply. We'd also tell him how to disable it again after debugging. You know, there might be people who want to debug something every now and then, but not deinstall and reinstall venkman for every one of those cases just so that it has no impact in between. And if he has deactivated it, we'll display the warning on venkman start again. What's so bad about this solution? (Actually, the easy on/off could even be a toolbar button in venkman, but not completely sure about that.)

(In reply to comment #29) > (In reply to comment #27) > > So I think it ought to be a JSD > > pref, which it should observe itself, so that no matter how it is changed > > (about:config / pref panel for Venkman/Firebug/Whatever), it always corresponds > > to what is actually going on. > > Hmm, JSD should already be remembering its state by itself, right? If not, it's > it own problem to do that, I agree on that. Well, that was the stuff about /startup-init not persisting... That's JSD not "remembering", because the category manager calls in http://mxr.mozilla.org/seamonkey/source/js/jsd/jsd_xpc.cpp#2377 are no-ops. And as Mook said in comment #28, there is currently no way around that. Which is a very big problem, and I haven't yet fully wrapped my head around how to fix that. The suggestion Mook gave might be helpful. It would hence always be registered in the category manager, and always field its startup call, but it would then be a no-op if the pref was set to false. In which case I'm not sure we actually need a pref observer, because that's not really going to do anything (the pref only gets used on the next startup, not before). > <snip> > > What's so bad about this solution? > > (Actually, the easy on/off could even be a toolbar button in venkman, but not > completely sure about that.) The bad thing is that we don't want the user to have to do anything. When the user explicitly installs it (rather than it coming with SeaMonkey), Venkman should automagically make sure it works. It should not surprise the user when it is started one or more sessions later by saying "actually, I don't work yet, click here to make me work, it'll restart your browser though!". That's absurd. That's what's wrong with your solution. I'd much prefer to automatically toggle this on install, point out to the user that we've done that, and give them the option to turn it off again then, rather than wait for them to actually launch Venkman (at which point scripts loaded earlier in their current session can't be debugged). If users are concerned about perf and installed the debugger for some reason other than to debug things (?!..) then they can turn it off straight away and restart, without any kind of dataloss or awkwardness - or perhaps uninstall Venkman again. I don't understand what the use of a toolbarbutton would be in this respect. It would only take effect after a restart. It'll be useless UI, in that sense. So I don't really understand why we should have primary UI for it. We can (no, should!) have UI when we change it without the user's ordering to do so, in order to alert the user to what we're doing, definitely. We can give them instructions on how to change it (we can just map the startup-init calls to a pref toggle instead) and that ought to be enough. Having a button which does basically nothing for the current session seems very counter-intuitive to me. Anyway, so we basically have three problems right now. 1. JSD is currently not listening to whatever the user tells him, which is arguably the worst of all. 2. Fixing Venkman to do the right thing to that JSD state, about which we apparently aren't quite in agreement yet (though I think we're getting there :-)). 3. Doing the followup work for bug 480765, so on trunk we can bypass the second point anyway. I think they ought to be fixed in this order. I will split off the second and third point into their own bugs now, and then this bug can focus on the first point. Once that is fixed, at least people who want it on or off can have JSD listen to them. I don't want to delay fixing that because we're still not agreed about UI and what Venkman should do to that JSD state. :-)

> I will split off the second and > third point into their own bugs now Uh, actually, it makes more sense to split off the first and third, cause this bug has been about the second one all along. Doing that instead. Sorry for spam.

(In reply to comment #30) > The bad thing is that we don't want the user to have to do anything. I'm not sure I agree fully here, but then I'm not a venkman user (yet, maybe I should finally learn it). I had that comment on my blog post (in which I actually wanted to highlight SpiderMonkey improvements, but commenters only cared about venkman, unfortunately): "Yep, goes for TB trunk also. Somewhere around 16% perf boost after disabling the extension. Who would have thought that an inactive extension could have this impact." That tells me that people who installed venkman *explicitely* also don't seem to be able to deal with the impact of having that debugging on when they don't even (intend to) run venkman and debug anything in that session. > We can (no, should!) have UI when we change it without the user's ordering to > do so, in order to alert the user to what we're doing, definitely. OK, so I agree here. And with that, dealing with the above case also would be easier. How are you supposed to bring up a notification to the user during component registration, though? Or do you want to drop some more stuff to be run on every application startup after opening windows? Dealing with that within existing venkman UI sounded easiest for me - and the additional benefit of warning the user when launching later again with the feature off also seems compelling to me. > 1. JSD is currently not listening to whatever the user tells him, which is > arguably the worst of all. When I tried, it seemed to work in SeaMonkey 1.1.x (for me), I'm not yet sure if the component registration call is actually the problem why it doesn't stick in my case. > 2. Fixing Venkman to do the right thing to that JSD state, about which we > apparently aren't quite in agreement yet (though I think we're getting there > :-)). True, I think we are getting there (and I think this might be the major issue here). :) > 3. Doing the followup work for bug 480765, so on trunk we can bypass the second > point anyway. I'm not sure I understand all the stuff there, but it sounds like it's helpful stuff ;-)

> > > Could it be that component registration for extensions is being run on > > > every startup or so, effectively making this code run on every startup? > > No. I specifically tested that (on trunk). > Out of curiosity, how did you test that? registerSelf in venkman-service.js already contains debug() calls which dump onto the shell in debug builds. Furthermore, I put a bunch of printf into jsd_xpc.cpp::GetInitAtStartup/SetInitAtStartup. The first run after recompiling will then dump/print this stuff on startup while subsequent runs will not. GetInitAtStartup/SetInitAtStartup are called when doing /startup-init on/off, of course, with SetInitAtStartup (de)registering the components relevant for startup observation. Sadly, as observed by Mook in bug 364864, the compreg changes are "usually" not saved afterwards. (I noticed a build system bug while testing this with my trunk/debug build: despite my --enable-chrome-format=symlink, Venkman's venkman-service.js still ends up being copied due to xpi staging. My SM 1.1.14 is an official build and as such not using symlinks, so this shouldn't matter wrt this bug.) > As for the /startup-init and so on... when JSD starts is not only a Venkman > issue. We need to be mindful of the other add-ons that use it, and the fact > that what you're really configuring is JSD. So I think it ought to be a JSD > pref, which it should observe itself, so that no matter how it is changed > (about:config / pref panel for Venkman/Firebug/Whatever), it always > corresponds to what is actually going on. jsdService::mInitAtStartup is inited to "unknown". It's only deduced from the actual catman component states when you read/write the initAtStartup attribute, which does *not* happen by JSD itself, but has to be done by debuggers. That's why venkman-service.js sets it explicitly. JSD does no registration etc. here, it just stuffs data into the registered components, if present. I'm not sure if having a JSD *pref* is useful here or where to put it. (Installing multiple debugger is probably no good idea, neither in the past, nor in future.) > I would still argue that Venkman should turn it > on automatically if it were off (which should be the default) *when it is > explicitly installed by the user*. Loads of add-ons do first-run pages and so > on, so I presume Venkman could use a similar system (post-install event that > it can observe or something?). In the 1.9 world, Venkman could provide its own default preferences file, setting this pref. (It's not so easy/nice for the 1.8 branch.) It should suffice to read this on compreg, assuming we get a fix for bug 364864 that simply invalides compreg.dat if the in-memory compreg is dirty. > I am a bit confused about the category manager thing. Is there a workaround > for bug 364864? From the looks of that bug it's not getting fixed soon, It shouldn't be very hard to fix, it seems just that (a) nobody uses catman the way Venkman does, thus (b) noone tried to fix it. AFAICS, there are two possible approaches in fixing 364864: - If the in-memory catman is dirty on app shutdown, rewrite compreg.dat. No need to run autocompreg, which might even reenable undesired categories. - Merely somehow "invalidating" compreg.dat and assume autocompreg will know about state changes (which it doesn't for Venkman atm). But that should be discussed in that bug. > it seems to me that as long as it doesn't work, we can't actually make sure > we do/don't succesfully start on startup unless we start on startup and > add/remove ourselves with the categorymanager at that point? Yes, that's how I see it.

What about users who install venkman for CONTENT page debugging (and hate firebug's UI). Or say a user is working on figuring out an a feature for one of his extensions at his MOTHERS house, installs venkman, but gets called away before he is even able to restart firefox. His mother restarts computer when it notifies her of the mandatory windows update restart, suddenly she has this addon. [which she won't use, and doesn't need] slowing down her system. Or said developer only needs it for a brief time, while majority of his time he plays games, or uses google maps, etc. I dont' see a good reason for enabling a perf impact on a _user_ 100% of the time, (even first run) without informing user there _will be_ a perf impact 100% of the time when the pref is flipped. By doing the dialog in Venkman on [venkman] startup, you solve all these cases. Either way, doing it per default in SeaMonkey would be bad (and will need to come up with some UI to inform seamonkey users anyway, and I don't think a split UI is your goal here).

(In reply to comment #2) > I'm included to agree with the assessment based on this, and we either need > this turned off by default (at --least-- for seamonkey) or venkman not > installed (I hear some users clammoring for Firebug/Chromebug instead of > Venkman anyway) Relacing Venkman by Firebug/Chromebug doesn't seem to be the solution, see http://adblockplus.org/blog/hidden-cost-of-not-using-venkman#c001704. Firebug has the same impact on performance, at least if you had Chromebug installed on your system once (it doesn't have to stay installed).

(In reply to comment #34) > By doing the dialog in Venkman on [venkman] startup, you solve all these cases. No, because the user would still enable the feature, even though it has perf impact, in order to actually have a fully working debugger, and then his mother's Firefox/SeaMonkey instance would still have this slowdown. I don't see how that solves any problems - it just forces more steps on the user that the add-on could have taken automatically. Not to mention how incredibly far-fetched this scenario is. Also, I have already stated at least 3 times (comment 31, comment 27, comment 21) that I have no intention of penalising SeaMonkey users by default. I have also said that there should be information on startup if Venkman automatically enables this pref on Firefox/Thunderbird/Flock/XULRunner (comment 31). So for goodness' sake, stop beating the strawman claiming I am (or anyone is) in favour of "enabling a perf impact ... without informing the user", or of duping SeaMonkey users not interested in debugging.

(In reply to comment #36) > (comment 31) I meant comment 30, of course. D'oh.

I agree with Gijs on that he has proposed a solution that is workable and will stop people having a penalty without even knowing it (something ChromeBug should look into as well, btw) - even if I still am not 100% in agreement with this proposed solution, let's see it first. What he proposes (and hopefully finds the time to work on as well, I fully understand being busy) is surely better than the current situation.

I've set jsd.initAtStartup=false for Firebug 1.4 and chromebug 0.5.

But that doesn't really change anything if you enable jsd explicitly at XPCOM startup...

I thought the problem here was users getting performance overhead because jsd.initAtStartup = true for all time whether they use venkman/firebug/chromebug or not (and they did not know). Since Firebug does not use initAtStartup and I don't think chromebug needs it, I turned it off to avoid this issue. Of course if they turn on jsd, then they get its cost and benefits, no doubt. For firebug 1.4+ that would happen on first use; for chromebug that will happen from start up.

The initAtStartup deal is this: Setting initAtStartup means effectively registering the jsdASObserver::Observe handler for app-startup, which then instantiates the jsdIDebuggerService, which then degrades perf. (In reply to comment #41) > Of course if they turn on jsd, then they get its cost and benefits, no doubt. Sure, and that's not the problem this bug's about. > For firebug 1.4+ that would happen on first use; for chromebug that will > happen from start up. Then Chromebug does have the same issue as Venkman, while Firebug as such has not.

(In reply to comment #42) > > For firebug 1.4+ that would happen on first use; for chromebug that will > > happen from start up. > > Then Chromebug does have the same issue as Venkman, > while Firebug as such has not. Just to clarify: Venkman sets initAtStartup = true; Chromebug (and Firebug) set initAtStartup = false. If the user puts -chromebug on the commmand line when they run a xul app, with chromebug installed, then chromebug will start jsd. Users will not be surprised, they caused the action.

> If the user puts -chromebug on the commmand line when they run a xul app, with > chromebug installed, then chromebug will start jsd. Is this early enough to catch everything relevant? (-venkman opens Venkman's main window, incidently starting jsd.) > Users will not be surprised, they caused the action. True.

(In reply to comment #44) > > If the user puts -chromebug on the commmand line when they run a xul app, with > > chromebug installed, then chromebug will start jsd. > > Is this early enough to catch everything relevant? > (-venkman opens Venkman's main window, incidently starting jsd.) Ah, now I am beginning to understand. To "catch everything", jsd needs to be started in app-startup. But that is before the command line processing, so a command line flag cannot control whether or not something runs in app-startup. Chromebug catches everything after command-line processing, but nothing before. I guess one can set the observer if -chromebug is given and remove it otherwise. That means the user has to restart Firefox to deal with app-start components.

Ok I verified that I can get 'all' of the app-start components now in Chromebug. I re-wrote the startup code, moving it out of the command line processor, and into an app-start observer. Then I named the app-start observer "@@@Chromebug Startup Observer Service". That puts it in the app-start list just in front of @maone.net/noscript-service;1. Then I can grab the noScript component code. (Unfortunately it looks like the executable line numbers are all out of whack, sigh). So Chromebug and Firebug do no set jsd.initAtStart. Firebug by itself should not cause startup jsd costs. You will still need to disable or uninstall Chromebug to avoid it's startup cost. Chromebug prints a message on the error console when it calls jsd.on() to warn users. Of course the only bit of this relevant to this bug is the potential for jsd.initAtStart from Firebug/chromebug messing with this effort.

Do we have any progress on the issues surrounding this bug, i.e. can we have venkman installed now without the debugger slowing down overall performance for people who don't even use venkman?

(In reply to comment #47) > Do we have any progress on the issues surrounding this bug, i.e. can we have > venkman installed now without the debugger slowing down overall performance for > people who don't even use venkman? I am not sure. I believe Mnyromyr's patch in bug 483681 (which has been checked in) allows SeaMonkey to set a pref so that is the case, although he would be best to ask about this (the bug is still open, and I am not sure why). Additionally, people who occasionally use Venkman (ergo toggling the pref back on) currently still pay a price, even for sessions where they do not do so. Fixing that is bug 483685, which is blocked by someone figuring out why JSD is not playing along. That someone would normally be me, except I am busy with my thesis and have very limited time to spend on Mozilla until after September (and after that, I am not sure :-) ).

You might be interested in Bug 506149. In short, once you turn jsd "on" you can't really turn it off; once jsd is on some code runs on every object creation.

I primarily care about the case where venkman was never activated in the session but is installed.

Can we please get a solution _very_ soon that makes users who never actually use venkman in SeaMonkey not see a perf impact caused by it? I'd really like to ship SeaMonkey 2.0 with the best perf we can offer, and our plans are to ship it in October after all.

As bug 364864 didn't make 1.9.1.4 but might have a good chance to make it in and hopefully enable us to solve this in a useful way for 1.9.1.5, I'll heavy-heartedly let this slip for 2.0 proper so we can push out the release itself, but I still think we really should fix this for 2.0.1, so moving the blocking flag to that one.

So because there is now a pref, I'd suggest SeaMonkey actually uses it, but I'm pretty sure that'd mean modifying SeaMonkey's all.js default prefs stuff. So that's a SeaMonkey issue. If that gets fixed, Venkman can re-enable it after the user enables it. I'd like to please keep this all on this bug (we're already using some 6-7 bugs to deal with the entire thing, some of them spurious), but for now, someone from the SeaMonkey side needs to take this and get the pref thing dealt with. Then I/we/Venkman will worry about how to re-enable the pref. I guess.

The code says: // - If extensions.venkman.jsd.initAtStartup is 0, we respect the current // running mode. // - If extensions.venkman.jsd.initAtStartup is 1, we enforce a running // jsdIDebuggerService. // - If extensions.venkman.jsd.initAtStartup is 2, we need to turn the // jsdIDebuggerService off. Which of those do we need by default in SeaMonkey so we don't trigger the slowdown for people not actively using venkman? Also, bug 483681 comment 5 says "As a sidenote: we still need to fix bug 364864" - I guess that's still true, right?

Moving blocking flag to next update, we didn't regress from the last version, so we can ship a security update. Still, progress here would be much appreciated. Karsten, what exactly needs to happen now to get a grip on this?

Hate to do this again, but forwarding to yet another update. Could we please get progress here?

Here we go again, pushing to yet another release.

Just another data point to illustrate: When I let my add-on render the basic Mandelbrot set with a normal SeaMonkey trunk build, it takes 4.8 seconds - with venkman disabled, it's 0.5 seconds. Can we please get something going here?

(In reply to comment #58) > Just another data point to illustrate: > When I let my add-on render the basic Mandelbrot set with a normal SeaMonkey > trunk build, it takes 4.8 seconds - with venkman disabled, it's 0.5 seconds. > > Can we please get something going here? initAtStartup is going away because of the new XPCOM registration things, and category manager entries won't be runtime-registerable. So, this issue will solve itself once the new XPCOM registration stuff lands (which, if I understand the newsgroup posts correctly, will be "soon").

Actually, I've put up patch for review in bug 480851 already last week, which would would allow to check in the catman fix again, which in turn would help fixing this one here. (In reply to comment #59) > initAtStartup is going away because of the new XPCOM registration things, and > category manager entries won't be runtime-registerable. Uh, what? Which bug is that? > So, this issue will solve itself once the new XPCOM registration stuff lands > (which, if I understand the newsgroup posts correctly, will be "soon"). Message-ID?

(In reply to comment #60) > Actually, I've put up patch for review in bug 480851 already last week, which > would would allow to check in the catman fix again, which in turn would help > fixing this one here. Sounds interesting. > (In reply to comment #59) > > initAtStartup is going away because of the new XPCOM registration things, and > > category manager entries won't be runtime-registerable. > > Uh, what? Which bug is that? https://bugzilla.mozilla.org/show_bug.cgi?id=568691 > > So, this issue will solve itself once the new XPCOM registration stuff lands > > (which, if I understand the newsgroup posts correctly, will be "soon"). > > Message-ID? <LL2dnX_dxMKty43RnZ2dnUVZ_vWdnZ2d@mozilla.org> and following.

I think I'm giving up on blocking any 2.0 release for this.

This should be fixed for 2.1a3, by the patch from bug 576869 if I understand correctly.