Remove prototype checks for leaf maps in optimized code.

Remove prototype checks for leaf maps in optimized code.

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

[ulan, 2013-02-08 14:10:05]

Please take a preliminary look, for now only ia32 arch.

This is based on https://chromiumcodereview.appspot.com/12224035/

Is lithium a good place to remove the check, or is it better to do on hydrogen
level by adjusting HCheckPrototypeMaps?

[Toon Verwaest, 2013-02-11 12:23:45]

Lithium is probably the least invasive way to remove checks (last-minute). For
now it is probably alright to keep it there if there's not too much code (so we
can easily move things when it becomes relevant).

Added some structural comments inline.

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/ia32/lithium-co...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/ia32/lithium-co...
src/ia32/lithium-codegen-ia32.cc:5338: if (maps->at(i)->is_dictionary_map())
can_ommit_checks = false;
This could should really go somewhere else; probably in the map. Add a utility
method Map::CanOmitPrototypeChecks or so.

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/objects.cc#newc...
src/objects.cc:5296:
DeoptimizeDependentCodes(DependentCodes::kPrototypeCheckGroup);
Placing this in CopyDropDescriptors is probably a bit too heavy-weight, given
that you only care about this when transitions are added. I presume you could
rather use the Map::AddTransition api as entry-point to deoptimize. It already
special-cases for the case that you are adding the first transition.

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/objects.cc#newc...
src/objects.cc:9581: }
I guess the if(!..) { } is not necessary; since after you go over it, it'll
always be set to true. I presume set_marked doesn't trigger extra computation
given that it's lowercase; but if it does, you could move the condition in that
method.

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/objects.cc#newc...
src/objects.cc:9582: }
I don't particularly like this API. It seems like we'd want to have an iterator
here. Also codes->Compute...(starts) seems a bit odd. Why not just
::GroupStartIndexes starts(codes) and let the constructor compute?

[Jakob Kummerow, 2013-02-11 13:27:00]

On 2013/02/11 12:23:45, Toon Verwaest wrote:
> Lithium is probably the least invasive way to remove checks (last-minute). For
> now it is probably alright to keep it there if there's not too much code (so
we
> can easily move things when it becomes relevant).

DBC: if the logic is going to be the same on all platforms, I would strongly
prefer to have it in some platform-independent place (such as Hydrogen). Happy
to discuss options/requirements/concerns offline.

[ulan, 2013-02-12 13:42:07]

Thanks for the comments. I uploaded new patch set.

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/ia32/lithium-co...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/ia32/lithium-co...
src/ia32/lithium-codegen-ia32.cc:5338: if (maps->at(i)->is_dictionary_map())
can_ommit_checks = false;
On 2013/02/11 12:23:45, Toon Verwaest wrote:
> This could should really go somewhere else; probably in the map. Add a utility
> method Map::CanOmitPrototypeChecks or so.
Done. After discussion with Jakob, I moved the loop above into a method of the
hydrogen instruction to make it platform independent.

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/objects.cc#newc...
src/objects.cc:5296:
DeoptimizeDependentCodes(DependentCodes::kPrototypeCheckGroup);
On 2013/02/11 12:23:45, Toon Verwaest wrote:
> Placing this in CopyDropDescriptors is probably a bit too heavy-weight, given
> that you only care about this when transitions are added. I presume you could
> rather use the Map::AddTransition api as entry-point to deoptimize. It already
> special-cases for the case that you are adding the first transition.
As discussed offline, adding abstraction function here to track object layout
changes that not necessarily lead to AddTransition.

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/objects.cc#newc...
src/objects.cc:9581: }
On 2013/02/11 12:23:45, Toon Verwaest wrote:
> I guess the if(!..) { } is not necessary; since after you go over it, it'll
> always be set to true. I presume set_marked doesn't trigger extra computation
> given that it's lowercase; but if it does, you could move the condition in
that
> method.

Done.

https://chromiumcodereview.appspot.com/12225099/diff/4001/src/objects.cc#newc...
src/objects.cc:9582: }
On 2013/02/11 12:23:45, Toon Verwaest wrote:
> I don't particularly like this API. It seems like we'd want to have an
iterator
> here. Also codes->Compute...(starts) seems a bit odd. Why not just
> ::GroupStartIndexes starts(codes) and let the constructor compute?

Done.

[Toon Verwaest, 2013-02-13 10:40:07]

Looking good. Some minor comments. Additionally, please disambiguate between
codes (encodings) and code (either code, code objects or code group).

https://chromiumcodereview.appspot.com/12225099/diff/6010/src/ia32/lithium-co...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/12225099/diff/6010/src/ia32/lithium-co...
src/ia32/lithium-codegen-ia32.cc:5342: }
Is there a reason you copy the maps list, rather than just storing the pointer?

