Søren,
I've incorporated all the changes you've suggested that I could. I did not
break the lol runtime functions out into their own list separated from the
debugger because it doesn't make sense to do so (see my response to your
comment).
Regarding inspector.cc/h, I considered merging it with objects-printer.cc,
but
felt that it doesn't quite fit. I created inspector.cc/h initially for
holding
a collection of functions used for inspecting the VM. Most of those were
only
for debugging purposes not relevant to lols. The only part included here
is the
DumpObjectType() functions. Unlike those in object-printers.cc, these
dumper
functions are less like member functions of Object but more like external
functions that inspects the objects.
For example, it prints all the types that the object can be (including the
superclasses) and not only the specific type of the object. I felt that it
was
more appropriate to keep it in the inspector module.
If you feel strongly about merging it into objects-printer.cc, I can move it
over there. If not, I would rather it stay where it is. Thanks.
All these changes will be reflected in the divided up uploads I will be
making
in the near future.
http://codereview.chromium.org/6174007/diff/29002/SConstruct
File SConstruct (right):
http://codereview.chromium.org/6174007/diff/29002/SConstruct#newcode127
SConstruct:127: },
On 2011/01/13 22:25:53, Søren Gjesse wrote:
There should probably be support for this in the v8.gyp file as well.
Done. Not sure I did it right, but this will be included in my divided
up uploads.
http://codereview.chromium.org/6174007/diff/29002/src/SConscript
File src/SConscript (right):
http://codereview.chromium.org/6174007/diff/29002/src/SConscript#newcode84
src/SConscript:84: inspector.cc
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Add these files to v8.gyp as well.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/heap.cc
File src/heap.cc (right):
http://codereview.chromium.org/6174007/diff/29002/src/heap.cc#newcode902
src/heap.cc:902: class UpdateLiveObjectListVisitor: public ObjectVisitor
{
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Can this class move to the lol implementation files?
Done.
http://codereview.chromium.org/6174007/diff/29002/src/inspector.cc
File src/inspector.cc (right):
http://codereview.chromium.org/6174007/diff/29002/src/inspector.cc#newcode46
src/inspector.cc:46: fprintf(out, " size %d :", hobj->Size());
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Use OS::FPrint instead of fprintf.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/list-inl.h
File src/list-inl.h (right):
http://codereview.chromium.org/6174007/diff/29002/src/list-inl.h#newcode208
src/list-inl.h:208: }
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Two empty lines between functions.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/list.h
File src/list.h (right):
http://codereview.chromium.org/6174007/diff/29002/src/list.h#newcode165
src/list.h:165:
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Please add a comment on how this list works (e.g. it is not kept
sorted but
sorted when needed).
Done.
http://codereview.chromium.org/6174007/diff/29002/src/list.h#newcode167
src/list.h:167: class BSearchList: public List<T, P> {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
How about SearchableList instead of BSearchList?
Done.
http://codereview.chromium.org/6174007/diff/29002/src/list.h#newcode168
src/list.h:168: public:
On 2011/01/13 22:25:53, Søren Gjesse wrote:
No need for an empty line here.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/list.h#newcode170
src/list.h:170: BSearchList(int (*cmp)(const T* x, const T* y))
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Add explicit to constructors with one argument.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc
File src/liveobjectlist.cc (right):
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode156
src/liveobjectlist.cc:156: }
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Two empty lines between functions.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode164
src/liveobjectlist.cc:164: bool isOfType(LiveObjectType type, HeapObject
*obj) {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
All function names start with upper case letter.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode460
src/liveobjectlist.cc:460: snprintf(buffer, buffer_size, "%p <%s> len
%g",
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Please use OS::SNPrintF instead of snprintf.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode886
src/liveobjectlist.cc:886: } while (lol);
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Use != NULL when checking for NULL pointer.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode900
src/liveobjectlist.cc:900: if (prev() && prev()->Find(obj)) {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
prev() -> prev() != NULL
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode952
src/liveobjectlist.cc:952: while (lol) {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
!= NULL
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1001
src/liveobjectlist.cc:1001: while (lol) {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
!= NULL
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1026
src/liveobjectlist.cc:1026: int total_count = countHeapObjects();
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Start function name with upper case.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1031
src/liveobjectlist.cc:1031: if (last_lol) {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
!= NULL
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1047
src/liveobjectlist.cc:1047: if (!lol) {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Only indent by 2.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1111
src/liveobjectlist.cc:1111: while (lol) {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
!= NULL
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1119
src/liveobjectlist.cc:1119: if (!lol) return false;
On 2011/01/13 22:25:53, Søren Gjesse wrote:
== NULL
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1130
src/liveobjectlist.cc:1130: if (next) {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
!= NULL
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1136
src/liveobjectlist.cc:1136: if (prev) {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Ditto.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1221
src/liveobjectlist.cc:1221: // Calculate the number of entries of the
dump:
On 2011/01/13 22:25:53, Søren Gjesse wrote:
End comment with a period. More below.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1228
src/liveobjectlist.cc:1228: return Failure::Exception(); // invalid
start
On 2011/01/13 22:25:53, Søren Gjesse wrote:
2 space indent.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1300
src/liveobjectlist.cc:1300: if (newer_id) {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
!= NULL (more below).
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.cc#newcode1591
src/liveobjectlist.cc:1591: // Extract the address value from the
string:
On 2011/01/13 22:25:53, Søren Gjesse wrote:
There is a StringToInt in convertions.h.
Done. Changed to use the conversion function.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.h
File src/liveobjectlist.h (right):
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.h#newcode44
src/liveobjectlist.h:44: // #define VERIFY_LOL
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Maybe add the flag --verify-lol to flag-definitions.h instead.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/liveobjectlist.h#newcode53
src/liveobjectlist.h:53:
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Please add a short description of what lol is.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/mark-compact.cc
File src/mark-compact.cc (right):
http://codereview.chromium.org/6174007/diff/29002/src/mark-compact.cc#newcode1657
src/mark-compact.cc:1657: #ifdef LIVE_OBJECT_LIST
On 2011/01/13 22:25:53, Søren Gjesse wrote:
To reduce #ifdef/#endif I suggest making a LiveObjectList class with a
few empty
functions when LIVE_OBJECT_LIST is not defined (in liveobjectlist.h).
Done.
http://codereview.chromium.org/6174007/diff/29002/src/mark-compact.cc#newcode1904
src/mark-compact.cc:1904: if (LiveObjectList::NeedLOLProcessing()) {
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Can't the "if (LiveObjectList::NeedLOLProcessing())" check be moved
into
LiveObjectList::IterateElements()?
Some of the calls to LiveObjectList functions here is "guarded" by
this check
and some are not.
Done.
http://codereview.chromium.org/6174007/diff/29002/src/runtime.h
File src/runtime.h (right):
http://codereview.chromium.org/6174007/diff/29002/src/runtime.h#newcode372
src/runtime.h:372: /* LiveObjectList support*/ \
On 2011/01/13 22:25:53, Søren Gjesse wrote:
Shouldn't these functions be in a separate
RUNTIME_FUNCTION_LIST_LOL_SUPPORT
list, or should they always be there if ENABLE_DEBUGGER_SUPPORT is
defined?
I didn't break them out into a separate list because they do need to be
always there. I don't know of a way for the debugger to conditionally
include JS code right now. So, instead, each of the runtime functions
are always present when the debugger is enabled, but they will do
nothing except the first one which returns false to indicate that lol is
not enabled.
http://codereview.chromium.org/6174007/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
