Chromium Code Reviews
Help | Chromium Project | Sign in
(270)

Issue 11359104: Consolidate polymorphic calls due to elements transitions. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by Toon Verwaest
Modified:
1 year, 5 months ago
Reviewers:
danno
CC:
v8-dev_googlegroups.com
Visibility:
Public.

Description

Consolidate polymorphic calls due to elements transitions.

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

Patch Set 1 : u #

Total comments: 6

Patch Set 2 : Addressed comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+64 lines, -5 lines) Lint Patch
M src/hydrogen.cc View 1 2 chunks +64 lines, -5 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 3
Toon Verwaest
PTAL
1 year, 5 months ago #1
danno
LGTM with nit http://codereview.chromium.org/11359104/diff/6001/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/11359104/diff/6001/src/hydrogen.cc#newcode7515 src/hydrogen.cc:7515: // Checks if all maps in ...
1 year, 5 months ago #2
Toon Verwaest
1 year, 5 months ago #3
Addressed comments.

https://chromiumcodereview.appspot.com/11359104/diff/6001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/11359104/diff/6001/src/hydrogen.cc#new...
src/hydrogen.cc:7515: // Checks if all maps in |types| are from the same family,
ie, are elements
On 2012/11/13 22:52:32, danno wrote:
> nit: i.e.

Done.

https://chromiumcodereview.appspot.com/11359104/diff/6001/src/hydrogen.cc#new...
src/hydrogen.cc:7519: static Map* CheckSameElementsFamily(SmallMapList* types) {
For now I'd keep it here. I doubt that he can reuse it after discussing with him
last week. This code is really optimized to check if a set of maps all belong to
the same family, he rather will have to check if a single maps belongs to the
family of another single map.

On 2012/11/13 22:52:32, danno wrote:
> Maybe this should be a static method on Map? I know that Michael might want to
> reuse some of this functionality?

https://chromiumcodereview.appspot.com/11359104/diff/6001/src/hydrogen.cc#new...
src/hydrogen.cc:7607: if (!monomorphic) {
On 2012/11/13 22:52:32, danno wrote:
> since you handle both cases, why not make it "if (monomorphic)" with the
> positive case first? 

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6