[llvm-commits] SmallMap class

[Stepan Dyatkovskiy]

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