Mikhail,
I've addressed all your comments. Please take another look and let me know
if
you disagree with my resolutions. Thanks.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist-inl.h
File src/liveobjectlist-inl.h (right):
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist-inl.h#newcode40
src/liveobjectlist-inl.h:40:
On 2011/01/20 16:48:00, Michail Naganov wrote:
I think two blank lines are only needed between method definitions,
classes
declarations, etc. It is not needed after preprocessor directives.
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc
File src/liveobjectlist.cc (right):
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode139
src/liveobjectlist.cc:139: if (heap_obj->Is##type()) return TYPE_##type;
On 2011/01/20 16:48:00, Michail Naganov wrote:
Is it OK for this check to have a linear complexity? How often object
types are
queried? If often, I think it should be implemented by retrieving
instance_type
from Map.
That may be a good idea. But I would like to get this committed as is
for now if that is ok with you. It has been tested and it works. I can
investigate switching to the instance_type from Map later and see if it
works for the lol use cases here (I added a TODO comment for that).
Thanks.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode160
src/liveobjectlist.cc:160: const char* GetObjectTypeDesc(HeapObject*
heap_obj) {
On 2011/01/20 16:48:00, Michail Naganov wrote:
If you often deal with type descriptions in a form of Strings, I
think, you can
register them as heap strings. In this case, string comparison will be
more
effective, as in most cases it will result in hashcodes or pointers
comparison.
The resultant string is not used for lookup except for in
SummarizePrivate() where I look up a symbol for it. I don't think it is
used in paths where performance is critical. And it is also used to
assemble C strings. So, I think we should leave it as is.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode183
src/liveobjectlist.cc:183: #define INVALID_SPACE ((AllocationSpace)-1)
On 2011/01/20 16:48:00, Michail Naganov wrote:
Please, don't use #defines for constants.
Done. Replaced with kInvalidSpace.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode190
src/liveobjectlist.cc:190: if (key_str[0] == 'c') {
On 2011/01/20 16:48:00, Michail Naganov wrote:
To me, switch statement looks more suitable than if for checking the
first
letter.
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode216
src/liveobjectlist.cc:216: bool result =
On 2011/01/20 16:48:00, Michail Naganov wrote:
I think, using for (int i = FIRST_SPACE; i <= LAST_SPACE; i++) loop
would be
more generic.
Done. This is still fragile in that the loop needs to exclude the
LO_SPACE. Technically, this InSpace implementation can be pushed back
into Heap::InSpace() because it speeds it up, and it also makes it less
fragile as anyone adding/removing spaces to/from the heap will be more
likely to see and adjust it if needed. What do you think?
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode247
src/liveobjectlist.cc:247: inline bool IsActive() const { return
is_active_; }
On 2011/01/20 16:48:00, Michail Naganov wrote:
is_active
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode263
src/liveobjectlist.cc:263: : is_active_(false),
type_(INVALID_LIVE_OBJ_TYPE), space_(INVALID_SPACE),
On 2011/01/20 16:48:00, Michail Naganov wrote:
init per line, please.
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode266
src/liveobjectlist.cc:266: if (filter_obj.is_null()) {
On 2011/01/20 16:48:00, Michail Naganov wrote:
Can be shortened to if (filter_obj.is_null()) return;
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode274
src/liveobjectlist.cc:274: { MaybeObject* maybe_result =
filter_obj->GetProperty(*type_sym);
On 2011/01/20 16:48:00, Michail Naganov wrote:
Put { on its own line, please.
This pattern came from existing code that uses MaybeObject return values
in this way, and appears to be the consistent way to format this. For
example, see stub-cache.cc, runtime.cc, objects.cc, objects-inl.h,
ic.cc, etc.
Do you want me to not conform to that?
Actually, it's a moot point. I moved this block out into a separate
helper function based on your suggestion below.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode278
src/liveobjectlist.cc:278: String* type_str = String::cast(type_obj);
On 2011/01/20 16:48:00, Michail Naganov wrote:
I'd factor out these 3 steps (if, if, cast) into a separate helper
function.
Done. I moved the whole { MaybeObhect ...} block out into a helper
function. Once I looked into refactoring it, it didn't seem to make
sense to only move part of the logic. Please let me know if my revised
code. Thanks.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode340
src/liveobjectlist.cc:340: while (!elements_ && !Done()) {
On 2011/01/20 16:48:00, Michail Naganov wrote:
Please, use explicit elements_ != NULL check.
Done. It should actually be elements_ == NULL.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode372
src/liveobjectlist.cc:372: if (curr_) { return curr_->obj_count_; }
On 2011/01/20 16:48:00, Michail Naganov wrote:
Either split this statement in 3 lines, or remove curly braces.
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode391
src/liveobjectlist.cc:391: int i_;
On 2011/01/20 16:48:00, Michail Naganov wrote:
I think, using a one-letter name for a class member isn't OK.
Done. Replaced with index_.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode456
src/liveobjectlist.cc:456: // Generates a custome description based on
the specific type of
On 2011/01/20 16:48:00, Michail Naganov wrote:
typo: custome
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode476
src/liveobjectlist.cc:476: str->ToCString(DISALLOW_NULLS,
ROBUST_STRING_TRAVERSAL, 0, 160);
On 2011/01/20 16:48:00, Michail Naganov wrote:
Should 160 be a constant derived from another one with value 80?
Done. Added constants to capture this.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode486
src/liveobjectlist.cc:486: JSFunction* func =JSFunction::cast(obj);
On 2011/01/20 16:48:00, Michail Naganov wrote:
missing space after =
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode493
src/liveobjectlist.cc:493: if (func_name->ToBoolean()->IsFalse()) {
On 2011/01/20 16:48:00, Michail Naganov wrote:
SharedFunctionInfo::DebugName() will do the work for you.
Done. Replaced this code.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode559
src/liveobjectlist.cc:559: { MaybeObject* maybe_result =
On 2011/01/20 16:48:00, Michail Naganov wrote:
Again please put { on its own line.
Again, this pattern came from existing code in stub-cache.cc,
runtime.cc, objects.cc, objects-inl.h, ic.cc, etc. Should I deviate
from the established convention?
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode637
src/liveobjectlist.cc:637: first_lol = older_->next_;
On 2011/01/20 16:48:00, Michail Naganov wrote:
I'd write this as:
LiveObjectList* first_lol = older_ != NULL ? older_->next_ :
LiveObjectList::first_;
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode901
src/liveobjectlist.cc:901: delete [] elements_;
On 2011/01/20 16:48:00, Michail Naganov wrote:
Please use DeleteArray
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode903
src/liveobjectlist.cc:903: delete prev_;
On 2011/01/20 16:48:00, Michail Naganov wrote:
This check is redundant. The C++ language guarantees that delete p
will do
nothing if p is equal to NULL.
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode955
src/liveobjectlist.cc:955: return reinterpret_cast<int>(a->obj) -
reinterpret_cast<int>(b->obj);
On 2011/01/20 16:48:00, Michail Naganov wrote:
This may not compile (and may be totally wrong) on x64. Please use
explicit
comparison.
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode998
src/liveobjectlist.cc:998: int i = result - lol->elements_;
On 2011/01/20 16:48:00, Michail Naganov wrote:
Please check whether this compiles on x64.
I just tried it. It seemed to compile fine on a linux x64 here.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode1026
src/liveobjectlist.cc:1026: if (obj_count_ > 0) {
On 2011/01/20 16:48:00, Michail Naganov wrote:
I'd prefer using Vector::Sort here.
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode1049
src/liveobjectlist.cc:1049: HeapIterator iterator;
On 2011/01/20 16:48:00, Michail Naganov wrote:
There is an issue with iterating heap objects, that some of them are
actually
free list elements. I think you should specify kFilterFreeListNodes
here.
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcode1098
src/liveobjectlist.cc:1098: delete lol; // We'll start over with a
bigger count.
On 2011/01/20 16:48:00, Michail Naganov wrote:
Will it be easier to capture contents the way that doesn't modify them
(you can
put AssertNoAllocation in a block)?
I agree with you. However, I recall that I added this retry mechanism
because there was some need for it. I think that this code could
resulted in a potential crash before the mechanism was added.
Regardless, it doesn't hurt if it's there. I would like to leave it in
for now. I added a TODO comment for myself to look into this some more.
I think removing this mechanism requires some lengthy testing to ensure
that there really is no opportunity for the heap object count to
increase.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h
File src/liveobjectlist.h (right):
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode44
src/liveobjectlist.h:44: // Uncomment the following to enable thorough
verification of lol data.
On 2011/01/20 16:48:00, Michail Naganov wrote:
Hmm... It's already uncommented. Is this comment still makes sense?
I've changed it to something that reads better.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode66
src/liveobjectlist.h:66: // The object is unique. Once assigned on an
object, an object id can never
On 2011/01/20 16:48:00, Michail Naganov wrote:
object -> object id?
Fixed.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode86
src/liveobjectlist.h:86: // Note: LOLs can be listed by calling Dump(0,
<lol id), and 2 LOLs can be
On 2011/01/20 16:48:00, Michail Naganov wrote:
<lol id --> <lol id>
Done.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode224
src/liveobjectlist.h:224: for (Object** p = start; p < end; p++)
UpdatePointer(p);
On 2011/01/20 16:48:00, Michail Naganov wrote:
This is misaligned with ObjectVisitor declaration. It expresses
VisitPointer
using VisitPointers, while you are doing it opposite.
I didn't do that. I was having both call UpdatePointer() directly. I
see no reason to do another virtual dispatch to VisitPointers() just to
call UpdatePointer().
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode234
src/liveobjectlist.h:234: HeapObject* heap_obj =
reinterpret_cast<HeapObject *>(object);
On 2011/01/20 16:48:00, Michail Naganov wrote:
Using HeapObject::cast is preferred.
Fixed.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode251
src/liveobjectlist.h:251:
LiveObjectList::NullifyNonLivePointer(reinterpret_cast<HeapObject**>(p));
On 2011/01/20 16:48:00, Michail Naganov wrote:
Why not heap_obj?
Because p points to the lol element that I need to nullify. I need the
address of the HeapObject*, not the HeapObject* value itself.
http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode264
src/liveobjectlist.h:264: static void IterateElements(ObjectVisitor* v)
{}
When I removed it, tools/presubmit.py says:
src/liveobjectlist.h:264: All parameters should be named in a function
[readability/function] [3]
src/liveobjectlist.h:265: All parameters should be named in a function
[readability/function] [3]
So, it looks like I should keep them.
http://codereview.chromium.org/6357005/
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
