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

[David Blaikie via llvm-commits]

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

>
> On Mar 25, 2016, at 9:28 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> 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)
>
>
> I know about the std::move() part, but you are still passing it as an
> argument and I am not sure if you have a guarantee that no move will occur.
> Maybe the fact that the formal parameter is an rvalue guarantees no
> move-construction?
>

Right - there's no copying because the parameters are reference type.

in the same way that you know that:

void f(const Foo&);
...
Foo g;
f(g);

doesn't copy or move (& indeed, that code is valid even if the type is
non-movable/non-copyable)



>
>
>
>
>> }
>>
>> 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.
>
>
> I had the same intuition about std::make_pair.
>
> 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.
>
>
> Will we have any guarantee about the number of move using std::pair's
> piecewise_construct?
>

Yes - it's guaranteed not to do anything other than construct the objects
in-place inside the pair, no moves or copies of anything when running that
ctor (except for any created by the ctors of the subobjects, of course).
Like the example above, it works even if the types involved have no
copy/move ops.


> I was thinking that I could also create all the pairs in a vector before
> starting counting the moves during insertion. This way I would be bullet
> proof against std::pair internals.
>

Another option, sure. (that'll produce copies or moves into the data
structure, of course)


>
> --
> Mehdi
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160325/11635ac4/attachment.html>