[llvm-commits] [PATCH] PR12156, SmallPtrMap

[Stepan Dyatkovskiy]

Ping
Stepan Dyatkovskiy wrote:
> 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
>