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

Issue 202293004: Work in progress: ES6 Maps and Sets with deterministic iteration support (Closed)

Created:
6 years, 9 months ago by adamk
Modified:
6 years, 8 months ago
CC:
v8-dev
Visibility:
Public.

Description

Work in progress: ES6 Maps and Sets with deterministic iteration support

Patch Set 1 #

Patch Set 2 : Added support for chaining #

Patch Set 3 : Added growth #

Total comments: 5

Patch Set 4 : Enough to pass Set tests #

Patch Set 5 : Fixed Rehash to have two separate counters #

Patch Set 6 : Now with Map support #

Patch Set 7 : Added tests copied from existing ObjectHashTable tests #

Patch Set 8 : Add shrinking #

Patch Set 9 : Avoid unnecessary growth #

Total comments: 4

Patch Set 10 : Rename all the things #

Patch Set 11 : More cleanup #

Patch Set 12 : Move back into objects.h #

Patch Set 13 : Passes lint check #

Patch Set 14 : Ready for first review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+543 lines, -74 lines) Patch
M src/factory.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -0 lines 0 comments Download
M src/factory.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +14 lines, -0 lines 0 comments Download
M src/objects.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +157 lines, -0 lines 0 comments Download
M src/objects.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +225 lines, -0 lines 0 comments Download
M test/cctest/cctest.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +1 line, -0 lines 0 comments Download
A + test/cctest/test-ordered-hash-table.cc View 1 2 3 4 5 6 7 8 9 10 11 12 8 chunks +143 lines, -74 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
arv (Not doing code reviews)
https://codereview.chromium.org/202293004/diff/50001/src/close-table.h File src/close-table.h (right): https://codereview.chromium.org/202293004/diff/50001/src/close-table.h#newcode77 src/close-table.h:77: return kHashTableStartIndex + Smi::cast(get(kNumberOfBucketsIndex))->value(); This can be return kHashTableStartIndex ...
6 years, 9 months ago (2014-03-27 22:39:02 UTC) #1
adamk
https://codereview.chromium.org/202293004/diff/50001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/202293004/diff/50001/src/objects.cc#newcode15946 src/objects.cc:15946: new_table->set(index, key); On 2014/03/27 22:39:02, arv wrote: > For ...
6 years, 9 months ago (2014-03-27 22:43:13 UTC) #2
adamk
Latest patch has Map support (I'm following the existing ObjectHashTable interface which uses Put with ...
6 years, 9 months ago (2014-03-27 23:14:21 UTC) #3
arv (Not doing code reviews)
https://codereview.chromium.org/202293004/diff/160001/src/objects.cc File src/objects.cc (right): https://codereview.chromium.org/202293004/diff/160001/src/objects.cc#newcode15903 src/objects.cc:15903: capacity = Max(kMinCapacity, capacity); Can be an assert instead? ...
6 years, 9 months ago (2014-03-29 02:26:07 UTC) #4
adamk
6 years, 8 months ago (2014-03-31 15:36:39 UTC) #5
https://codereview.chromium.org/202293004/diff/160001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/202293004/diff/160001/src/objects.cc#newcode1...
src/objects.cc:15903: capacity = Max(kMinCapacity, capacity);
On 2014/03/29 02:26:07, arv wrote:
> Can be an assert instead? I don't think this needs to be a runtime check.

Something like that, yeah. I'm sort of waiting till the callers shake out to fix
the API of Allocate (and Factory::New*)

https://codereview.chromium.org/202293004/diff/160001/src/objects.cc#newcode1...
src/objects.cc:15929: // Don't need to grow if we can simply clear out deleted
entries instead.
On 2014/03/29 02:26:07, arv wrote:
> This comment does not look accurate. We still call Allocate.
> 
> We should probably add a Compact function that does the in place cleanup.

The comment is confusing, I admit. It's trying to explain the ternary operator
on the next line, which doubles capacity unless keeping the same capacity (but a
new backing store) would work, due to deletions.

I don't believe this data structure supports in-place rehashing, since
collapsing chains can break the ordering. But perhaps you have something clever
in mind?

Powered by Google App Engine
This is Rietveld 408576698