299788 - jsd_ObjectHook and jsd_FunctionCallHook popping up in profiles

Status: RESOLVED WORKSFORME

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

Description

[Boris Zbarsky [:bzbarsky]]

BUILD:  Mozilla build with --enable-jsd

STEPS TO REPRODUCE:  Run 003.html testcase at
http://www.hixie.ch/tests/adhoc/perf/video/ and profile it.

ACTUAL RESULTS:  jsd_ObjectHook and jsd_FunctionCallHook together take up over
4% of the total time.

EXPECTED RESULTS:  venkman hooks take no noticeable time unless venkman is
actually running.

I've seen these hooks coming up in "dhtml" profiles pretty consistently; is
there anything we can do about them?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looking at the methods:

jsd_FunctionCallHook does some checks (in _callHook, actually) to see whether
anything needs to be done, and bails out if not.  But it locks and unlocks
before it ever gets to that point, and this is what shows up in the profile. 
The exact breakdown of hits is (8206 total hits under jsd_FunctionCallHook, 1027
hits in the function itself):

  1027   8206 jsd_FunctionCallHook
               3120 jsd_Lock
               2198 jsd_Unlock
                474 PR_GetCurrentThread
                433 _callHook
                370 PR_Lock
                250 __i686.get_pc_thunk.bx
                247 PR_Unlock
                 87 _init

jsd_ObjectHook doesn't do any checks for whether anything needs to be done; it
just always does it's thing.  Breakdown here is:

  221  5142 jsd_ObjectHook
               1788 _createJSDObject
                880 _destroyJSDObject
                654 jsd_GetJSDObjectForJSObject
                536 jsd_Lock
                452 jsd_Unlock
                165 PR_GetCurrentThread
                160 JS_HashTableAdd
                109 __libc_calloc
                 45 PR_Unlock
                 43 PR_Lock
                 28 _init
                 26 __i686.get_pc_thunk.bx
                 25 free
                  7 JS_HashTableLookup
                  3 JS_HashTableRemove

Comment 2

[Brendan Eich [:brendan]]

I just mailed timeless, shaver, and mrbkap about this today.

/be

Comment 3

[Brendan Eich [:brendan]]

So instead of setting hooks at startup, and having the hooks test flags, why not
set no hooks till the condition that would set the relevant flag is true?

I'm not sure whether rginda is reading bugmail.  I'd like to see this fixed for
1.8, so I may need to take this bug -- but others feel free to beat me to it!

/be

Comment 4

[timeless]

