Adding debugger interface and runtime functions hooks for supporting...

Adding debugger interface and runtime functions hooks for supporting
LiveObjectList functionality.

Committed: http://code.google.com/p/v8/source/detail?r=7011

[marklam, 2011-01-19 08:01:32]

Adding debugger interface and runtime functions hooks for supporting
LiveObjectList functionality.

[Søren Thygesen Gjesse, 2011-01-19 08:36:40]

http://codereview.chromium.org/6351007/diff/1/src/d8.js
File src/d8.js (right):

http://codereview.chromium.org/6351007/diff/1/src/d8.js#newcode592
src/d8.js:592: // Remember the handle requested in a global variable.
Maybe change handle to id here to keep the distinction between # and @ numbers.

http://codereview.chromium.org/6351007/diff/1/src/debug-debugger.js
File src/debug-debugger.js (right):

http://codereview.chromium.org/6351007/diff/1/src/debug-debugger.js#newcode2053
src/debug-debugger.js:2053: response.body = MakeMirror(%GetLOLObj(obj_id));
I think we should be able to unify the handles ids used by the debugger and the
object ids used in lols.

What do you think about this:

If lol is enabled then the debugger should just use the lol id instead of
generating its own handle id. That will make the ids persistent between debugger
invocations as long as lol is activated. That way we don't need both the @ and
the # ids.

This unification can be done in a separate change it it can work.

