First round of comments. I especially like the CopyReplaceDescriptors
changes.
Some (simple) tests I can think of:
- Objects in fast mode should stay in fast mode if they don't have
normalized
properties.
- Freezing of objects does the right thing (maybe already tested?)
- Two fast objects with the same map should end up with the same map after
being
frozen (%HaveSameMap).
- And especially: ensure that freezing doesn't muck with existing
attributes in
both modes. (Currently broken I think).
https://codereview.chromium.org/14888005/diff/1/src/objects.cc
File src/objects.cc (right):
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5293
src/objects.cc:5293: // Do a map transition, other objects with this map
may still
This comment is a bit superfluous. It's true in general in V8: we cannot
just modify maps directly; we have to transition.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5355
src/objects.cc:5355: bool can_transition = HasFastProperties() &&
map()->CanHaveMoreTransitions();
If we already have a transition, we don't need to be able to add one
(and we are guaranteed to already be in fast mode).
So result->IsTransition() || (...)
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5357
src/objects.cc:5357: (!can_transition || elements() !=
heap->empty_fixed_array());
I'd use elements()->length() == 0 to be more resilient.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5369
src/objects.cc:5369: if (result.IsFound()) {
I'd use result.IsTransition() all over, since it more clearly indicates
your intent.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5402
src/objects.cc:5402: if (!needs_element_normalization) {
You don't need to do this if result.IsTransition()
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5403
src/objects.cc:5403: map()->set_elements_kind(DICTIONARY_ELEMENTS);
I'd move this map()->set_elements_kind down with the other map modifiers
(even if it also triggers when it was already normalized before).
Additionally I'd put those 3 assignments in a !result.IsTransition()
branch. That way we guarantee that we never muck with existing maps, but
only with the map we just created. Either that; or just duplicate those
map-assignments to where the maps are actually created.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode6439
src/objects.cc:6439: SimpleTransitionFlag simple_flag) {
Nice; this makes everything so much cleaner.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode6656
src/objects.cc:6656: PropertyDetails details =
GetDetails(i).CopyWithAttributes(attributes);
Wouldn't this make, eg, non-enum fields enum by freezing them?
https://codereview.chromium.org/14888005/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
