Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6357005:
Adding files for LiveObjectList implementation. (Closed)
|
Created:
9 years, 11 months ago by marklam Modified:
9 years, 7 months ago CC:
v8-dev Visibility:
Public. |
DescriptionAdding files for LiveObjectList implementation.
Committed: http://code.google.com/p/v8/source/detail?r=7012
Patch Set 1 #Patch Set 2 : '' #
Total comments: 73
Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #
Total comments: 52
Patch Set 6 : '' #
Messages
Total messages: 10 (0 generated)
On 2011/01/19 07:31:37, marklam wrote: I didn't managed to go through the full source yet. But I already have enough comments ;) I'll continue tomorrow.
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#new... src/liveobjectlist-inl.h:40: I think two blank lines are only needed between method definitions, classes declarations, etc. It is not needed after preprocessor directives. 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#newcod... src/liveobjectlist.cc:139: if (heap_obj->Is##type()) return TYPE_##type; 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. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:160: const char* GetObjectTypeDesc(HeapObject* heap_obj) { 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. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:183: #define INVALID_SPACE ((AllocationSpace)-1) Please, don't use #defines for constants. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:190: if (key_str[0] == 'c') { To me, switch statement looks more suitable than if for checking the first letter. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:216: bool result = I think, using for (int i = FIRST_SPACE; i <= LAST_SPACE; i++) loop would be more generic. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:247: inline bool IsActive() const { return is_active_; } is_active http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:263: : is_active_(false), type_(INVALID_LIVE_OBJ_TYPE), space_(INVALID_SPACE), init per line, please. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:266: if (filter_obj.is_null()) { Can be shortened to if (filter_obj.is_null()) return; http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:274: { MaybeObject* maybe_result = filter_obj->GetProperty(*type_sym); Put { on its own line, please. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:278: String* type_str = String::cast(type_obj); I'd factor out these 3 steps (if, if, cast) into a separate helper function. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:340: while (!elements_ && !Done()) { Please, use explicit elements_ != NULL check. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:372: if (curr_) { return curr_->obj_count_; } Either split this statement in 3 lines, or remove curly braces. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:391: int i_; I think, using a one-letter name for a class member isn't OK. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:456: // Generates a custome description based on the specific type of typo: custome http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:476: str->ToCString(DISALLOW_NULLS, ROBUST_STRING_TRAVERSAL, 0, 160); Should 160 be a constant derived from another one with value 80? http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:486: JSFunction* func =JSFunction::cast(obj); missing space after = http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:493: if (func_name->ToBoolean()->IsFalse()) { SharedFunctionInfo::DebugName() will do the work for you. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:559: { MaybeObject* maybe_result = Again please put { on its own line. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:637: first_lol = older_->next_; I'd write this as: LiveObjectList* first_lol = older_ != NULL ? older_->next_ : LiveObjectList::first_; http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:901: delete [] elements_; Please use DeleteArray http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:903: delete prev_; This check is redundant. The C++ language guarantees that delete p will do nothing if p is equal to NULL. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:955: return reinterpret_cast<int>(a->obj) - reinterpret_cast<int>(b->obj); This may not compile (and may be totally wrong) on x64. Please use explicit comparison. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:998: int i = result - lol->elements_; Please check whether this compiles on x64. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:1026: if (obj_count_ > 0) { I'd prefer using Vector::Sort here. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:1049: HeapIterator iterator; There is an issue with iterating heap objects, that some of them are actually free list elements. I think you should specify kFilterFreeListNodes here. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:1098: delete lol; // We'll start over with a bigger count. Will it be easier to capture contents the way that doesn't modify them (you can put AssertNoAllocation in a block)? 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. Hmm... It's already uncommented. Is this comment still makes sense? 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 object -> object id? 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 <lol id --> <lol id> http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode224 src/liveobjectlist.h:224: for (Object** p = start; p < end; p++) UpdatePointer(p); This is misaligned with ObjectVisitor declaration. It expresses VisitPointer using VisitPointers, while you are doing it opposite. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode234 src/liveobjectlist.h:234: HeapObject* heap_obj = reinterpret_cast<HeapObject *>(object); Using HeapObject::cast is preferred. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode251 src/liveobjectlist.h:251: LiveObjectList::NullifyNonLivePointer(reinterpret_cast<HeapObject**>(p)); Why not heap_obj? http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode264 src/liveobjectlist.h:264: static void IterateElements(ObjectVisitor* v) {} I think unused parameters should omit names.
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#new... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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#newcod... 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.
Replies to your answers. I will review the updated code later today. 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#newcod... src/liveobjectlist.cc:216: bool result = On 2011/01/21 03:47:49, marklam wrote: > 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? Heap::InSpace accepts addresses and doesn't assume that the address is in V8's heap. If it's true that !OS::IsOutsideAllocatedSpace implies that an address lies in one of the spaces, then your logic can be used, otherwise not. Soeren, can you tell anything about this? http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:274: { MaybeObject* maybe_result = filter_obj->GetProperty(*type_sym); On 2011/01/21 03:47:49, marklam wrote: > 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. OK, let's stick to the existing habit. Style guide says nothing about this. My own reasoning is that a block of code should be formatted like a code block of a statement, having a statement empty. E.g., as we format: if () { aaa; } we should write a block as { aaa; } http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.cc#newcod... src/liveobjectlist.cc:998: int i = result - lol->elements_; On 2011/01/21 03:47:49, marklam wrote: > 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. You should also try Windows. VS emits a warning: warning C4244: 'initializing' : conversion from '__int64' to 'int', possible loss of data And it's fatal, because we treat warnings as errors. I remember I had troubles when submitting your previous patch. 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#newcode251 src/liveobjectlist.h:251: LiveObjectList::NullifyNonLivePointer(reinterpret_cast<HeapObject**>(p)); On 2011/01/21 03:47:49, marklam wrote: > 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. Right, sorry I misread the code. http://codereview.chromium.org/6357005/diff/6001/src/liveobjectlist.h#newcode264 src/liveobjectlist.h:264: static void IterateElements(ObjectVisitor* v) {} On 2011/01/21 03:47:49, marklam wrote: > 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. OK, do as linter says.
On 2011/01/21 13:19:47, Michail Naganov wrote: > Heap::InSpace accepts addresses and doesn't assume that the address is in V8's > heap. If it's true that !OS::IsOutsideAllocatedSpace implies that an address > lies in one of the spaces, then your logic can be used, otherwise not. Soeren, > can you tell anything about this? Regardless of the implementation of OS::IsOutsideAllocatedSpace() which seems to assume that allocations are contiguously bounded by a start and end address, the rest of the implementation of Heap::InSpace() relies on the Contains() method. From the implementation of NewSpace and PagedSpace, the implementation of Contains() seem to be reliant on the tested address being a valid heap object address. In contrast, PagedSpace has a SafeContains() that "Never crashes even if a is not a valid pointer". So, my implementation is no less safe than the existing implementation.
I would also ask Soeren to look at this, because the patch is rather big. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc File src/liveobjectlist.cc (right): http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:979: return a->obj - b->obj; This will not work properly for x64. Please, use explicit comparison. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1384: if (older_id != 0) { I see this code fragment for the 4th time. Can you please factor it out? http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1458: LiveObjectType type = (LiveObjectType)i; reinterpret_cast http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1641: while (lol != NULL) { I think, this code has much in common with GetObj. Can you please factor the common part out? http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1757: if (lol_visitor.found()) { This fragment of code looks almost exactly as the previous one. Can you please factor out common code? http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1867: HeapObject* heap_obj = reinterpret_cast<HeapObject*>(GetObj(obj_id)); Please, use HeapObject::cast. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2292: obj1 = reinterpret_cast<HeapObject*>(GetObj(obj_id1)); Please, use HeapObject::cast. You'll need to check if it IsHeapObject first. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2298: HeapObject* obj2 = reinterpret_cast<HeapObject*>(GetObj(obj_id2)); ditto. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2345: Element* elements = new Element[total_count]; NewArray<Element> http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2406: delete[] elements; DeleteArray http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2447: i--; Personally, I prefer to write such loops as while-loops, incrementing counter when necessary, instead of undoing the work done by the for-loop increment. But it's not mandatory. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2455: delete[] elements; DeleteArray http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2485: new_elements = new Element[new_count]; NewArray<Element> And why don't you initialize during the declaration? http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2488: delete[] elements; DeleteArray http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2551: fflush(stdout); Flush() http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2552: HeapIterator iterator; should be iterator(HeapIterator::kFilterFreeListNodes) ? http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2561: Element* result = reinterpret_cast<Element *>( Element * -> Element* http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2569: result->obj = reinterpret_cast<HeapObject *>(heap_obj->address()); HeapObject * -> HeapObject* http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2598: delete[] elements; DeleteArray http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2650: for (it.Init(); !it.Done(); it.Next()) { just curious: why don't you declare the iterator in the for loop initialization part?
Hi Mark, A few general comments. I will provide more when your refactoring is uploaded. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc File src/liveobjectlist.cc (right): http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:231: } You might add an ASSERT or SLOW_ASSERT of 'Heap::InSpace(heap_obj, LO_SPACE)' here. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:381: if (elements_) break; elements_ != NULL http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:770: enum { Why use a enum here instead of static const int kNumberOfEntries = ...? http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1101: // in the interim. Please explain how more objects can get added to the heap during the call to Capture(). http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1149: Handle<JSObject> result = Factory::NewJSObject(Top::object_function()); What is supposed to happen if a lol is captured but we are not able to return a result object? http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1464: Why are you using mixed pointers/handles here - is that intentional? In handles.h/handles.cc there is a SetProperty function which takes handles and performs GC if required. As this function already uses Factory methods GC can happen, so handlifying the whole thing seems like the right thing to do. Other functions below uses a mixture of handles and raw objects.
Here is the update that addresses all the comments except for 3 issue left to be addressed in a future CL: 1. "rewind" Capture() if allocation fails. 2. refactor lol core and debugger/runtime parts into 2 components. 3. add unit tests. Please take a look. Thanks. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc File src/liveobjectlist.cc (right): http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:231: } On 2011/01/25 14:34:48, Søren Gjesse wrote: > You might add an ASSERT or SLOW_ASSERT of 'Heap::InSpace(heap_obj, LO_SPACE)' > here. Done. I added the SLOW_ASSERT. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:381: if (elements_) break; On 2011/01/25 14:34:48, Søren Gjesse wrote: > elements_ != NULL Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:770: enum { On 2011/01/25 14:34:48, Søren Gjesse wrote: > Why use a enum here instead of > > static const int kNumberOfEntries = ...? Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:979: return a->obj - b->obj; On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > This will not work properly for x64. Please, use explicit comparison. Done. Sorry that I misunderstood what you're asking for here. I've made the necessary change. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1101: // in the interim. On 2011/01/25 14:34:48, Søren Gjesse wrote: > Please explain how more objects can get added to the heap during the call to > Capture(). I've remove this retry and padding logic. It was added needed due to a previous temporary implementation of this code which could allocate objects. However, that need is no longer present. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1149: Handle<JSObject> result = Factory::NewJSObject(Top::object_function()); On 2011/01/25 14:34:48, Søren Gjesse wrote: > What is supposed to happen if a lol is captured but we are not able to return a > result object? Deferred fix for a future CL. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1384: if (older_id != 0) { On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > I see this code fragment for the 4th time. Can you please factor it out? Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1458: LiveObjectType type = (LiveObjectType)i; On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > reinterpret_cast Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1464: On 2011/01/25 14:34:48, Søren Gjesse wrote: > Why are you using mixed pointers/handles here - is that intentional? In > handles.h/handles.cc there is a SetProperty function which takes handles and > performs GC if required. > > As this function already uses Factory methods GC can happen, so handlifying the > whole thing seems like the right thing to do. > > Other functions below uses a mixture of handles and raw objects. Deferred fix for a future CL. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1641: while (lol != NULL) { On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > I think, this code has much in common with GetObj. Can you please factor the > common part out? Done. Templatized the common part. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1757: if (lol_visitor.found()) { On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > This fragment of code looks almost exactly as the previous one. Can you please > factor out common code? Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:1867: HeapObject* heap_obj = reinterpret_cast<HeapObject*>(GetObj(obj_id)); On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > Please, use HeapObject::cast. Done. I did a search for *> and replace all the cases that seemed appropriate with the respective cast() calls. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2292: obj1 = reinterpret_cast<HeapObject*>(GetObj(obj_id1)); On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > Please, use HeapObject::cast. You'll need to check if it IsHeapObject first. Done. GetObj() either returns undefined or a HeapObject. So, it should be fine. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2298: HeapObject* obj2 = reinterpret_cast<HeapObject*>(GetObj(obj_id2)); On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > ditto. Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2345: Element* elements = new Element[total_count]; On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > NewArray<Element> Done. I did a search for all new and delete[] in this file and replaced them accordingly. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2406: delete[] elements; On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > DeleteArray Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2447: i--; On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > Personally, I prefer to write such loops as while-loops, incrementing counter > when necessary, instead of undoing the work done by the for-loop increment. But > it's not mandatory. Done. Just to make you happy. =) http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2455: delete[] elements; On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > DeleteArray Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2485: new_elements = new Element[new_count]; On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > NewArray<Element> > And why don't you initialize during the declaration? Done. I think I had some debug / other experimental code in between before. That code is now purged, but these weren't recombined. I've re-combined them now. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2488: delete[] elements; On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > DeleteArray Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2551: fflush(stdout); On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > Flush() Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2552: HeapIterator iterator; On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > should be iterator(HeapIterator::kFilterFreeListNodes) ? Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2561: Element* result = reinterpret_cast<Element *>( On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > Element * -> Element* Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2569: result->obj = reinterpret_cast<HeapObject *>(heap_obj->address()); On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > HeapObject * -> HeapObject* Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2598: delete[] elements; On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > DeleteArray Done. http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... src/liveobjectlist.cc:2650: for (it.Init(); !it.Done(); it.Next()) { On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > just curious: why don't you declare the iterator in the for loop initialization > part? I suppose I could. But if I did that, I might as well declare the "int i = 0" in the loop initializer as well, and that will increase the line to 83 chars. It also makes the loop looks more complex and difficult to read. So, I left it this way mostly for readability. It's easy to see that it does at a glance.
On 2011/03/01 15:16:05, marklam wrote: > Here is the update that addresses all the comments except for 3 issue left to be > addressed in a future CL: > 1. "rewind" Capture() if allocation fails. > 2. refactor lol core and debugger/runtime parts into 2 components. > 3. add unit tests. > > Please take a look. Thanks. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc > File src/liveobjectlist.cc (right): > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:231: } > On 2011/01/25 14:34:48, Søren Gjesse wrote: > > You might add an ASSERT or SLOW_ASSERT of 'Heap::InSpace(heap_obj, LO_SPACE)' > > here. > > Done. I added the SLOW_ASSERT. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:381: if (elements_) break; > On 2011/01/25 14:34:48, Søren Gjesse wrote: > > elements_ != NULL > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:770: enum { > On 2011/01/25 14:34:48, Søren Gjesse wrote: > > Why use a enum here instead of > > > > static const int kNumberOfEntries = ...? > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:979: return a->obj - b->obj; > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > This will not work properly for x64. Please, use explicit comparison. > > Done. Sorry that I misunderstood what you're asking for here. I've made the > necessary change. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:1101: // in the interim. > On 2011/01/25 14:34:48, Søren Gjesse wrote: > > Please explain how more objects can get added to the heap during the call to > > Capture(). > > I've remove this retry and padding logic. It was added needed due to a previous > temporary implementation of this code which could allocate objects. However, > that need is no longer present. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:1149: Handle<JSObject> result = > Factory::NewJSObject(Top::object_function()); > On 2011/01/25 14:34:48, Søren Gjesse wrote: > > What is supposed to happen if a lol is captured but we are not able to return > a > > result object? > > Deferred fix for a future CL. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:1384: if (older_id != 0) { > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > I see this code fragment for the 4th time. Can you please factor it out? > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:1458: LiveObjectType type = (LiveObjectType)i; > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > reinterpret_cast > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:1464: > On 2011/01/25 14:34:48, Søren Gjesse wrote: > > Why are you using mixed pointers/handles here - is that intentional? In > > handles.h/handles.cc there is a SetProperty function which takes handles and > > performs GC if required. > > > > As this function already uses Factory methods GC can happen, so handlifying > the > > whole thing seems like the right thing to do. > > > > Other functions below uses a mixture of handles and raw objects. > > Deferred fix for a future CL. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:1641: while (lol != NULL) { > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > I think, this code has much in common with GetObj. Can you please factor the > > common part out? > > Done. Templatized the common part. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:1757: if (lol_visitor.found()) { > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > This fragment of code looks almost exactly as the previous one. Can you please > > factor out common code? > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:1867: HeapObject* heap_obj = > reinterpret_cast<HeapObject*>(GetObj(obj_id)); > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > Please, use HeapObject::cast. > > Done. I did a search for *> and replace all the cases that seemed appropriate > with the respective cast() calls. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2292: obj1 = > reinterpret_cast<HeapObject*>(GetObj(obj_id1)); > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > Please, use HeapObject::cast. You'll need to check if it IsHeapObject first. > > Done. GetObj() either returns undefined or a HeapObject. So, it should be > fine. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2298: HeapObject* obj2 = > reinterpret_cast<HeapObject*>(GetObj(obj_id2)); > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > ditto. > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2345: Element* elements = new Element[total_count]; > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > NewArray<Element> > > Done. I did a search for all new and delete[] in this file and replaced them > accordingly. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2406: delete[] elements; > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > DeleteArray > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2447: i--; > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > Personally, I prefer to write such loops as while-loops, incrementing counter > > when necessary, instead of undoing the work done by the for-loop increment. > But > > it's not mandatory. > > Done. Just to make you happy. =) > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2455: delete[] elements; > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > DeleteArray > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2485: new_elements = new Element[new_count]; > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > NewArray<Element> > > And why don't you initialize during the declaration? > > Done. I think I had some debug / other experimental code in between before. > That code is now purged, but these weren't recombined. I've re-combined them > now. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2488: delete[] elements; > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > DeleteArray > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2551: fflush(stdout); > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > Flush() > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2552: HeapIterator iterator; > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > should be iterator(HeapIterator::kFilterFreeListNodes) ? > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2561: Element* result = reinterpret_cast<Element *>( > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > Element * -> Element* > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2569: result->obj = reinterpret_cast<HeapObject > *>(heap_obj->address()); > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > HeapObject * -> HeapObject* > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2598: delete[] elements; > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > DeleteArray > > Done. > > http://codereview.chromium.org/6357005/diff/23001/src/liveobjectlist.cc#newco... > src/liveobjectlist.cc:2650: for (it.Init(); !it.Done(); it.Next()) { > On 2011/01/21 16:52:36, Mikhail Naganov (Chromium) wrote: > > just curious: why don't you declare the iterator in the for loop > initialization > > part? > > I suppose I could. But if I did that, I might as well declare the "int i = 0" > in the loop initializer as well, and that will increase the line to 83 chars. > It also makes the loop looks more complex and difficult to read. So, I left it > this way mostly for readability. It's easy to see that it does at a glance. Committed: http://code.google.com/p/v8/source/detail?r=7012 Thank you for the patch. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
