On 2013/09/11 10:34:07, rossberg wrote:
https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2309
src/hydrogen.h:2309: class HUnique V8_FINAL {
I think this should live somewhere else (e.g. handles.h or perhaps its own
file), and be named Unique, since it isn't really specific to Hydrogen.
Yeah I agree that we might want it to live in a more general place later,
but I
don't want to go too far down the path of generalizing it just yet. So, I
want
to introduce it and start using in hydrogen, and when we find other uses
for it,
we can move it out.
https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2384
src/hydrogen.h:2384: for (int j = size_ - 1; j >= i; j--) {
Could it be worth using memmove here?
I thought about that, but that call would look like:
OS::MemMove(array_ + j + 1, array_ + j, (size_ - j - 1) *
sizeof(HUnique<T>));
https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2412
src/hydrogen.h:2412: bool found = false;
The rest of this for-loop could be simplified to:
while (j < that->size_ && sought != that->array_[j]) j++;
if (j == that->size_) return false;
Nice.
https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2429
src/hydrogen.h:2429: out->Grow(this->size_ > that->size_ ? this->size_ :
that->size_, zone);
Nit: Min(...)
Done.
https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2434
src/hydrogen.h:2434: if (j >= that->size_) break; // Right has been
exhausted.
Why not simply
while (i < this->size && j < that->size_)
Nice.
https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2462
src/hydrogen.h:2462: for (;;) {
Same here:
while (i < this->size && j < that->size_)
and then simply process the rests after the loop:
while (j < that->size_) out->array_[k++] = that->array_[j++];
while (i < this->size_) out->array_[k++] = this->array_[i++];
Oh alright. You fancy kids and your && constructs.
https://codereview.chromium.org/23609020/diff/1/src/hydrogen.h#newcode2518
src/hydrogen.h:2518: int new_capacity = capacity_ + capacity_ + size; //
2*current + needed
That seems like a lot, and can cause up to a 200% space overhead. What's
the
rationale?
I've used this growth heuristic before and it works pretty nicely. The
rationale
is that most sets will be 1 element, probably 99.9% after that will be <= 4
elements (in fact, SmallMapList is limited to 4 elements, IIRC). Other sets
might be big, so you want to grow aggressively to avoid copying.
https://codereview.chromium.org/23609020/
--
--
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/groups/opt_out.
