Addressed comments. Rebased. Landed.
https://chromiumcodereview.appspot.com/10381053/diff/1/src/mark-compact.cc File src/mark-compact.cc (left): https://chromiumcodereview.appspot.com/10381053/diff/1/src/mark-compact.cc#oldcode683 src/mark-compact.cc:683: if (collect_maps_) CreateBackPointers(); On 2012/05/08 14:29:19, Vyacheslav Egorov wrote:
side effect of always having back pointer and always treating it as a
strong
reference is that map trees will never be able to die from the root
(which could
previously happen when running V8 with --nocollect-maps).
We have seen obscure cases when this would lead to OOM.
Consider making back pointer weak when appropriate to fully mimic old --collect-maps behavior.
Yes, that is a good point. I will address this comment in the follow-up CL that enables map collection for incremental marking. Because it's much easier to do that once we don't have to special-case incremental marking anymore. https://chromiumcodereview.appspot.com/10381053/diff/1/src/objects.cc File src/objects.cc (right): https://chromiumcodereview.appspot.com/10381053/diff/1/src/objects.cc#newcode5172 src/objects.cc:5172: Object* header = *Header(); On 2012/05/08 14:29:19, Vyacheslav Egorov wrote:
is this change really required?
Yes it is, because this iterator overwrites the map of the prototype_transitions array to hold a SMI representing the iteration index. So here I need to check whether proto_trans_ actually is a valid FixedArray only if the map is still intact. I admit it's mighty confusing, so I tried to clean it up a bit. https://chromiumcodereview.appspot.com/10381053/diff/1/src/objects.cc#newcode5174 src/objects.cc:5174: proto_trans_->length() >= Map::kProtoTransitionHeaderSize); On 2012/05/08 14:29:19, Vyacheslav Egorov wrote:
should not this just be >?
Done. The length check is obsolete actually. The prototype transitions can never have an empty fixed array as a backing store, it's size is always > kProtoTransitionHeaderSize. https://chromiumcodereview.appspot.com/10381053/diff/1/src/objects.h File src/objects.h (right): https://chromiumcodereview.appspot.com/10381053/diff/1/src/objects.h#newcode4729 src/objects.h:4729: DECL_ACCESSORS(back_pointer, Object) On 2012/05/08 14:29:19, Vyacheslav Egorov wrote:
I would prefer if this was declared as GetBackPointer() and
SetBackPointer() to
show that this is not a trivial getter/setter.
Done. https://chromiumcodereview.appspot.com/10381053/diff/1/src/profile-generator.cc File src/profile-generator.cc (right): https://chromiumcodereview.appspot.com/10381053/diff/1/src/profile-generator.cc#newcode2155 src/profile-generator.cc:2155: SetInternalReference(map, entry, On 2012/05/08 14:29:19, Vyacheslav Egorov wrote:
You might also want to include back pointer as a strong reference
(which it is). Done. https://chromiumcodereview.appspot.com/10381053/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
