Thanks for the review. I've added some tests (there were already some basic
tests for freeze, so I just added more to that file).
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
On 2013/05/16 06:47:58, Toon Verwaest wrote:
This comment is a bit superfluous. It's true in general in V8: we
cannot just
modify maps directly; we have to transition.
Removed, and removed the similar one below.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5295
src/objects.cc:5295: // TODO(adamk): Extend the NormalizedMapCache to
handle non-extensible maps.
To expand on why I didn't add these to the NormalizedMapCache right now:
it seems like that cache is fairly minimal right now. It uses a basic
hash of the map, and if the existing cached one isn't equivalent, it
just gets overridden. And I'm worried I might slow down some important
case if I start shoving maps in there for this purpose. I'd be
interested in your thoughts on whether caching in this case could be bad
for existing caching scenarios.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5355
src/objects.cc:5355: bool can_transition = HasFastProperties() &&
map()->CanHaveMoreTransitions();
On 2013/05/16 06:47:58, Toon Verwaest wrote:
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() || (...)
Yeah, that's right semantically. It doesn't make any difference for the
uses of this bool currently, but will fix.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5357
src/objects.cc:5357: (!can_transition || elements() !=
heap->empty_fixed_array());
On 2013/05/16 06:47:58, Toon Verwaest wrote:
I'd use elements()->length() == 0 to be more resilient.
That'll actually be wrong for NON_STRICT_ARGUMENTS_ELEMENTS, I believe.
Which reminds me that should be another test-case, I'm not at all sure
this code will work with arguments (who knows why people would freeze
arguments).
Turns out this code is broken for arguments. My proposal for now is to
fall back to the existing implementation to avoid duplicating logic
about non-strict arguments elements backing stores. Let me know if you'd
rather I make this function handle arguments.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5369
src/objects.cc:5369: if (result.IsFound()) {
On 2013/05/16 06:47:58, Toon Verwaest wrote:
I'd use result.IsTransition() all over, since it more clearly
indicates your
intent.
Done.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5402
src/objects.cc:5402: if (!needs_element_normalization) {
On 2013/05/16 06:47:58, Toon Verwaest wrote:
You don't need to do this if result.IsTransition()
The elements kind will be correct in that case, but not the backing
store.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode5403
src/objects.cc:5403: map()->set_elements_kind(DICTIONARY_ELEMENTS);
On 2013/05/16 06:47:58, Toon Verwaest wrote:
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.
I've restructured this quite a lot, It was something of a mess. The
biggest change is that I no longer call NormalizeElements(), but instead
create my own SeededNumberDictionary with copies of the elements (I
extracted the copying code from NormalizeElements into a static helper).
This avoids thinking about the possible elements transition introduced
by NormalizeElements, and helps keep the rest of the code sane.
https://codereview.chromium.org/14888005/diff/1/src/objects.cc#newcode6656
src/objects.cc:6656: PropertyDetails details =
GetDetails(i).CopyWithAttributes(attributes);
On 2013/05/16 06:47:58, Toon Verwaest wrote:
Wouldn't this make, eg, non-enum fields enum by freezing them?
Clearly I misunderstood what BitField::update does (that name implied
additive behavior). Fixing...
https://codereview.chromium.org/14888005/
--
--
v8-dev mailing list
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
