341758 - Venkman doesn't really need to make linemaps of __toplevel__ scripts on startup

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

Firebug in effect only loads line mappings for scripts when it actually gets asked to do stuff to the script by the user. Venkman loads all scripts, and line maps them, on startup (and does it immediately for all new scripts). I/We think this is something that could be improved. The only concern is what happens to __toplevel__ things, and whether or not that is going to be a problem.

CC-ing Silver because he first talked about the issue. Copying some discussion off one of my wiki userpages to make sure things are findable at all:

====================

[this is only how I, Silver/James Ross, understand things]

SpiderMonkey (and Mozilla's use of it) wont keep scripts around forever, of course, but some script objects die very quickly. One of these is refered to in Venkman as __toplevel__, and is the script object for the global scope in a webpage (for example).

AIUI, it gets created first, when the <script> is seen/handled, and SpiderMonkey runs through it running the code and defining the functions. Once that's completed, it tends to go away. I don't know any details, this is just based on something someone (possibly Robert Ginda) said, or a comment in Venkman's code.

The issue is that once the SpiderMonkey script object has evaporated, you can't get any information from it (duh), which means you can't calculate what lines you could set breakpoints on (they'd have to be future breakpoints).

====================

Don't all breakpoints on top-level code in an already-loaded page have to be future breakpoints? Or does Venkman give a way of re-running that top-level code without reloading the page? -- shaver 

====================

Elaborating further on this (on IRC or on the bug) would probably help :-).

Comment 1

[James Ross]

I've added a reply on the wiki explaining.

Comment 2

[:Gijs (he/him)]

Profile URL, saved for history: http://wiki.mozilla.org/User:GijsKruitbosch/JS_Debugging/Profiling
=============

Now, thanks to some hard work by James Ross, we're fairly sure Venkman actually plays very nicely about being considerate in loading linemaps. That was the 'good' news. Bad news is that in the jsdScript constructor (see URL), it tries to figure out the starting PC and for that ends up calling JS_LineNumberToPC:
http://lxr.mozilla.org/seamonkey/source/js/jsd/jsd_scpt.c#480 http://lxr.mozilla.org/seamonkey/source/js/src/jsscript.c#1464


Unfortunately, if that gets passed 0 for a line number *I think* it ends up in a roughly worst case scenario because script->lineno will almost always be > 0, and the loop won't bail except for an exact match.

I *think* that mFirstPC = mScript->code; would do just as well in the jsdScript constructor - and be a hell of a lot faster. However, I'm not sure if I'm right as I'm really not as familiar with this code as some of the people I just CC-ed, and additionally most of the work right now has been done by James (again, kudos to him!)

So I'm morphing this bug and wondering if we can make jsd get its first PC that way (mFirstPC = mScript->code;), and/or if we'd need to add a new API for it to get to that. Brendan, Mike, what do you think?

Comment 3

[:Gijs (he/him)]

Short note on the above comment: it doesn't work out. It gave me roughly a 0.5% advantage on startup time, which could just as well be stupid fluctuations in other things (background programs, whatever).

===

Now. Back to the original issue we morph, now that poking about at various things, and James Ross (thanks *again* !) dumping js stacks out of cpp from windbg (slow...) for an entire evening, eventually the idea became: why not just not do a full linemap of __toplevel__ scripts on startup, because they're bound to only take future breakpoints, which you can set wherever you want anyway.

Which then gave me a startup time reduced by 31% on average, and the time spent in initDebugger reduced by 46%. This is on a fast machine (3.4Ghz Intel P4 HT, 1GB of RAM) and a clean profile, directly after the default Google homepage finishes loading. I'm *assuming* that the gains will be more noticeable on a slower machine with a profile that's not as blank as they come (as more pages/extensions (=scripts) loaded would make more difference in this specific case).

I'll attach a patch tomorrow and test on my (much older and slower) laptop tomorrow, see if I can get even cheerier perf notes from that.

Comment 4

[James Ross]

Uh, the reason I suggested not doing linemaps of __toplevel__ scripts on startup was not because Vnk doesn't need them! It was because if the __toplevel__ script survived from whenever it was made until Vnk startup, chances are good it'll still be around if the user opens the script - at which point we absolute should get the line map.

Comment 5

[:Gijs (he/him)]

Attachment: Patch

Patch adding a flag if we're enumerating on startup. I'm not sure if we could save looping the breaks here, too, so I didn't do that yet. We could just skip the entire addToLineMap call if possible, but I didn't want to take that risk without further discussion. So here we go.

One-off testing on my sloooow laptop shows a reduction from 7.799 to 4.224 secs for the initDebugger function, and a reduction from 12.382 to 8.849 secs overall, which is less than I hoped, I suppose. This is just a quick test using the -venkman startup flag (on an otherwise empty profile), so I guess YMMV. Either way, it's a pretty large tuck out of the general startup time, so I think it's definitely worth it. :-)

Comment 6

[James Ross]

Have you checked Vnk still loads the __toplevel__ line map when the scripts are opened? That is the key to this - I don't want it to just not load it for these scripts, but just not do it /while starting/.

Comment 7

[:Gijs (he/him)]

(In reply to comment #6)
> Have you checked Vnk still loads the __toplevel__ line map when the scripts are
> opened? That is the key to this - I don't want it to just not load it for these
> scripts, but just not do it /while starting/.

I have now, and it does load the tolpevel line map when you open the script in the source view.

Comment 8

[:Gijs (he/him)]

Checked in --> FIXED