http://codereview.chromium.org/6351007/diff/1/src/debug-debugger.js#newcode2387
src/debug-debugger.js:2387: DebugCommandProcessor.prototype.lolCaptureRequest_ =
function(request, response) {
We try to split the long lines when possible, e.g.
JSONProtocolSerializer.prototype.serializeReferenceWithDisplayData_ and
JSONProtocolSerializer.prototype.serialize_ in mirror-debugger.js

http://codereview.chromium.org/6351007/diff/1/src/debug-debugger.js#newcode2392
src/debug-debugger.js:2392: DebugCommandProcessor.prototype.lolDeleteRequest_ =
function(request, response) {
Ditto.

http://codereview.chromium.org/6351007/diff/1/src/debug-debugger.js#newcode2437
src/debug-debugger.js:2437: DebugCommandProcessor.prototype.lolRetainersRequest_
= function(request, response) {
Ditto.

http://codereview.chromium.org/6351007/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6351007/diff/1/src/runtime.cc#newcode45
src/runtime.cc:45: #include "liveobjectlist.h"
This suggests that the actual lol implementation lands before this change. Or
should we have an "empty" class LiveObjectList in liveobjects.h added to this
change?

[marklam, 2011-01-19 09:04:02]

I'll prepare stubbed out lol headers in a separate CL (coming soon).

http://codereview.chromium.org/6351007/diff/1/src/d8.js
File src/d8.js (right):

http://codereview.chromium.org/6351007/diff/1/src/d8.js#newcode592
src/d8.js:592: // Remember the handle requested in a global variable.
On 2011/01/19 08:36:40, Søren Gjesse wrote:
> Maybe change handle to id here to keep the distinction between # and @
numbers.

This comment is a cut-and-paste error from the # case.  I removed it.

http://codereview.chromium.org/6351007/diff/1/src/debug-debugger.js
File src/debug-debugger.js (right):

http://codereview.chromium.org/6351007/diff/1/src/debug-debugger.js#newcode2053
src/debug-debugger.js:2053: response.body = MakeMirror(%GetLOLObj(obj_id));
On 2011/01/19 08:36:40, Søren Gjesse wrote:
> I think we should be able to unify the handles ids used by the debugger and
the
> object ids used in lols.
> 
> What do you think about this:
> 
> If lol is enabled then the debugger should just use the lol id instead of
> generating its own handle id. That will make the ids persistent between
debugger
> invocations as long as lol is activated. That way we don't need both the @ and
> the # ids.
> 
> This unification can be done in a separate change it it can work.

The problem with that is that the @ids only work if one has captured a lol. 
Whereas the #handle# can be used whenever the debugger breaks and the user
inspects a var.  If we unify it, then we need to fore the capture of lols every
time the user inspects a var in the debugger.  This will interfere with the
user's usage of lols because the debugger will be automatically capturing the
lols where the user didn't request any.

If we try to hide these implicitly captured lols, there are other issues with
the deletion of lols where the user may delete all lols, but we may not want to
delete the implicit ones.

Also, it introduces inconsistencies in the behavior of the merged #handles# when
lols are not in use.  @ids do not keep the GC from collecting an object, while
#handle#s do.  Hence, the user will be getting a different experience depending
on whether lols are in use or not.  This can be confusing to the user.

I think it is better to be consistent and transparent about the difference
between the 2.

http://codereview.chromium.org/6351007/diff/1/src/debug-debugger.js#newcode2387
src/debug-debugger.js:2387: DebugCommandProcessor.prototype.lolCaptureRequest_ =
function(request, response) {
On 2011/01/19 08:36:40, Søren Gjesse wrote:
> We try to split the long lines when possible, e.g.
> JSONProtocolSerializer.prototype.serializeReferenceWithDisplayData_ and
> JSONProtocolSerializer.prototype.serialize_ in mirror-debugger.js

Done.

http://codereview.chromium.org/6351007/diff/1/src/debug-debugger.js#newcode2392
src/debug-debugger.js:2392: DebugCommandProcessor.prototype.lolDeleteRequest_ =
function(request, response) {
On 2011/01/19 08:36:40, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/6351007/diff/1/src/debug-debugger.js#newcode2437
src/debug-debugger.js:2437: DebugCommandProcessor.prototype.lolRetainersRequest_
= function(request, response) {
On 2011/01/19 08:36:40, Søren Gjesse wrote:
> Ditto.

Done.

http://codereview.chromium.org/6351007/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6351007/diff/1/src/runtime.cc#newcode45
src/runtime.cc:45: #include "liveobjectlist.h"
On 2011/01/19 08:36:40, Søren Gjesse wrote:
> This suggests that the actual lol implementation lands before this change. Or
> should we have an "empty" class LiveObjectList in liveobjects.h added to this
> change?

Mikhail's suggested division of the commits was to land the liveobjectlist files
first.  If you want to land the liveobjectlist files last, I can prepare another
CL with stubbed out liveobjectlist.h and -inl.h.

[Søren Thygesen Gjesse, 2011-01-25 12:44:15]

Sorry for not getting back before. Now that the empty implementation is there we
should cut down on the conditional compilation.

http://codereview.chromium.org/6351007/diff/9001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6351007/diff/9001/src/runtime.cc#newcode10479
src/runtime.cc:10479: // Captures a live object list from the present heap.
With the stubbed-out implementation of LiveObjectList I don't think we need
#ifdef/#else/#endif in here anymore.

http://codereview.chromium.org/6351007/diff/9001/src/runtime.cc#newcode10481
src/runtime.cc:10481: #ifdef LIVE_OBJECT_LIST
How about adding LiveObjectList::Enabled() returning Heap::false_value() in the
empty implementation?

[marklam, 2011-03-01 15:23:12]

I've addressed Søren's comments below, and would like to defer the requested
changes as part of the debugger/runtime vs lol core isolation that I'll submit
in a later CL.

The only change in the code since last time is the #include of
liveobjectlist-inl.h instead of liveobjectlist.h.

Please take a look.  Thanks.

http://codereview.chromium.org/6351007/diff/9001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6351007/diff/9001/src/runtime.cc#newcode10479
src/runtime.cc:10479: // Captures a live object list from the present heap.
On 2011/01/25 12:44:15, Søren Gjesse wrote:
> With the stubbed-out implementation of LiveObjectList I don't think we need
> #ifdef/#else/#endif in here anymore.

I was hoping to not incur any footprint at all for code space (though admittedly
tiny) when lol is not built in.  In the functions below this one, there is code
needed for marshalling args as well that is not removed by current stubbed
implementations.

If it's ok, I can move all the code out of here into another module later when I
do the separation of core lol code from the lol debugger/runtime interfaces. 
That would be a natural time to abstract these out.

http://codereview.chromium.org/6351007/diff/9001/src/runtime.cc#newcode10481
src/runtime.cc:10481: #ifdef LIVE_OBJECT_LIST
On 2011/01/25 12:44:15, Søren Gjesse wrote:
> How about adding LiveObjectList::Enabled() returning Heap::false_value() in
the
> empty implementation?

Same as above (for consistency).

[Søren Thygesen Gjesse, 2011-03-02 09:17:18]

On 2011/03/01 15:23:12, marklam wrote:
> I've addressed Søren's comments below, and would like to defer the requested
> changes as part of the debugger/runtime vs lol core isolation that I'll submit
> in a later CL.
> 
> The only change in the code since last time is the #include of
> liveobjectlist-inl.h instead of liveobjectlist.h.
> 
> Please take a look.  Thanks.
> 
> http://codereview.chromium.org/6351007/diff/9001/src/runtime.cc
> File src/runtime.cc (right):
> 
> http://codereview.chromium.org/6351007/diff/9001/src/runtime.cc#newcode10479
> src/runtime.cc:10479: // Captures a live object list from the present heap.
> On 2011/01/25 12:44:15, Søren Gjesse wrote:
> > With the stubbed-out implementation of LiveObjectList I don't think we need
> > #ifdef/#else/#endif in here anymore.
> 
> I was hoping to not incur any footprint at all for code space (though
admittedly
> tiny) when lol is not built in.  In the functions below this one, there is
code
> needed for marshalling args as well that is not removed by current stubbed
> implementations.
> 
> If it's ok, I can move all the code out of here into another module later when
I
> do the separation of core lol code from the lol debugger/runtime interfaces. 
> That would be a natural time to abstract these out.
> 
> http://codereview.chromium.org/6351007/diff/9001/src/runtime.cc#newcode10481
> src/runtime.cc:10481: #ifdef LIVE_OBJECT_LIST
> On 2011/01/25 12:44:15, Søren Gjesse wrote:
> > How about adding LiveObjectList::Enabled() returning Heap::false_value() in
> the
> > empty implementation?
> 
> Same as above (for consistency).

Committed: http://code.google.com/p/v8/source/detail?r=7011

Thank you for the patch.