[llvm-commits] [PATCH] PR12156, SmallPtrMap

[Stepan Dyatkovskiy]

Hi Chris. I fixed all you ask.

About MultiMapIterator.
For FlatArrayMap + DenseMap couple it is very reasonable to use 
DenseMapIterator.
I defined MultiImplMapIteratorsFactory template that presents iterator 
type itself and interface for iterators construction.
For FlatArrayMap + DenseMap it has specialization that allows to use 
DenseMapIterator directly keeping genericity of default MultiImplMap
implementation.

Just for clarity...
Code below:
    typedef MultiImplMapIterator<SmallMapTy, BigMapTy> iterator;
is replaced with these ones:
    typedef MultiImplIteratorsFactory<SmallMapTy, BigMapTy> ItFactory;
    typedef typename ItFactory::iterator iterator;
Iterator construction that was used in previous patch:
    return const_iterator(SmallMap.find(K))
is replaced with:
    return ItFactory::const_it(SmallMap, SmallMap.find(K));
and so on.
This approach allows us to optimize iterators for some special cases and 
to keep default behaviour for all others.

So, please, find reworked patch in attachment for review.

-Stepan.

Chris Lattner wrote:
>
> On Mar 30, 2012, at 12:34 AM, Stepan Dyatkovskiy wrote:
>
>> Hi all. I reworked my patch again.
>> In MultiImplMap class I added "bool DenseMapCompatible" template parameter that makes it DenseMap compatible. For DenseMapCompatible = true user should present map implementations with DenseMap specific methods, though.
>> SmallMap became special case of MultiImplMap that uses FlatArrayMap for small mode, DenseMap for big mode and that set DenseMapCompatible = true.
>
> Thank you for working on this Stepan!  This looks like great progress, though I have a strong concern about the design (stemming from the iterator, see the end).  Here are some random comments:
>
>
> Please include a patch that updates the data structures section of docs/ProgrammerManual.html.
>
>
> +//===- llvm/ADT/FlatArrayMap.h - 'Normally small' pointer set ----*- C++ -*-==//
> ...
> +// After maximum number of elements is reached, map declines any farther attempts
>
> "further attempts".  Also, please fit to 80 columns.
>
>
>
> Please add a doxygen /// comment to the FlatArrayMap class definition.
>
>
> Out of curiosity, why use:
> +  template<typename KeyTy, typename MappedTy, unsigned N>
> ...
> +    enum { ArraySize = N };
>
> instead of just renaming 'N' to 'ArraySize'?
>
> ...
> +    const_iterator begin() const {
> +      return const_iterator(Array);    }
>
> Please put } on the next line.
>
>
> +    void clear() {
> +      // Call destructor for each item and forget them.
> +      for (unsigned i = 0; i<  NumElements; ++i) {
>
> This loop will reload NumElements every time through the loop if the dtors are non-trivial, please cache it in a local variable.
>
> +//===- llvm/ADT/SmallMap.h - 'Normally small' pointer set ----*- C++ -*-===//
>
> Please fill this out to exactly 80 columns.
>
> +++ include/llvm/ADT/MultiImplMap.h	(revision 0)
>
> +    void moveAllToBigMap() {
> +        BigMap.insert(SmallMap.begin(), SmallMap.end());
> +        SmallMap.clear();
> +    }
>
> This should just be inlined into its single caller.  If you really want it out of line, please indent by 2, not 4.
>
>
> +    MultiImplMap(const self&  other) {
> +      UseSmall = other.UseSmall;
> +      if (UseSmall)
> +        SmallMap = other.SmallMap;
> +      else
> +        BigMap = other.BigMap;
> +    }
>
> Shouldn't this check the size of "other" and use SmallMap if it can?  If "other" started grew to "large" size, then had most of its elements removed (so that it could fit in "small") then was copied, it would make sense to start the copy out small.
>
>
> +    void clear() {
> +      if (UseSmall)
> +        SmallMap.clear();
> +      else
> +        BigMap.clear();
> +    }
>
> In contrast to the copy ctor, this should stay the way it is.  Clearing a densemap doesn't free its buckets.  Please add a comment to this effect so that it is clearly intentional.
>
>
> +    void swap(MultiImplMap&  rhs) {
> +}
>
> Why not just define this as:
>
>    SmallMap.swap(rhs.SmallMap);
>    BigMap.swap(rhs.BitMap);
>
> with no cases in it?
>
>
> +    self&  operator=(const self&  other) {
>
> Similar to the 'clear' method, switching back to small doesn't help you.  If 'this' is already big, stay big.
>
>
>
> +  template<class SmallMapTy, class BigMapTy, bool IsConst>
> +  class MultiImplMapIterator {
>
> I'm not a big fan of this class: it is going to be really inefficient to copy around and deal with, particularly because so many of the MultiImplMap methods are defined in terms of it.  Can't we do something trickier if we drop the genericity of the underlying Array/Map classes?  A densemap is just an array of buckets, exactly like the small case.  If SmallMap took advantage of that, the iterator would be *much much* more efficient.
>
> -Chris

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smallptrmap-3.patch
Type: text/x-patch
Size: 36003 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120406/ffdbbfcf/attachment.bin>