[llvm-commits] SmallMap class

[Chris Lattner]

Hi Stephan,

Where is the most recent version of the patch?

-Chris

On Apr 19, 2012, at 12:45 AM, Stepan Dyatkovskiy wrote:

> Hello guys. Are you still interested in SmallMap class?
> -Stepan.
> Stepan Dyatkovskiy wrote:
>> Ping.
>> Stepan Dyatkovskiy wrote:
>>> 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
>>>> 
>>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>