https://chromiumcodereview.appspot.com/12225099/diff/6010/src/ia32/lithium-co...
src/ia32/lithium-codegen-ia32.cc:5343: __ LoadHeapObject(reg,
prototypes->at(prototypes->length() - 1));
Do all clients depend on this object being loaded? If not, we may want to look
into a way to remove this load as well. Perhaps in a different CL though.

https://chromiumcodereview.appspot.com/12225099/diff/6010/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/12225099/diff/6010/src/objects.h#newco...
src/objects.h:4710: // and depend on being deoptimized when map layout changes.
Group of code that omit run-time prototype checks for prototypes described by
this map. The group is deoptimized whenever an object described by this map
changes shape (and transitions to a new map), possibly invalidating the
assumptions embedded in the code.

[ulan, 2013-02-13 12:27:38]

Thanks, uploaded new patch set.

https://chromiumcodereview.appspot.com/12225099/diff/6010/src/ia32/lithium-co...
File src/ia32/lithium-codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/12225099/diff/6010/src/ia32/lithium-co...
src/ia32/lithium-codegen-ia32.cc:5342: }
On 2013/02/13 10:40:07, Toon Verwaest wrote:
> Is there a reason you copy the maps list, rather than just storing the
pointer?
Yep, the prototype_maps_ list collects maps from many instr->maps().

https://chromiumcodereview.appspot.com/12225099/diff/6010/src/ia32/lithium-co...
src/ia32/lithium-codegen-ia32.cc:5343: __ LoadHeapObject(reg,
prototypes->at(prototypes->length() - 1));
On 2013/02/13 10:40:07, Toon Verwaest wrote:
> Do all clients depend on this object being loaded? If not, we may want to look
> into a way to remove this load as well. Perhaps in a different CL though.
The result is the holder and it is used in all callsites.

https://chromiumcodereview.appspot.com/12225099/diff/6010/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/12225099/diff/6010/src/objects.h#newco...
src/objects.h:4710: // and depend on being deoptimized when map layout changes.
On 2013/02/13 10:40:07, Toon Verwaest wrote:
> Group of code that omit run-time prototype checks for prototypes described by
> this map. The group is deoptimized whenever an object described by this map
> changes shape (and transitions to a new map), possibly invalidating the
> assumptions embedded in the code.

Done.

[Toon Verwaest, 2013-02-13 15:18:25]

lgtm

https://chromiumcodereview.appspot.com/12225099/diff/14001/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/12225099/diff/14001/src/heap.cc#newcod...
src/heap.cc:2209: SKIP_WRITE_BARRIER);
reindent.

https://chromiumcodereview.appspot.com/12225099/diff/14001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/12225099/diff/14001/src/objects.h#newc...
src/objects.h:4706: // Group of codes that weakly embed this map and depend on
being
Group of code

[ulan, 2013-02-14 09:30:46]

I upload patch with other platforms.

https://chromiumcodereview.appspot.com/12225099/diff/14001/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/12225099/diff/14001/src/heap.cc#newcod...
src/heap.cc:2209: SKIP_WRITE_BARRIER);
On 2013/02/13 15:18:25, Toon Verwaest wrote:
> reindent.

Done.

https://chromiumcodereview.appspot.com/12225099/diff/14001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/12225099/diff/14001/src/objects.h#newc...
src/objects.h:4706: // Group of codes that weakly embed this map and depend on
being
On 2013/02/13 15:18:25, Toon Verwaest wrote:
> Group of code

Done.

[Toon Verwaest, 2013-02-14 10:30:32]

lgtm

[ulan, 2013-02-14 12:00:07]

Michael and Toon, please take another look. I merged
https://codereview.chromium.org/12224035/ into this CL.

[Michael Starzinger, 2013-02-14 12:44:04]

High-level comments: This leaves the HCheckPrototypeMaps in the graph and turns
LCheckPrototypeMaps into a constant load under some conditions. IMHO it would be
cleaner to actually replace the HCheckPrototypeMaps with an appropriate
HConstant (e.g. during canonicalization).

As discussed offline, this interferes with parallel recompilation, but I still
think it would be cleaner this way. I'll think about it some more.

https://codereview.chromium.org/12225099/diff/24002/src/mark-compact.cc
File src/mark-compact.cc (right):

https://codereview.chromium.org/12225099/diff/24002/src/mark-compact.cc#newco...
src/mark-compact.cc:2373: int number_of_codes =
starts.at(DependentCode::kGroupCount);
We should definitely add a number_of_codes() helper to GroupStartIndexes instead
of hard-coding the magic constant here.

https://codereview.chromium.org/12225099/diff/24002/src/mark-compact.cc#newco...
src/mark-compact.cc:2390: int number_of_codes =
starts.at(DependentCode::kGroupCount);
Likewise.

https://codereview.chromium.org/12225099/diff/24002/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/12225099/diff/24002/src/objects.h#newcode4716
src/objects.h:4716: };
Add an empty newline after this enum.

