Make Object.freeze fast

Make Object.freeze fast

This patch both speeds up the freeze operation itself, but also
allows properties to remain in fast mode. Objects with non-empty
elements backing stores still end up with slow elements.

BUG=v8:1858, 115960
R=verwaest@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=14758

[adamk, 2013-05-16 01:07:44]

No tests yet: I'd be interested in thoughts on what tests I should be adding
(and how to craft those tests so they retain their value, i.e., they properly
force objects into various modes).

[Toon Verwaest, 2013-05-16 06:47:58]

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?

[adamk, 2013-05-16 21:39:41]

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...

[Toon Verwaest, 2013-05-16 22:46:38]

lgtm. Please wait with landing until the we've managed to roll.

[adamk, 2013-05-21 21:35:27]

On 2013/05/16 22:46:38, Toon Verwaest wrote:
> lgtm. Please wait with landing until the we've managed to roll.

Am I OK to land this?

[adamk, 2013-05-22 18:54:06]

Committed patchset #2 manually as r14758 (presubmit successful).