debugging services that load before venkman can possibly init is already hit or
miss (believe me, it isn't fun).

my understanding is that to debug things that happen before the debugger gui
loads, certain account needs to be done during the normal sequence, now perhaps
it's possible for venkman to figure everything out that it needs to when it
inits, but it already takes about a minute for it to init here (closer to 10 w/
some of my more interesting builds)

as to venkman, there's actually supposedly a way to disable it for the next
startup. the code that plays with it is jsdService::GetInitAtStartup, i don't
quite remember the magic to make it follow the path you want, you can read it,
i'm going to sleep.

Comment 5

[Robert Ginda]

Venkman needs these hooks in place ahead of time in order to know what functions
are loaded.  AFAIK, there is no way to enumerate the list of known function
objects in a given runtime.  Without that list, you can only debug functions
that have been created since the Venkman UI started.

I'm reading bugmail, and will be around for comments, but I'm not going to have
time to actually hack on a fix.

Comment 6

[Mike Shaver (:shaver emeritus)]

As a hack, we could maybe add an jsdbgapi.h call that took the GC lock, scanned
the GC heap, and made a callback for every function object found.  Would that
suffice?  Do you need objects as well, for setting watchpoints?  Maybe we just
report all the non-primitives to you, and you can do with them what you will.

Comment 7

[Robert Ginda]

Functions are the most important things.  Internally, JSD might need to know
about objects, I don't recall at the moment.

JSD provides a way to locate the line of code that caused a particular object to
be instantiated.  I imagine that functionality depends on getting the "object
created" hook at a time where the the engine's state knows the current function
and PC.  That's a pretty cool feature, it'd be great to find a way to keep it.

Comment 8

[Mike Shaver (:shaver emeritus)]

That _is_ a cool feature.  (I have a thing on JS components that let you walk
the [[Parent]] chain to find a property indicating where the script was loaded
from, for better diagnostics on "object does not have a method named" XPConnect
errors.)

Would it suffice for that to only record objects that were created after venkman
loaded?  (Possibly quite early, if you registered for xpcom startup notification
when a pref was set, and unregistered when it was cleared.  You'd have to
restart after setting the pref, but if it's an early-in-start thing then you'd
need to restart anyway, I suspect.

Comment 9

[Robert Ginda]

I think that'd be a reasonable tradeoff.  Venkman already has a way to turn off
the hooks.  Rather than check a pref, it uses a startup observer that can be
registered or unregistered from the venkman command line.  /startup-init
(on|off), I think.  A pref would be easier to tweak, but checking the pref would
cost in precious startup time.

Comment 10

[Mike Shaver (:shaver emeritus)]

You wouldn't have to check the pref at startup; a pref observer would run the
startup-init code to add or remove the hook, I guess.

Or maybe this bug is fixed by having us default that to off, and the first run
of venkman could tell people that it had turned it on?

Comment 11

[timeless]

shaver: again, i spend a lot of my time debugging stuff that inits before a gui
happens, so if you trade off i'm hosed unless i can get back to where i am today.

Comment 12

[Mike Shaver (:shaver emeritus)]

_Again_, you'd just have to restart after setting the pref (and therefore the
startup entry), and then you'd be back to where you are today.  But the vast
majority of people who don't use venkman, or use it to debug web scripts,
wouldn't be paying the jsd_*Hook tax until/unless they opened the debugger.

Comment 13

[Brendan Eich [:brendan]]

This bug was filed to be fixed, not to be obstructed because someone who is not
among the tens of millions of Firefox users who will never, ever run Venkman
might need to set a pref to debug JS that runs extremely early in some embedding.

So who is going to fix it?

/be

Comment 14

[timeless]

Attachment: this should do it

Comment 15

[Robert Ginda]

I don't think "Debug Startup Files" quite says it.  The old label "Initialize at
Startup" didn't work out too well either.  How about "Install JS Engine Hooks",
or maybe the less accurate but happier "Install Browser Hooks".

Ideally, when this was off, we'd present a dialog like this at startup:

  In the current configuration, you can only debug scripts that are
  loaded AFTER the Venkman UI has been launched.  If you would like
  to be able to debug scripts that were loaded BEFORE starting the
  Venkman UI, you will need to enable the "Install Browser Hooks"
  option.  Doing so will make your browser slightly slower.  You can
  toggle this option from the Debug menu, or using the options below.
  Changes will take effect after restarting your browser.

    [ ] Don't ask me again.

    [ Yes, Install Browser Hooks ]  [ No Thanks ]

And for bonus points, we'd alert("You will need to restart your browser for this
change to take effect") whenever "Install Browser Hooks" changed from off to on.

Comment 16

[Mike Shaver (:shaver emeritus)]

Hang on a sec: Boris, do you have venkman installed when you got these profiles,
or just --enable-jsd set in the build?  The patch makes me think that you need
venkman installed to see the hook-at-startup behaviour, which isn't the
impression I got from the initial description.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I have --enable-jsd and I have --enable-extension=all.  I didn't do anything
special other than that.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Of course --enable-extensions=default also builds the venkman extension for some
of our apps...

Comment 19

[Robert Ginda]

Shaver, that's a great point.  The venkman-service.js component comes with the
Venkman extension, not with --enable-jsd.  So app start observer isn't
registered unless the Venkman UI is installed.

Comment 20

[:Gijs (he/him)]

initAtStartup was removed on trunk m-c a while ago, so I'm going to call this WFM.