https://codereview.chromium.org/12225099/diff/24002/src/objects.h#newcode4726
src/objects.h:4726: };
Add and empty newline after this class.

https://codereview.chromium.org/12225099/diff/24002/src/objects.h#newcode4734
src/objects.h:4734: // and the mark compactor.
s/mark compactor/mark compact collector/

https://codereview.chromium.org/12225099/diff/24002/src/objects.h#newcode4980
src/objects.h:4980: // [dependent codes]: list of optimized codes that have this
map embedded.
The name in the comment is outdated.

[Toon Verwaest, 2013-02-14 12:56:46]

What about splitting up the prototype chain check into 2 different H
instructions. 1 that checks the chain; and 1 that actually loads the holder
(HConstant). This way we could fully eliminate the prototype chain check, and
can actually apply GVN and whatnot to the HConstant.

[Michael Starzinger, 2013-02-14 13:04:30]

I like Toon's idea of splitting the loading of the holder out of the
HCheckPrototypeMaps. I don't want to block this CL and could be convinced to
land it like this for now if we keep a TODO around with a semi-concrete plan on
how to clean this up in follow-up CLs.

[ulan, 2013-02-15 09:24:01]

Thanks, I agree that removing the prototype checks on hydrogen level would be
cleaner. I'll think about the parallel recompilation issue. For now I added TODO
in ia32 lithium-codegen.

https://chromiumcodereview.appspot.com/12225099/diff/24002/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/12225099/diff/24002/src/mark-compact.c...
src/mark-compact.cc:2373: int number_of_codes =
starts.at(DependentCode::kGroupCount);
On 2013/02/14 12:44:04, Michael Starzinger wrote:
> We should definitely add a number_of_codes() helper to GroupStartIndexes
instead
> of hard-coding the magic constant here.

Done.

https://chromiumcodereview.appspot.com/12225099/diff/24002/src/mark-compact.c...
src/mark-compact.cc:2390: int number_of_codes =
starts.at(DependentCode::kGroupCount);
On 2013/02/14 12:44:04, Michael Starzinger wrote:
> Likewise.

Done.

https://chromiumcodereview.appspot.com/12225099/diff/24002/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/12225099/diff/24002/src/objects.h#newc...
src/objects.h:4716: };
On 2013/02/14 12:44:04, Michael Starzinger wrote:
> Add an empty newline after this enum.

Done.

https://chromiumcodereview.appspot.com/12225099/diff/24002/src/objects.h#newc...
src/objects.h:4726: };
On 2013/02/14 12:44:04, Michael Starzinger wrote:
> Add and empty newline after this class.

Done.

https://chromiumcodereview.appspot.com/12225099/diff/24002/src/objects.h#newc...
src/objects.h:4734: // and the mark compactor.
On 2013/02/14 12:44:04, Michael Starzinger wrote:
> s/mark compactor/mark compact collector/

Done.

https://chromiumcodereview.appspot.com/12225099/diff/24002/src/objects.h#newc...
src/objects.h:4980: // [dependent codes]: list of optimized codes that have this
map embedded.
On 2013/02/14 12:44:04, Michael Starzinger wrote:
> The name in the comment is outdated.

Done.

[Michael Starzinger, 2013-02-20 10:12:26]

LGTM with one final comment.

https://codereview.chromium.org/12225099/diff/35001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/12225099/diff/35001/src/objects.h#newcode5213
src/objects.h:5213: 
Drop one of the two empty newlines.

https://codereview.chromium.org/12225099/diff/35001/src/objects.h#newcode5214
src/objects.h:5214: inline void NotifyObjectLayoutChange();
Could we rename this to "NotifyLeafMapLayoutChange()", because it only fires if
we don't have a transition. Also we definitely need a comment above this
declaration explaining when this notification fires.

[ulan, 2013-02-20 10:30:33]

Thank you! Landing.

https://chromiumcodereview.appspot.com/12225099/diff/35001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/12225099/diff/35001/src/objects.h#newc...
src/objects.h:5213: 
On 2013/02/20 10:12:26, Michael Starzinger wrote:
> Drop one of the two empty newlines.

Done.

https://chromiumcodereview.appspot.com/12225099/diff/35001/src/objects.h#newc...
src/objects.h:5214: inline void NotifyObjectLayoutChange();
On 2013/02/20 10:12:26, Michael Starzinger wrote:
> Could we rename this to "NotifyLeafMapLayoutChange()", because it only fires
if
> we don't have a transition. Also we definitely need a comment above this
> declaration explaining when this notification fires.

Done.

[ulan, 2013-02-20 11:10:19]

I added a flag and verification of an invariant that non-leaf maps have no
dependent code.

[Michael Starzinger, 2013-02-20 11:33:32]

Still LGTM.

[ulan, 2013-02-20 11:50:06]

Committed patchset #10 manually as r13697 (presubmit successful).