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#newcode5003
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#newcode1478
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#newcode1481
src/heap/heap.h:1481: }
unreachable?
https://codereview.chromium.org/1105693002/diff/20001/src/heap/heap.h#newcode1484
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.cc#newcode41
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.cc#newcode64
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.cc#newcode72
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#newcode68
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#newcode86
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#newcode91
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/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
