[llvm-commits] Patch: SparseMultiSet

[Chandler Carruth]

Hmm, could you create a phabricator review? For a big new patch like this I
would find it much easier to review there... but maybe it's close enough
already.


On Thu, Jan 17, 2013 at 2:34 PM, Michael Ilseman <milseman at apple.com> wrote:

> 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!
>
>
>
>
> 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
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130117/5b3a48a3/attachment.html>