First implementation of HUnique<T> and HUniqueSet<T>, which is supposed to replace UniqueValueId.

src/hydrogen.h

Index: src/hydrogen.h
diff --git a/src/hydrogen.h b/src/hydrogen.h
index 395d1cdbc39d74f138114cdfc075bdd0e6a5e001..58eb0eb269387dbcebed6bcd749537c53fe35d58 100644
--- a/src/hydrogen.h
+++ b/src/hydrogen.h
@@ -2301,6 +2301,231 @@ class NoObservableSideEffectsScope V8_FINAL {
 };
 
 
+template <typename T>
+class HUniqueSet;
+
+
+template <typename T>
+class HUnique V8_FINAL {

[rossberg, 2013/09/11 10:34:07]

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.

[Toon Verwaest, 2013/09/11 11:17:57]

A separate file would be nice.

+ public:
+  // TODO(titzer): this should be private.
+  explicit HUnique(Handle<T> handle) {
+    if (handle.is_null()) {
+      raw_address_ = NULL;
+    } else {
+      raw_address_ = reinterpret_cast<Address>(*handle);

[Toon Verwaest, 2013/09/11 11:17:57]

Can we ensure that this is only called in a DisallowHeapAllocation? This is
pretty scary stuff.

The UniqueValueIds are in exactly such a scope, so they are already protected.
If you cannot do this because you plan on using this in parallel compilation;
could we just use the unique value ids that are stored up front rather than the
handles?

+      ASSERT_NE(raw_address_, NULL);
+    }
+    handle_ = handle;
+  }
+
+  // Constructor for handling automatic up casting.
+  // Ex. HUnique<JSFunction> can be passed when HUnique<Object> is expected.
+  template <class S> HUnique(HUnique<S> uniq) {
+#ifdef DEBUG
+    T* a = NULL;
+    S* b = NULL;
+    a = b;  // Fake assignment to enforce type checks.
+    USE(a);
+#endif
+    raw_address_ = uniq.raw_address_;
+    handle_ = uniq.handle_;  // Creates a new handle sharing the same location.
+  }
+
+  template <typename U>
+  bool operator==(const HUnique<U>& other) const {
+    return raw_address_ == other.raw_address_;
+  }
+
+  template <typename U>
+  bool operator!=(const HUnique<U>& other) const {
+    return raw_address_ != other.raw_address_;
+  }
+
+  intptr_t Hashcode() const {
+    return reinterpret_cast<intptr_t>(raw_address_);
+  }
+
+  bool IsNull() {
+    return raw_address_ == NULL;
+  }
+
+  // Don't do this unless you have access to the heap!
+  // No, seriously! You can compare and hash and set-ify uniques that were
+  // all created at the same time; please don't dereference.
+  Handle<T> handle() {
+    return handle_;
+  }
+
+  friend class HUniqueSet<T>;  // Uses internal implementation details for speed
+  template <class U>
+  friend class HUnique;  // For comparing raw_address values
+
+ private:
+  Address raw_address_;
+  Handle<T> handle_;
+};
+
+
+template <typename T>
+class HUniqueSet V8_FINAL : public ZoneObject {
+ public:
+  // Constructor; a new set will be empty.
+  HUniqueSet() : size_(0), capacity_(0), array_(NULL) { }
+
+  // Add a new element to this unique set; mutates this set.
+  void Add(HUnique<T> uniq, Zone* zone) {
+    // Keep the set sorted by the {raw_address} of the unique elements.
+    for (int i = 0; i < size_; i++) {
+      if (array_[i] == uniq) return;
+      if (array_[i].raw_address_ > uniq.raw_address_) {
+        // Insert in the middle.
+        Grow(size_ + 1, zone);

[Toon Verwaest, 2013/09/11 11:17:57]

You can merge this code with the code after the for-loop by declaring "int i"
outside of the for-loop; then always growing by 1, moving elements past i to the
top (won't be any if i == size), and assigning array_[i] = uniq;

+        for (int j = size_ - 1; j >= i; j--) {

[rossberg, 2013/09/11 10:34:07]

Could it be worth using memmove here?

+          array_[j + 1] = array_[j];
+        }
+        array_[i] = uniq;
+        size_++;
+        return;
+      }
+    }
+    // Append the element to the the end.
+    Grow(size_ + 1, zone);
+    array_[size_++] = uniq;
+  }
+
+  // Compare this set against another set.
+  bool Equals(HUniqueSet<T>* that) {
+    if (that->size_ != this->size_) return false;
+    for (int i = 0; i < this->size_; i++) {
+      if (this->array_[i] != that->array_[i]) return false;
+    }
+    return true;
+  }
+
+  // Check if this set is a subset of the given set.
+  bool IsSubset(HUniqueSet<T>* that) {
+    if (that->size_ < this->size_) return false;
+    int j = 0;
+    for (int i = 0; i < this->size_; i++) {
+      HUnique<T> sought = this->array_[i];
+      bool found = false;

[rossberg, 2013/09/11 10:34:07]

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;

[Toon Verwaest, 2013/09/11 14:24:11]

This version double-checks every entry though; since you don't increment to the
next value if sought == that->array[j].

On 2013/09/11 10:34:07, rossberg wrote:

[rossberg, 2013/09/11 15:01:49]

Good catch. Tweaking that is left as an exercise. :)

[Toon Verwaest, 2013/09/11 15:27:53]

Hence the version I attached in my previous comment ;)

On 2013/09/11 15:01:49, rossberg wrote:

[rossberg, 2013/09/12 09:34:45]

That's still one for the kitty. :)

+      while (j < that->size_) {
+        if (sought == that->array_[j++]) {
+          found = true;
+          break;
+        }
+      }
+      if (!found) return false;

[Toon Verwaest, 2013/09/11 11:17:57]

For minimal checking

if (that->size_ < this->size_) return false;
int j = 0;
for (int i = 0; i < this->size_; i++) {
  HUnique<T> sought = this->array_[i];
  do {
    if (sought == that->array[j++]) break;
    // Fail whenever we have less elements left in [that] than
    // we still need to find from [this].
    if (that->size_ - j < this->size_ - i) return false;
  } while (true);
}
return true;

+    }
+    return true;
+  }
+
+  // Returns a new set representing the intersection of this set and the other.
+  HUniqueSet<T>* Intersect(HUniqueSet<T>* that, Zone* zone) {
+    if (that->size_ == 0 || this->size_ == 0) return new(zone) HUniqueSet<T>();
+
+    HUniqueSet<T>* out = new(zone) HUniqueSet<T>();
+    out->Grow(this->size_ > that->size_ ? this->size_ : that->size_, zone);

[rossberg, 2013/09/11 10:34:07]

Nit: Min(...)

[Toon Verwaest, 2013/09/11 11:17:57]

I guess you meant to flip the sizes?
What about just using out->Grow(Min(this->size_, that->size_), zone) to avoid
problems :)

+
+    int i = 0, j = 0, k = 0;
+    for (;;) {
+      if (i >= this->size_) break;  // Left has been exhausted.
+      if (j >= that->size_) break;  // Right has been exhausted.

[rossberg, 2013/09/11 10:34:07]

Why not simply

while (i < this->size && j < that->size_)

+
+      HUnique<T> a = this->array_[i];
+      HUnique<T> b = that->array_[j];
+      if (a == b) {
+        out->array_[k++] = a;
+        i++;
+        j++;
+      } else if (a.raw_address_ < b.raw_address_) {
+        i++;
+      } else {
+        j++;
+      }
+    }
+
+    out->size_ = k;
+    return out;
+  }
+
+  // Returns a new set representing the union of this set and the other.
+  HUniqueSet<T>* Union(HUniqueSet<T>* that, Zone* zone) {
+    if (that->size_ == 0) return this->Copy(zone);
+    if (this->size_ == 0) return that->Copy(zone);
+
+    HUniqueSet<T>* out = new(zone) HUniqueSet<T>();
+    out->Grow(this->size_ + that->size_, zone);
+
+    int i = 0, j = 0, k = 0;
+    for (;;) {

[rossberg, 2013/09/11 10:34:07]

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++];

+      if (i >= this->size_) {  // Left has been exhausted.
+        while (j < that->size_) out->array_[k++] = that->array_[j++];
+        break;
+      }
+      if (j >= that->size_) {  // Right has been exhausted.
+        while (i < this->size_) out->array_[k++] = this->array_[i++];
+        break;
+      }
+
+      HUnique<T> a = this->array_[i];
+      HUnique<T> b = that->array_[j];
+      if (a == b) {
+        out->array_[k++] = a;
+        i++;
+        j++;
+      } else if (a.raw_address_ < b.raw_address_) {
+        out->array_[k++] = a;
+        i++;
+      } else {
+        out->array_[k++] = b;
+        j++;
+      }
+    }
+
+    out->size_ = k;
+    return out;
+  }
+
+  // Makes an exact copy of this set.
+  HUniqueSet<T>* Copy(Zone* zone) {
+    HUniqueSet<T>* copy = new(zone) HUniqueSet<T>();
+    copy->size_ = this->size_;
+    copy->capacity_ = this->size_;
+    copy->array_ = zone->NewArray<HUnique<T> >(this->size_);
+    memcpy(copy->array_, this->array_, this->size_ * sizeof(HUnique<T>));
+    return copy;
+  }
+
+  inline int size() {
+    return size_;
+  }
+
+ private:
+  // These sets should be small, since operations are implemented with simple
+  // linear algorithms. Enforce a maximum size.
+  static const int kMaxCapacity = 65535;
+
+  uint16_t size_;
+  uint16_t capacity_;
+  HUnique<T>* array_;
+
+  // Grow the size of internal storage to be at least {size} elements.
+  void Grow(int size, Zone* zone) {
+    CHECK(size < kMaxCapacity);  // Enforce maximum size.
+    if (capacity_ < size) {
+      int new_capacity = capacity_ + capacity_ + size;  // 2*current + needed

[rossberg, 2013/09/11 10:34:07]

That seems like a lot, and can cause up to a 200% space overhead. What's the
rationale?

[Toon Verwaest, 2013/09/11 11:17:57]

I guess this is 0, 1 or 4? Otherwise it seems like a lot of extra space...

+      if (new_capacity > kMaxCapacity) new_capacity = kMaxCapacity;
+      HUnique<T>* new_array = zone->NewArray<HUnique<T> >(new_capacity);
+      memcpy(new_array, array_, size_ * sizeof(HUnique<T>));
+      capacity_ = new_capacity;
+      array_ = new_array;
+    }
+  }
+};
+
+
 } }  // namespace v8::internal
 
 #endif  // V8_HYDROGEN_H_