[llvm] r264383 - StringMap: reserve appropriate size when initializing from an initializer list

Fri Mar 25 09:28:28 PDT 2016

On Fri, Mar 25, 2016 at 9:09 AM, Mehdi Amini <mehdi.amini at apple.com> wrote:

>
> On Mar 25, 2016, at 9:01 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Fri, Mar 25, 2016 at 8:59 AM, Mehdi Amini <mehdi.amini at apple.com>
> wrote:
>
>> It would, it is just hard to write :)
>>
>> Usually the pattern is that the client side has two statements: 1 for the
>> allocation and 1 for the insertion. So you can get the number of buckets
>> between the allocation and the insertion, and verify that it is the same
>> after. Here everything happens in the constructor...
>>
>
> I was thinking you could count the moves - if the initial size is set to
> small, you should see a reallocation during the init list ctor execution,
> and you should see a bunch of move constructions. With the size set
> correctly you shouldn't see any move constructions, only copy constructions
> (in either case you should see exactly as many copy constructions as there
> are elements in the init list).
>
>
> OK, but we first need to figure the move-elision thing :(
>
> So when we start with
> (StringMap)Map.insert(std::make_pair(Twine(i).str(), CountCtorCopyAndMove()));
>

Pretty sure we have to have a move here ^ (there's a distinct object that
can't be constructed in place inside the pair)

>
> We have a first potential move to create the pair.
> Then we can have a second move when the pair is moved as a parameter to
> insert().
>
> std::pair<iterator, bool> insert(std::pair<StringRef, ValueTy> KV) {
>   return emplace_second(KV.first, std::move(KV.second));
>

std::move doesn't move anything - it just lets the lvalue be treated as an
xvalue. (it's a static cast to T&& type)

> }
>
> Here we have an explicit move (can it be legally elided or is it forced?).
>
> In emplace_second we forward (another potential move):
>
>   template <typename... ArgsTy>
>   std::pair<iterator, bool> emplace_second(StringRef Key, ArgsTy &&...
> Args) {
> ....
>     Bucket
> = MapEntryTy::Create(Key, Allocator, std::forward<ArgsTy>(Args)...);
>

std::forward doesn't move anything either - same deal as std::move (except
it's applying whatever type the parameter was, rather than necessarily
casting to an rvalue type)

>
>
> In `create`, we forward again (yet another potential move?):
>
>   template <typename AllocatorTy, typename... InitTy>
>   static StringMapEntry *Create(StringRef Key, AllocatorTy &Allocator,
>                                 InitTy &&... InitVals) {
> ....
>     // Construct the value.
>
>   new (NewItem) StringMapEntry(KeyLength, std::forward<InitTy>(InitVals)...);
>
> Finally the last move is when we construct the value in the entry:
>
>   StringMapEntry(unsigned strLen, InitTy &&... InitVals)
>
>   : StringMapEntryBase(strLen), second(std::forward<InitTy>(InitVals)...) {}
>

Then this is actually going to move.

>
>
>
> So a potential of 6 moves, but clang will perform only 3, but MSVC will
> perform 4.
>

My /guess/ is that MSVC is doing an extra move in make_pair. A test case
using emplace should produce a predictable/guaranteed number of moves
instead.

For DenseMap, since we don't have emplace there yet, you could at least use
std::pair's piecewise_construct, to ensure that no extra moves are
happening through library layers, etc.

>
>
> --
> Mehdi
>
>
>
>
> At least that's how I'm picturing it.
>
> - Dave
>
>
>>
>> --
>> Mehdi
>>
>> On Mar 25, 2016, at 7:11 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> Worth a test?
>> On Mar 24, 2016 11:02 PM, "Mehdi Amini via llvm-commits" <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: mehdi_amini
>>> Date: Fri Mar 25 00:57:47 2016
>>> New Revision: 264383
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=264383&view=rev
>>> Log:
>>> StringMap: reserve appropriate size when initializing from an
>>> initializer list
>>>
>>> From: Mehdi Amini <mehdi.amini at apple.com>
>>>
>>> Modified:
>>>     llvm/trunk/include/llvm/ADT/StringMap.h
>>>
>>> Modified: llvm/trunk/include/llvm/ADT/StringMap.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringMap.h?rev=264383&r1=264382&r2=264383&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/ADT/StringMap.h (original)
>>> +++ llvm/trunk/include/llvm/ADT/StringMap.h Fri Mar 25 00:57:47 2016
>>> @@ -233,7 +233,7 @@ public:
>>>        Allocator(A) {}
>>>
>>>    StringMap(std::initializer_list<std::pair<StringRef, ValueTy>> List)
>>> -      : StringMapImpl(static_cast<unsigned>(sizeof(MapEntryTy))) {
>>> +      : StringMapImpl(List.size(),
>>> static_cast<unsigned>(sizeof(MapEntryTy))) {
>>>      for (const auto &P : List) {
>>>        insert(P);
>>>      }
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160325/2bb32ae2/attachment.html>