Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(453)

Issue 1195823002: Add fast path for setting length (Closed)

Created:
5 years, 6 months ago by Toon Verwaest
Modified:
5 years, 6 months ago
Reviewers:
Jakob Kummerow
CC:
v8-dev
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

Add fast path for setting array.length BUG= Committed: https://crrev.com/a803254413faf817785c9fc86db89c5d858223de Cr-Commit-Position: refs/heads/master@{#29154}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+46 lines, -46 lines) Patch
M src/accessors.h View 1 chunk +0 lines, -5 lines 0 comments Download
M src/accessors.cc View 1 2 2 chunks +46 lines, -41 lines 2 comments Download

Messages

Total messages: 9 (2 generated)
Toon Verwaest
ptal
5 years, 6 months ago (2015-06-19 14:08:48 UTC) #2
Jakob Kummerow
lgtm
5 years, 6 months ago (2015-06-19 14:56:03 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1195823002/60001
5 years, 6 months ago (2015-06-19 15:01:12 UTC) #5
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 6 months ago (2015-06-19 15:27:46 UTC) #6
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/a803254413faf817785c9fc86db89c5d858223de Cr-Commit-Position: refs/heads/master@{#29154}
5 years, 6 months ago (2015-06-19 15:27:59 UTC) #7
Toon Verwaest
https://codereview.chromium.org/1195823002/diff/60001/src/accessors.cc File src/accessors.cc (right): https://codereview.chromium.org/1195823002/diff/60001/src/accessors.cc#newcode205 src/accessors.cc:205: return wrapper->value()->ToArrayIndex(length); I think this is actually wrong... Number.prototype.valueOf ...
5 years, 6 months ago (2015-06-19 20:48:57 UTC) #8
Jakob Kummerow
5 years, 6 months ago (2015-06-20 11:26:38 UTC) #9
Message was sent while issue was closed.
https://codereview.chromium.org/1195823002/diff/60001/src/accessors.cc
File src/accessors.cc (right):

https://codereview.chromium.org/1195823002/diff/60001/src/accessors.cc#newcod...
src/accessors.cc:205: return wrapper->value()->ToArrayIndex(length);
On 2015/06/19 20:48:57, Toon Verwaest wrote:
> I think this is actually wrong...
> 
> Number.prototype.valueOf = function() { return 100; }
> a = [];
> a.length = new Number(10)
> should probably result in an array of size 100, not 10 as it does right now.

Good point, in addition to checking the wrapper's map for being pristine, we
also need to check its prototype.

Powered by Google App Engine
This is Rietveld 408576698