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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 3 months ago by Toon Verwaest
Modified:
2 years, 3 months ago
Reviewers:
danno
CC:
v8-dev
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) Patch
M src/hydrogen.cc View 1 2 chunks +64 lines, -5 lines 0 comments Download
Trybot results:
Commit:

Messages

Total messages: 3 (0 generated)
Toon Verwaest
PTAL
2 years, 3 months ago (2012-11-08 13:47:06 UTC) #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 ...
2 years, 3 months ago (2012-11-13 22:52:32 UTC) #2
Toon Verwaest
2 years, 3 months ago (2012-11-14 11:57:38 UTC) #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 87e6a26