Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1105693002:
Implement IdentityMap<V>, a robust, GC-safe object-identity HashMap. (Closed)
|
Created:
5 years, 8 months ago by titzer Modified:
5 years, 7 months ago Reviewers:
Benedikt Meurer, Hannes Payer (out of office), Erik Corry, Sven Panne, Erik Corry Chromium.org CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionImplement IdentityMap<V>, a robust, GC-safe object-identity HashMap.
R=hpayer@chromium.org, erikcorry@chromium.org
BUG=
Committed: https://crrev.com/6d26ec0b4c74b77a3c8079291e9bbc4bb8998aed
Cr-Commit-Position: refs/heads/master@{#28257}
Patch Set 1 #
Total comments: 16
Patch Set 2 : #
Total comments: 31
Patch Set 3 : #
Total comments: 11
Patch Set 4 : #
Total comments: 6
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Add visitor synchronization. #Patch Set 9 : Fix use-after-free screwup. #Patch Set 10 : #
Messages
Total messages: 44 (5 generated)
On 2015/04/23 17:10:26, titzer wrote: PTAL, still needs a test that actually runs queries in a parallel thread while the main thread is GC'ing.
Drive-by interest: I've been wishing for something better than the Object Identity Hash for hashing JSObjects in Map/Set/WeakMap/WeakSet; any chance the approach here is extendable to those?
Drive-by interest: I've been wishing for something better than the Object Identity Hash for hashing JSObjects in Map/Set/WeakMap/WeakSet; any chance the approach here is extendable to those?
On 2015/04/23 18:15:03, adamk wrote: > Drive-by interest: I've been wishing for something better than the Object > Identity Hash for hashing JSObjects in Map/Set/WeakMap/WeakSet; any chance the > approach here is extendable to those? I haven't thought about that use case. It probably wouldn't work for the direct Object->Value mapping, but maybe could work for Object->Stable hash (?). For one thing, these maps live in the C++ world only and always have strong references to their objects. I suppose it wouldn't be too much work to make them weak, though. Second, it's expected that there will be only a few of them in flight at once; they need to be iterated during scavenge. Initially, at most one per TF compilation, since that will be the first usage. Third, they should be small, since the implementation is a hashed linear search and it's hoped the array will be pretty sparse. Fourth, implementation needs to hold the relocation lock during search of the table.
On 2015/04/24 08:28:24, titzer wrote: > On 2015/04/23 18:15:03, adamk wrote: > > Drive-by interest: I've been wishing for something better than the Object > > Identity Hash for hashing JSObjects in Map/Set/WeakMap/WeakSet; any chance the > > approach here is extendable to those? > > I haven't thought about that use case. It probably wouldn't work for the direct > Object->Value mapping, but maybe could work for Object->Stable hash (?). > > For one thing, these maps live in the C++ world only and always have strong > references to their objects. I suppose it wouldn't be too much work to make them > weak, though. Second, it's expected that there will be only a few of them in > flight at once; they need to be iterated during scavenge. Initially, at most one > per TF compilation, since that will be the first usage. Third, they should be > small, since the implementation is a hashed linear search and it's hoped the > array will be pretty sparse. Fourth, implementation needs to hold the relocation > lock during search of the table. Thanks for the details, that set of restrictions doesn't sound like something we'd want to have to deal with in author code, so I'll go back to the thought-experiment board for a better way to hash objects than identity hash fields. Carry on :)
bmeurer@chromium.org changed reviewers: + bmeurer@chromium.org
https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.cc File src/heap/identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.cc#ne... src/heap/identity-map.cc:17: if (concurrent_) heap_->relocation_mutex()->Lock(); How about a simple helper class like this: namespace { class AutoLocker { public: AutoLocker(Heap* heap, bool concurrent) : heap_(heap), concurrent_(concurent) { if (concurrent_) heap_->relocation_mutex()->Lock(); } ~AutoLocker() { if (concurrent_) heap_->relocation_mutex()->Unlock(); } private: Heap* heap_; bool concurrent_; }; } and then use it like this: AutoLocker locker(heap_, concurrent_); same below. That should make the code more robust. https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h File src/heap/identity-map.h (right): https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:51: RawEntry Lookup(Handle<Object> handle) { Style nit: Since these methods are private, and all call sites are in the .cc file anyway, there's probably no reason to have them in the .h file. Can you move them to the .cc file as well? https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:53: int index = LookupIndex(*handle); How about size_t here, and check for index < size_ instead of index >= 0? https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:57: RawEntry Insert(Handle<Object> handle) { Style nit: Since these methods are private, and all call sites are in the .cc file anyway, there's probably no reason to have them in the .h file. Can you move them to the .cc file as well? https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:64: int Hash(Object* address) { Style nit: Since these methods are private, and all call sites are in the .cc file anyway, there's probably no reason to have them in the .h file. Can you move them to the .cc file as well? https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:68: return static_cast<int>((raw_address >> 11) ^ raw_address); Down casting to int looks wrong here. How about using uintptr_t here? Or even better use size_t?
svenpanne@chromium.org changed reviewers: + svenpanne@chromium.org
DBC https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h File src/heap/identity-map.h (right): https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:68: return static_cast<int>((raw_address >> 11) ^ raw_address); On 2015/04/27 04:04:17, Benedikt Meurer wrote: > Down casting to int looks wrong here. How about using uintptr_t here? Or even > better use size_t? Veto from the *San sheriff, too: This line is causing implementation-defined behavior...
https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h File src/heap/identity-map.h (right): https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:68: return static_cast<int>((raw_address >> 11) ^ raw_address); On 2015/04/27 06:26:47, Sven Panne wrote: > On 2015/04/27 04:04:17, Benedikt Meurer wrote: > > Down casting to int looks wrong here. How about using uintptr_t here? Or even > > better use size_t? > > Veto from the *San sheriff, too: This line is causing implementation-defined > behavior... This is the computation of a hashcode from an address and is inherently implementation-dependent. This is not _undefined_ behavior.
https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.cc File src/heap/identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.cc#ne... src/heap/identity-map.cc:17: if (concurrent_) heap_->relocation_mutex()->Lock(); On 2015/04/27 04:04:17, Benedikt Meurer wrote: > How about a simple helper class like this: > > namespace { > class AutoLocker { > public: > AutoLocker(Heap* heap, bool concurrent) : heap_(heap), concurrent_(concurent) > { > if (concurrent_) heap_->relocation_mutex()->Lock(); > } > ~AutoLocker() { if (concurrent_) heap_->relocation_mutex()->Unlock(); } > > private: > Heap* heap_; > bool concurrent_; > }; > } > > and then use it like this: > > AutoLocker locker(heap_, concurrent_); > > same below. That should make the code more robust. Done. I've made one in the Heap called OptionalRelocationLock. Now the mutex doesn't have to be accessible outside. https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h File src/heap/identity-map.h (right): https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:51: RawEntry Lookup(Handle<Object> handle) { On 2015/04/27 04:04:18, Benedikt Meurer wrote: > Style nit: Since these methods are private, and all call sites are in the .cc > file anyway, there's probably no reason to have them in the .h file. Can you > move them to the .cc file as well? Done. https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:53: int index = LookupIndex(*handle); On 2015/04/27 04:04:18, Benedikt Meurer wrote: > How about size_t here, and check for index < size_ instead of index >= 0? I think int works better here, since we are using as an index, and negative for out-of-bounds is a pretty clear indication. https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:57: RawEntry Insert(Handle<Object> handle) { On 2015/04/27 04:04:17, Benedikt Meurer wrote: > Style nit: Since these methods are private, and all call sites are in the .cc > file anyway, there's probably no reason to have them in the .h file. Can you > move them to the .cc file as well? Done. https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:64: int Hash(Object* address) { On 2015/04/27 04:04:18, Benedikt Meurer wrote: > Style nit: Since these methods are private, and all call sites are in the .cc > file anyway, there's probably no reason to have them in the .h file. Can you > move them to the .cc file as well? Done. https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... src/heap/identity-map.h:68: return static_cast<int>((raw_address >> 11) ^ raw_address); On 2015/04/27 04:04:17, Benedikt Meurer wrote: > Down casting to int looks wrong here. How about using uintptr_t here? Or even > better use size_t? size_t isn't any safer than int. As per below comment, both are implementation-dependent, but this is a hash operation, so it doesn't matter.
looking good, just nits https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.cc File src/heap/identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.cc#ne... src/heap/identity-map.cc:50: Resize(); // Should only have to resize once, since we grow 4x. Let's assert that we just resize once. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.cc File src/heap/identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:40: CHECK_NE(0, raw_address); // cannot store Smi 0 as a key here, sorry. Cannot https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:70: if (--limit == 0) break; // searched too far; resize to insert. Searched https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:72: Resize(); // Should only have to resize once, since we grow 4x. 4 = kResizeFactor https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:129: void IdentityMapBase::Rehash() { Can we assert here that we have the relocation lock? https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:167: size_ = size_ * 4; 4 = kResizeFactor; make this a constant https://codereview.chromium.org/1105693002/diff/20001/test/cctest/test-identi... File test/cctest/test-identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/20001/test/cctest/test-identi... test/cctest/test-identity-map.cc:330: t.heap()->CollectGarbage(i::NEW_SPACE); Let's also write another test where these objects are on an evacuation candidate and get compacted.
erik.corry@gmail.com changed reviewers: + erik.corry@gmail.com
I'm going to stop here and ask a more fundamental question: We already have HashTable from objects.h. Can't we add rehashing-on-GC to that instead of adding code to the code base? This implementation increases the number of roots, potentially quite a lot, thus making our atomic pauses at the start of GC bigger. HashTable from objects.h has the table on-heap so that each n-sized table is just one root instead of a bunch of roots. https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:5003: for (StrongRootsList* list = strong_roots_list_; list; list = list->next_) { It looks like this stuff should be before the serializer/deserializer iteration and should have its own call to v->Synchronize https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1478: delete list; Might we not just as well return here? https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1481: } unreachable? https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1484: class OptionalRelocationLock { Perhaps you could spend a couple of lines of comments here on why we would want a lock that was optional? https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.cc File src/heap/identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:41: // xor some of the upper bits, since the lower 2 or 3 are usually aligned. Xor Also see comments below with missing caps and full stops (periods to you!) https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:64: for (int index = start; index < size_; index++) { For hashes that are near the end of the array we will resize immediately (by 4x!), rather than wrap around and search a bit more. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:72: Resize(); // Should only have to resize once, since we grow 4x. That's very aggressive. Was 2x too slow? Also this heuristic for growing will make the timing of the growing dependent on ASLR, which is rather unpredictable for my taste. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h File src/heap/identity-map.h (right): https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h... src/heap/identity-map.h:68: // Note that in general, SMIs are valid keys, but {SMI #0} is _not_ a valid key. This is a very strange API. I would either ban SMIs altogether or move to a tagged null pointer as the internal empty representation and allow all SMIs. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h... src/heap/identity-map.h:86: V* Find(Handle<Object> handle) { Why does this not return a handle? https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h... src/heap/identity-map.h:91: void Set(Handle<Object> handle, V value) { handle should be called key? Also several other places?
https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.cc File src/heap/identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.cc#ne... src/heap/identity-map.cc:50: Resize(); // Should only have to resize once, since we grow 4x. On 2015/04/27 15:06:32, Hannes Payer wrote: > Let's assert that we just resize once. Resize() will fail if it gets too big. Should I count the number of resizes here? https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.cc File src/heap/identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:40: CHECK_NE(0, raw_address); // cannot store Smi 0 as a key here, sorry. On 2015/04/27 15:06:32, Hannes Payer wrote: > Cannot Done. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:41: // xor some of the upper bits, since the lower 2 or 3 are usually aligned. On 2015/04/27 15:39:13, Erik Corry wrote: > Xor > > Also see comments below with missing caps and full stops (periods to you!) Done. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:64: for (int index = start; index < size_; index++) { On 2015/04/27 15:39:13, Erik Corry wrote: > For hashes that are near the end of the array we will resize immediately (by > 4x!), rather than wrap around and search a bit more. Good catch. I wrap the search now. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:70: if (--limit == 0) break; // searched too far; resize to insert. On 2015/04/27 15:06:32, Hannes Payer wrote: > Searched Done. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:72: Resize(); // Should only have to resize once, since we grow 4x. On 2015/04/27 15:39:13, Erik Corry wrote: > That's very aggressive. Was 2x too slow? > Also this heuristic for growing will make the timing of the growing dependent on > ASLR, which is rather unpredictable for my taste. 4x guarantees that there are no chains that size_ / 2 the next time through the loop, because now size_ is 4x as big. The backstory here is that we expect these tables to be sparse and rather than go for a complex hashing scheme, start with one that's much easier to get right. Thus I wanted to avoid doing buckets and chains which would require a more complex interface to the GC. I considered doing cuckoo hashing (e.g. after a collision, use some more bits from the hash for the next search location). Without any data about the collision frequency, the complexity doesn't seem justified yet. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:129: void IdentityMapBase::Rehash() { On 2015/04/27 15:06:32, Hannes Payer wrote: > Can we assert here that we have the relocation lock? Unfortunately that API appears to have been refactored away... https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:167: size_ = size_ * 4; On 2015/04/27 15:06:32, Hannes Payer wrote: > 4 = kResizeFactor; make this a constant Done.
https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1478: delete list; On 2015/04/27 15:39:13, Erik Corry wrote: > Might we not just as well return here? Doesn't really matter. As written, the code will be robust to accidental duplicates. https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1481: } On 2015/04/27 15:39:13, Erik Corry wrote: > unreachable? Don't need it if we just allow the above. https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h#newcode... src/heap/heap.h:1484: class OptionalRelocationLock { On 2015/04/27 15:39:13, Erik Corry wrote: > Perhaps you could spend a couple of lines of comments here on why we would want > a lock that was optional? Done. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h File src/heap/identity-map.h (right): https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h... src/heap/identity-map.h:68: // Note that in general, SMIs are valid keys, but {SMI #0} is _not_ a valid key. On 2015/04/27 15:39:13, Erik Corry wrote: > This is a very strange API. I would either ban SMIs altogether or move to a > tagged null pointer as the internal empty representation and allow all SMIs. I could solve this with a special cell for SMI #0. The intention was mostly to be useful for real heap objects, but it works with SMIs too. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h... src/heap/identity-map.h:86: V* Find(Handle<Object> handle) { On 2015/04/27 15:39:13, Erik Corry wrote: > Why does this not return a handle? Because the value type V is not a heap object. E.g. we will use this in the compiler for Node* pointers to canonicalize constants. https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h... src/heap/identity-map.h:91: void Set(Handle<Object> handle, V value) { On 2015/04/27 15:39:13, Erik Corry wrote: > handle should be called key? Also several other places? Done.
On 2015/04/27 15:39:13, Erik Corry wrote: > I'm going to stop here and ask a more fundamental question: We already have > HashTable from objects.h. Can't we add rehashing-on-GC to that instead of > adding code to the code base? > Ok, some more context is necessary. These hashmaps are intended to be used in a fully concurrent mode, e.g. by the compiler. A primary use case is to canonicalize references to heap objects to their representation as nodes in the compiler's IR. In particular, maps. This is a job that is only half done in Crankshaft through the use of Unique<T>. With this canonicalization, we can make TurboFan's creation of constants almost fully concurrent. Unfortunately, if the hashtable's internal storage were on the heap that simply won't work, since inserting would then require allocating on the heap, and supporting multiple allocating threads is science fiction at the moment. > This implementation increases the number of roots, potentially quite a lot, thus > making our atomic pauses at the start of GC bigger. HashTable from objects.h > has the table on-heap so that each n-sized table is just one root instead of a > bunch of roots. True, the number of roots increases while the tables are live. But they are expected to be small and short-living. E.g. the ones for the compiler should only have to handle 10s to 100s of entries and are only live for (about half) the life of a compilation. There shouldn't be many compilations in flight at once. Also, scanning these roots should be ideally fast; they're dense little regions of strong roots, no pointer chasing. > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.cc > File src/heap/heap.cc (right): > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.cc#newcod... > src/heap/heap.cc:5003: for (StrongRootsList* list = strong_roots_list_; list; > list = list->next_) { > It looks like this stuff should be before the serializer/deserializer iteration > and should have its own call to v->Synchronize > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h > File src/heap/heap.h (right): > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h#newcode... > src/heap/heap.h:1478: delete list; > Might we not just as well return here? > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h#newcode... > src/heap/heap.h:1481: } > unreachable? > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h#newcode... > src/heap/heap.h:1484: class OptionalRelocationLock { > Perhaps you could spend a couple of lines of comments here on why we would want > a lock that was optional? > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.cc > File src/heap/identity-map.cc (right): > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... > src/heap/identity-map.cc:41: // xor some of the upper bits, since the lower 2 or > 3 are usually aligned. > Xor > > Also see comments below with missing caps and full stops (periods to you!) > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... > src/heap/identity-map.cc:64: for (int index = start; index < size_; index++) { > For hashes that are near the end of the array we will resize immediately (by > 4x!), rather than wrap around and search a bit more. > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... > src/heap/identity-map.cc:72: Resize(); // Should only have to resize once, > since we grow 4x. > That's very aggressive. Was 2x too slow? > Also this heuristic for growing will make the timing of the growing dependent on > ASLR, which is rather unpredictable for my taste. > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h > File src/heap/identity-map.h (right): > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h... > src/heap/identity-map.h:68: // Note that in general, SMIs are valid keys, but > {SMI #0} is _not_ a valid key. > This is a very strange API. I would either ban SMIs altogether or move to a > tagged null pointer as the internal empty representation and allow all SMIs. > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h... > src/heap/identity-map.h:86: V* Find(Handle<Object> handle) { > Why does this not return a handle? > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.h... > src/heap/identity-map.h:91: void Set(Handle<Object> handle, V value) { > handle should be called key? Also several other places?
Not happy with this change. Using addresses for hashes is nasty because it adds work to the atomic phases of the GC. This is the opposite way to the one we want to go. If the argument is that we won't use this feature heavily, then I wonder if we can find a cleaner way to implement this rarely used feature. Can we calculate the hash from the objects themselves instead of their address. As I understand it, they are often maps, which are largely immutable. Are thread-safe handles created on the compiler thread? If yes, does it read concurrently from objects on the single-threaded heap to create them and is that not dangerous. If no, can we put a stable hash in the thread-safe handle, eg the address of the object at the time the handle was created? https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.cc File src/heap/identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... src/heap/identity-map.cc:72: Resize(); // Should only have to resize once, since we grow 4x. On 2015/04/27 16:32:50, titzer wrote: > On 2015/04/27 15:39:13, Erik Corry wrote: > > That's very aggressive. Was 2x too slow? > > Also this heuristic for growing will make the timing of the growing dependent > on > > ASLR, which is rather unpredictable for my taste. > > 4x guarantees that there are no chains that size_ / 2 the next time through the > loop, because now size_ is 4x as big. > > The backstory here is that we expect these tables to be sparse and rather than > go for a complex hashing scheme, start with one that's much easier to get right. > Thus I wanted to avoid doing buckets and chains which would require a more > complex interface to the GC. > > I considered doing cuckoo hashing (e.g. after a collision, use some more bits > from the hash for the next search location). Without any data about the > collision frequency, the complexity doesn't seem justified yet. Rather than cuckoo I would go with Robin Hood hashes, but I agree it's not necessarily worth it. https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.cc File src/heap/identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... src/heap/identity-map.cc:13: static const int kInitialIdentityMapSize = 32; I just reduced the similar constant for element dictionaries from 32 to 4 at no cost in performance. Let's not burn memory if we can avoid it. https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... src/heap/identity-map.cc:14: static const int kResizeFactor = 4; Add a comment that the correctness of the table depends on this being >= 4 Alternatively change from a consecutive-collisions based growing heuristic to a more conventional fullness-based heuristic. This would prevent the growth from being dependent on a source of randomness (ASLR). https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... src/heap/identity-map.cc:172: CHECK_LE(size_, (1024 * 1024 * 16)); // that would be extreme... You really want to deliberately crash the release mode VM for this? Also capital 't'. https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... src/heap/identity-map.cc:188: heap_->RegisterStrongRoots(keys_, keys_ + size_); Where do we free the keys and values? https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.h File src/heap/identity-map.h (right): https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.h... src/heap/identity-map.h:65: // The value type {V} must be reinterpret_cast'able to void*. Add comment that V should not be a heap object type or a pointer to a heap object type. Ideally also a static assert. https://codereview.chromium.org/1105693002/diff/40001/test/cctest/test-identi... File test/cctest/test-identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/40001/test/cctest/test-identi... test/cctest/test-identity-map.cc:121: CHECK_NULL(t.map.Find(t.smi(i))); Is there a missing assert, since you can look up Smi 0, which is an illegal key?
On 2015/04/28 09:32:34, Erik Corry wrote: > Not happy with this change. > > Using addresses for hashes is nasty because it adds work to the atomic phases of > the GC. This is the opposite way to the one we want to go. If the argument is > that we won't use this feature heavily, then I wonder if we can find a cleaner > way to implement this rarely used feature. > Ideally there would be no extra work, but that's not really workable. We might reduce this by splitting up the storage into many smaller regions that don't have to be atomically scanned as roots (i.e. could be incrementally marked), but I foresee nothing but complexity for little real benefit. In comparison to the other roots we are scanning, e.g. the stack and string tables, a couple 32-word regions of pointers is miniscule. > Can we calculate the hash from the objects themselves instead of their address. > As I understand it, they are often maps, which are largely immutable. > No, because that would mean not only dereferencing the handle, but dereferencing the object address; there is no stable hash for an arbitrary object, and yes, the compiler absolutely wants to canonicalize objects based on their identity. > Are thread-safe handles created on the compiler thread? If yes, does it read > concurrently from objects on the single-threaded heap to create them and is that > not dangerous. If no, can we put a stable hash in the thread-safe handle, eg > the address of the object at the time the handle was created? > At the moment, no. Canonicalizing object references is step 0. But yes, in the future we will want to read from objects on the heap concurrently eventually, in particular the map and then start reading data from the map. Each of those steps will require careful thought to make sure it is safe and properly fenced. Putting a "stable" hash in the thread-safe handle is exactly what Unique<T> does. That's not enough. We need to canonicalize based on object-identity. One could consider a special handle scope that canonicalizes handle locations, and then use the location of the handle as the uniqueness. But that's essentially just making this IdentityMap<T> itself into a handlescope. Maybe something for the future. > https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.cc > File src/heap/identity-map.cc (right): > > https://codereview.chromium.org/1105693002/diff/20001/src/heap/identity-map.c... > src/heap/identity-map.cc:72: Resize(); // Should only have to resize once, > since we grow 4x. > On 2015/04/27 16:32:50, titzer wrote: > > On 2015/04/27 15:39:13, Erik Corry wrote: > > > That's very aggressive. Was 2x too slow? > > > Also this heuristic for growing will make the timing of the growing > dependent > > on > > > ASLR, which is rather unpredictable for my taste. > > > > 4x guarantees that there are no chains that size_ / 2 the next time through > the > > loop, because now size_ is 4x as big. > > > > The backstory here is that we expect these tables to be sparse and rather than > > go for a complex hashing scheme, start with one that's much easier to get > right. > > Thus I wanted to avoid doing buckets and chains which would require a more > > complex interface to the GC. > > > > I considered doing cuckoo hashing (e.g. after a collision, use some more bits > > from the hash for the next search location). Without any data about the > > collision frequency, the complexity doesn't seem justified yet. > > Rather than cuckoo I would go with Robin Hood hashes, but I agree it's not > necessarily worth it. > > https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.cc > File src/heap/identity-map.cc (right): > > https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... > src/heap/identity-map.cc:13: static const int kInitialIdentityMapSize = 32; > I just reduced the similar constant for element dictionaries from 32 to 4 at no > cost in performance. Let's not burn memory if we can avoid it. > > https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... > src/heap/identity-map.cc:14: static const int kResizeFactor = 4; > Add a comment that the correctness of the table depends on this being >= 4 > > Alternatively change from a consecutive-collisions based growing heuristic to a > more conventional fullness-based heuristic. This would prevent the growth from > being dependent on a source of randomness (ASLR). > > https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... > src/heap/identity-map.cc:172: CHECK_LE(size_, (1024 * 1024 * 16)); // that > would be extreme... > You really want to deliberately crash the release mode VM for this? > Also capital 't'. > > https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... > src/heap/identity-map.cc:188: heap_->RegisterStrongRoots(keys_, keys_ + size_); > Where do we free the keys and values? > > https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.h > File src/heap/identity-map.h (right): > > https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.h... > src/heap/identity-map.h:65: // The value type {V} must be reinterpret_cast'able > to void*. > Add comment that V should not be a heap object type or a pointer to a heap > object type. Ideally also a static assert. > > https://codereview.chromium.org/1105693002/diff/40001/test/cctest/test-identi... > File test/cctest/test-identity-map.cc (right): > > https://codereview.chromium.org/1105693002/diff/40001/test/cctest/test-identi... > test/cctest/test-identity-map.cc:121: CHECK_NULL(t.map.Find(t.smi(i))); > Is there a missing assert, since you can look up Smi 0, which is an illegal key?
https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.cc File src/heap/identity-map.cc (right): https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... src/heap/identity-map.cc:13: static const int kInitialIdentityMapSize = 32; On 2015/04/28 09:32:34, Erik Corry wrote: > I just reduced the similar constant for element dictionaries from 32 to 4 at no > cost in performance. Let's not burn memory if we can avoid it. Done. https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... src/heap/identity-map.cc:14: static const int kResizeFactor = 4; On 2015/04/28 09:32:34, Erik Corry wrote: > Add a comment that the correctness of the table depends on this being >= 4 > > Alternatively change from a consecutive-collisions based growing heuristic to a > more conventional fullness-based heuristic. This would prevent the growth from > being dependent on a source of randomness (ASLR). As written, it doesn't. It will just keep resizing and trying to insert until the longest collision chain is less than the limit. It so happens that 4x guarantees that it will never iterate more than once. https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... src/heap/identity-map.cc:172: CHECK_LE(size_, (1024 * 1024 * 16)); // that would be extreme... On 2015/04/28 09:32:34, Erik Corry wrote: > You really want to deliberately crash the release mode VM for this? > Also capital 't'. Good question...then again, having 16 million entries in such a hashmap is going to be a performance problem and might indicate a bug elsewhere. https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.c... src/heap/identity-map.cc:188: heap_->RegisterStrongRoots(keys_, keys_ + size_); On 2015/04/28 09:32:34, Erik Corry wrote: > Where do we free the keys and values? They're zone-allocated.
https://codereview.chromium.org/1105693002/diff/60001/src/heap/identity-map.h File src/heap/identity-map.h (right): https://codereview.chromium.org/1105693002/diff/60001/src/heap/identity-map.h... src/heap/identity-map.h:88: *(reinterpret_cast<V*>(GetEntry(key))) = value; This assignment takes place without holding the lock, but outside the lock, things can move around. You could be writing in the old version. Similar issues with Find and Get. They are returning pointers into the values array, which the caller can use without holding the lock.
On 2015/04/28 13:51:16, Erik Corry wrote: > https://codereview.chromium.org/1105693002/diff/60001/src/heap/identity-map.h > File src/heap/identity-map.h (right): > > https://codereview.chromium.org/1105693002/diff/60001/src/heap/identity-map.h... > src/heap/identity-map.h:88: *(reinterpret_cast<V*>(GetEntry(key))) = value; > This assignment takes place without holding the lock, but outside the lock, > things can move around. You could be writing in the old version. > > Similar issues with Find and Get. They are returning pointers into the values > array, which the caller can use without holding the lock. The values array does not contain pointers into the heap, and is not visible to the GC.
My mistake. I had missed that the Rehash takes place on the compiler thread after a failed lookup. Tricky! https://codereview.chromium.org/1105693002/diff/60001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1105693002/diff/60001/src/heap/heap.cc#newcod... src/heap/heap.cc:5034: for (StrongRootsList* list = strong_roots_list_; list; list = list->next_) { We don't need a lock here?
I'm warming to this change/coming to terms with its inevitability. Some non-blocking comments to consider. * Please reconsider the 4x growth factor. It's caused by the resizing heuristic which is an unusual feature. * Currently you can have a resize during a rehash. You should either be sure the tests cover this happening or change the resizing heuristic so it can't happen. * Those methods that require the mutex should assert that they have it. This includes Heap::IterateStrongRoots * Release mode asserts that are not security relevant bloat the code.
On 2015/04/27 07:57:51, titzer wrote: > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h > File src/heap/identity-map.h (right): > > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... > src/heap/identity-map.h:68: return static_cast<int>((raw_address >> 11) ^ > raw_address); > On 2015/04/27 06:26:47, Sven Panne wrote: > > On 2015/04/27 04:04:17, Benedikt Meurer wrote: > > > Down casting to int looks wrong here. How about using uintptr_t here? Or > even > > > better use size_t? > > > > Veto from the *San sheriff, too: This line is causing implementation-defined > > behavior... > > This is the computation of a hashcode from an address and is inherently > implementation-dependent. This is not _undefined_ behavior. I think you're talking about different implementation-defined behaviors: There's (a) the cast from a pointer to an integral type, which is _desired_ implementation-defined behavior, and (b) the down cast to int, which is _undesired_ implementation-defined behavior. And I think Sven was only talking about (b).
https://codereview.chromium.org/1105693002/diff/60001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1105693002/diff/60001/src/heap/heap.h#newcode... src/heap/heap.h:1456: struct StrongRootsList { Style nit: Please forward declare struct and the two methods below in the .h file and move the definition of the struct and the implementations of the methods to the .cc file. Also since it's a struct and all fields are public, you don't need the _ suffix on their names. https://codereview.chromium.org/1105693002/diff/60001/src/heap/heap.h#newcode... src/heap/heap.h:1504: explicit OptionalRelocationLock(Heap* heap, bool concurrent) You don't need explicit here.
On 2015/04/29 03:43:10, Benedikt Meurer wrote: > On 2015/04/27 07:57:51, titzer wrote: > > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h > > File src/heap/identity-map.h (right): > > > > > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... > > src/heap/identity-map.h:68: return static_cast<int>((raw_address >> 11) ^ > > raw_address); > > On 2015/04/27 06:26:47, Sven Panne wrote: > > > On 2015/04/27 04:04:17, Benedikt Meurer wrote: > > > > Down casting to int looks wrong here. How about using uintptr_t here? Or > > even > > > > better use size_t? > > > > > > Veto from the *San sheriff, too: This line is causing implementation-defined > > > behavior... > > > > This is the computation of a hashcode from an address and is inherently > > implementation-dependent. This is not _undefined_ behavior. > > I think you're talking about different implementation-defined behaviors: There's > (a) the cast from a pointer to an integral type, which is _desired_ > implementation-defined behavior, and (b) the down cast to int, which is > _undesired_ implementation-defined behavior. And I think Sven was only talking > about (b). If you cast the pointer to unintptr_t then the subsequent downcast to unsigned (not int) will be well defined, not implementation defined. It won't, in practice, make any difference to anything, because none of the compilers we care about define this 'wrong', and in any case the code works whatever result we calculate here. V8 is full of undefined behaviour. Tagged pointers, a JIT, Smi 'pointers' etc. Getting rid of this tiny piece of implementation defined behaviour in a hash code calculator seems like a waste of time to me.
On 2015/04/29 09:29:36, Erik Corry wrote: > If you cast the pointer to unintptr_t then the subsequent downcast to unsigned > (not int) will be well defined, not implementation defined. > > It won't, in practice, make any difference to anything, because none of the > compilers we care about define this 'wrong', and in any case the code works > whatever result we calculate here. Basically, I just wanted to raise the awareness regarding implementation-defined/undefined behavior here (once again), especially when it is easy to avoid it, like in this case. > V8 is full of undefined behaviour. Tagged pointers, a JIT, Smi 'pointers' etc. > Getting rid of this tiny piece of implementation defined behaviour in a hash > code calculator seems like a waste of time to me. *Undefined* behavior is a totally different beast and far from being harmless. We already had several hard-to-debug issues in the past because of this (fun stuff like "this" being null because of our Smis etc., with the compiler rightfully removing any "if (this == null)...", we saw sign checks being removed because of undefined shift behavior for signed values, etc.). For undefined behavior the compiler can even choose to do different things at different places, which doesn't exactly makes things easier. Furthermore, v8 is not exactly a good citizen in the Chrome/google3 eco system, where people are trying to make UBSan happy. In a nutshell: We can possibly live with implementation-defined behavior (if necessary), but we should remove undefined behavior, there is already a tracking bug for this: https://code.google.com/p/v8/issues/detail?id=3770.
On 2015/04/29 03:43:10, Benedikt Meurer wrote: > On 2015/04/27 07:57:51, titzer wrote: > > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h > > File src/heap/identity-map.h (right): > > > > > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... > > src/heap/identity-map.h:68: return static_cast<int>((raw_address >> 11) ^ > > raw_address); > > On 2015/04/27 06:26:47, Sven Panne wrote: > > > On 2015/04/27 04:04:17, Benedikt Meurer wrote: > > > > Down casting to int looks wrong here. How about using uintptr_t here? Or > > even > > > > better use size_t? > > > > > > Veto from the *San sheriff, too: This line is causing implementation-defined > > > behavior... > > > > This is the computation of a hashcode from an address and is inherently > > implementation-dependent. This is not _undefined_ behavior. > > I think you're talking about different implementation-defined behaviors: There's > (a) the cast from a pointer to an integral type, which is _desired_ > implementation-defined behavior, and (b) the down cast to int, which is > _undesired_ implementation-defined behavior. And I think Sven was only talking > about (b). We always mask the hash before going to search the table, so I think the implementation-defined handling of the sign bit is a non-issue here.
On 2015/04/29 15:03:35, titzer wrote: > On 2015/04/29 03:43:10, Benedikt Meurer wrote: > > On 2015/04/27 07:57:51, titzer wrote: > > > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h > > > File src/heap/identity-map.h (right): > > > > > > > > > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... > > > src/heap/identity-map.h:68: return static_cast<int>((raw_address >> 11) ^ > > > raw_address); > > > On 2015/04/27 06:26:47, Sven Panne wrote: > > > > On 2015/04/27 04:04:17, Benedikt Meurer wrote: > > > > > Down casting to int looks wrong here. How about using uintptr_t here? Or > > > even > > > > > better use size_t? > > > > > > > > Veto from the *San sheriff, too: This line is causing > implementation-defined > > > > behavior... > > > > > > This is the computation of a hashcode from an address and is inherently > > > implementation-dependent. This is not _undefined_ behavior. > > > > I think you're talking about different implementation-defined behaviors: > There's > > (a) the cast from a pointer to an integral type, which is _desired_ > > implementation-defined behavior, and (b) the down cast to int, which is > > _undesired_ implementation-defined behavior. And I think Sven was only talking > > about (b). > > We always mask the hash before going to search the table, so I think the > implementation-defined handling of the sign bit is a non-issue here. I think it all comes down to the fact that int is simply the wrong type here, and it doesn't buy you anything since you are just using it as an array index, so you could as well just use the correct type instead, which is probably size_t.
On 2015/04/30 05:33:51, Benedikt Meurer wrote: > On 2015/04/29 15:03:35, titzer wrote: > > On 2015/04/29 03:43:10, Benedikt Meurer wrote: > > > On 2015/04/27 07:57:51, titzer wrote: > > > > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h > > > > File src/heap/identity-map.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... > > > > src/heap/identity-map.h:68: return static_cast<int>((raw_address >> 11) ^ > > > > raw_address); > > > > On 2015/04/27 06:26:47, Sven Panne wrote: > > > > > On 2015/04/27 04:04:17, Benedikt Meurer wrote: > > > > > > Down casting to int looks wrong here. How about using uintptr_t here? > Or > > > > even > > > > > > better use size_t? > > > > > > > > > > Veto from the *San sheriff, too: This line is causing > > implementation-defined > > > > > behavior... > > > > > > > > This is the computation of a hashcode from an address and is inherently > > > > implementation-dependent. This is not _undefined_ behavior. > > > > > > I think you're talking about different implementation-defined behaviors: > > There's > > > (a) the cast from a pointer to an integral type, which is _desired_ > > > implementation-defined behavior, and (b) the down cast to int, which is > > > _undesired_ implementation-defined behavior. And I think Sven was only > talking > > > about (b). > > > > We always mask the hash before going to search the table, so I think the > > implementation-defined handling of the sign bit is a non-issue here. > > I think it all comes down to the fact that int is simply the wrong type here, > and it doesn't buy you anything since you are just using it as an array index, > so you could as well just use the correct type instead, which is probably > size_t. ... which will also help the C++ compiler to generate better code because then there's no need to care about sign extension when using 32-bit array indices on x64.
https://codereview.chromium.org/1105693002/diff/60001/src/heap/heap.h File src/heap/heap.h (right): https://codereview.chromium.org/1105693002/diff/60001/src/heap/heap.h#newcode... src/heap/heap.h:1456: struct StrongRootsList { On 2015/04/29 03:48:35, Benedikt Meurer wrote: > Style nit: Please forward declare struct and the two methods below in the .h > file and move the definition of the struct and the implementations of the > methods to the .cc file. > > Also since it's a struct and all fields are public, you don't need the _ suffix > on their names. Done. https://codereview.chromium.org/1105693002/diff/60001/src/heap/heap.h#newcode... src/heap/heap.h:1504: explicit OptionalRelocationLock(Heap* heap, bool concurrent) On 2015/04/29 03:48:35, Benedikt Meurer wrote: > You don't need explicit here. Done.
On 2015/04/30 05:33:51, Benedikt Meurer wrote: > On 2015/04/29 15:03:35, titzer wrote: > > On 2015/04/29 03:43:10, Benedikt Meurer wrote: > > > On 2015/04/27 07:57:51, titzer wrote: > > > > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h > > > > File src/heap/identity-map.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1105693002/diff/1/src/heap/identity-map.h#new... > > > > src/heap/identity-map.h:68: return static_cast<int>((raw_address >> 11) ^ > > > > raw_address); > > > > On 2015/04/27 06:26:47, Sven Panne wrote: > > > > > On 2015/04/27 04:04:17, Benedikt Meurer wrote: > > > > > > Down casting to int looks wrong here. How about using uintptr_t here? > Or > > > > even > > > > > > better use size_t? > > > > > > > > > > Veto from the *San sheriff, too: This line is causing > > implementation-defined > > > > > behavior... > > > > > > > > This is the computation of a hashcode from an address and is inherently > > > > implementation-dependent. This is not _undefined_ behavior. > > > > > > I think you're talking about different implementation-defined behaviors: > > There's > > > (a) the cast from a pointer to an integral type, which is _desired_ > > > implementation-defined behavior, and (b) the down cast to int, which is > > > _undesired_ implementation-defined behavior. And I think Sven was only > talking > > > about (b). > > > > We always mask the hash before going to search the table, so I think the > > implementation-defined handling of the sign bit is a non-issue here. > > I think it all comes down to the fact that int is simply the wrong type here, > and it doesn't buy you anything since you are just using it as an array index, > so you could as well just use the correct type instead, which is probably > size_t. There is no "right" and "wrong" type here. I used int because it's convenient to represent an out-of-bounds index as negative. That's nice because -1 is always out of bounds, even if size_ changes.
LGTM, if you have considered the things in comment 25 and a few missed comments futher up.
https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.h File src/heap/identity-map.h (right): https://codereview.chromium.org/1105693002/diff/40001/src/heap/identity-map.h... src/heap/identity-map.h:65: // The value type {V} must be reinterpret_cast'able to void*. On 2015/04/28 09:32:34, Erik Corry wrote: > Add comment that V should not be a heap object type or a pointer to a heap > object type. Ideally also a static assert. I've added a bulleted list to make usage more clear.
https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.cc File src/heap/heap.cc (right): https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.cc#newcod... src/heap/heap.cc:5003: for (StrongRootsList* list = strong_roots_list_; list; list = list->next_) { On 2015/04/27 15:39:13, Erik Corry wrote: > It looks like this stuff should be before the serializer/deserializer iteration > and should have its own call to v->Synchronize Done.
Check out the ASAN bot failures and please add a motivation to the description.
LGTM
The CQ bit was checked by titzer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erik.corry@gmail.com Link to the patchset: https://codereview.chromium.org/1105693002/#ps170001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1105693002/170001
Message was sent while issue was closed.
Committed patchset #10 (id:170001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/6d26ec0b4c74b77a3c8079291e9bbc4bb8998aed Cr-Commit-Position: refs/heads/master@{#28257} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
