[llvm-commits] Patch: SparseMultiSet

[Michael Ilseman]

New patch. I have the iterators save an index into the sparse array so they can re-issue a find when they're end-decremented. This means that they're no longer bi-modal. I fixed the other suggestions and added more test coverage.

Thanks!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: sparseMultiSet.patch
Type: application/octet-stream
Size: 34391 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130117/caf914fe/attachment.obj>
-------------- next part --------------


On Jan 17, 2013, at 10:45 AM, Michael Ilseman <milseman at apple.com> wrote:

> 
> On Jan 17, 2013, at 10:15 AM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
> 
>> 
>> On Jan 16, 2013, at 10:39 AM, Michael Ilseman <milseman at apple.com> wrote:
>> 
>>> Attached is a patch introducing a new data structure, the SparseMultiSet, and changes to the MI scheduler to use it.
>>> 
>>> A SparseMultiSet adds multiset behavior to SparseSet, while retaining SparseSet's desirable properties. Essentially, SparseMultiSet provides multiset behavior by storing its dense data in doubly linked lists that are inlined into the dense vector. This allows it to provide good data locality as well as vector-like constant-time clear() and fast constant time find(), insert(), and erase(). It also allows SparseMultiSet to have a builtin recycler rather than keeping SparseSet's behavior of always swapping upon removal, which allows it to preserve more iterators. It's often a better alternative to a SparseSet of a growable container or vector-of-vector.
>> 
>> +    return (Dense.begin() + N.Prev) == &N;
>> 
>> AFAICT, this assumes that begin() is a pointer, but it could be some iterator class.
>> 
> 
> True, I'll do (&Dense[0] + N.Prev) == &N; This keeps the requirement that it be laid out in contiguous memory, but removes the dependence on pointer-iterators.
> 
>> +    const iterator_base &operator=(const iterator_base &RHS) {
>> +      Dense = RHS.Dense;
>> 
>> Dense is a reference, so this assigns the whole array. Please make sure you have a test case that catches this.
>> 
> 
> Good catch, I'll change to a pointer rather than a reference.
> 
>> +    /// Increment and decrement operators
>> +    iterator_base &operator--() { // predecrement - Back up
>> +      assert(isValid() && "Decrementing an invalid iterator");
>> +
>> +      // If we're at the end, then interpret our index as a previous index
>> +      if (IsEnd)
>> +        IsEnd = false;
>> 
>> How do you know Idx is still valid here? Please add a test that decrements end() after erasing elements.
>> 
> 
> You're right, to ensure that end iterators are still valid post tail erasure, I'll have to re-issue a find on end decrement.
> 
>> +  std::pair<iterator,iterator> equal_range(const KeyT &K) {
>> +    iterator I = find(K);
>> +    if (I == end())
>> +      return make_pair(I, I);
>> +
>> +    iterator B = I--;
>> +    return make_pair(B, ++I);
>> +  }
>> 
>> This looks very odd. I don't think the iterator class should expose to clients that it is a circular linked list. It would be better is --I on find() were an assertion. Use private methods to access the linked list.
>> 
> 
> Will do.
> 
>> It looks like you need better test coverage for all the corner cases.
>> 
> 
> Will do.
> 
>> /jakob
>> 
> 
> Thanks!
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits