[PATCH] D16597: Query the StringMap only once when creating MDString (NFC)

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Jan-26, at 14:03, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Jan 26, 2016 at 1:40 PM, Mehdi Amini <mehdi.amini at apple.com> wrote:
> 
>> On Jan 26, 2016, at 1:30 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> 
>> 
>> 
>> On Tue, Jan 26, 2016 at 12:34 PM, Mehdi AMINI via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>> joker.eph created this revision.
>> joker.eph added a reviewer: dexonsmith.
>> joker.eph added a subscriber: llvm-commits.
>> 
>> Loading IR with debug info improves MDString::get() from 19ms to 10ms.
>> 
>> http://reviews.llvm.org/D16597
>> 
>> Files:
>>   include/llvm/IR/Metadata.h
>>   lib/IR/Metadata.cpp
>> 
>> Index: lib/IR/Metadata.cpp
>> ===================================================================
>> --- lib/IR/Metadata.cpp
>> +++ lib/IR/Metadata.cpp
>> @@ -399,17 +399,12 @@
>> 
>>  MDString *MDString::get(LLVMContext &Context, StringRef Str) {
>>    auto &Store = Context.pImpl->MDStringCache;
>> -  auto I = Store.find(Str);
>> -  if (I != Store.end())
>> -    return &I->second;
>> -
>> -  auto *Entry =
>> -      StringMapEntry<MDString>::Create(Str, Store.getAllocator(), MDString());
>> -  bool WasInserted = Store.insert(Entry);
>> -  (void)WasInserted;
>> -  assert(WasInserted && "Expected entry to be inserted");
>> -  Entry->second.Entry = Entry;
>> -  return &Entry->second;
>> +  auto I = Store.insert(std::make_pair(Str, MDString()));
>> +  auto &MapEntry = I.first->getValue();
>> +  if (!I.second)
>> +    return &MapEntry;
>> +  MapEntry.Entry = &*I.first;
>> +  return &MapEntry;
>>  }
>> 
>>  StringRef MDString::getString() const {
>> Index: include/llvm/IR/Metadata.h
>> ===================================================================
>> --- include/llvm/IR/Metadata.h
>> +++ include/llvm/IR/Metadata.h
>> @@ -599,9 +599,9 @@
>> 
>>    StringMapEntry<MDString> *Entry;
>>    MDString() : Metadata(MDStringKind, Uniqued), Entry(nullptr) {}
>> -  MDString(MDString &&) : Metadata(MDStringKind, Uniqued) {}
>> 
>> Well that was weird/scary/bogus ^
> 
> Yes but private :)
> 
>>  
>> 
>>  public:
>> +  MDString(MDString &&R) : Metadata(MDStringKind, Uniqued), Entry(R.Entry) {}
>> 
>> Could we just remove this, and rely on the implicit move/copy ctors? ^ I guess maybe the base copy/move ctor doesn't do the right thing (since it's not being used here)
> 
> Good point!
> 
>> 
>> Should we just make this the copy ctor? It doesn't seme to really be moving anything.
> 
> We could consider MDString to be a “lightweight” value-based type (like StringRef) and declare a copy constructor, but will it be coherent with the rest of the other metadata types?
> 
> Not sure about the rest of them, but it seems reasonable for this one (if we're going to amke it public at all - at that point we admit that MDString is much like StringRef, as you say)
>  
> I was not really happy to make it public in the first place, but std::pair needs it to insert in the map.
> 

IIRC, the history is that after David unified the
StringMapEntry::Create() implementations, I added a bogus move
constructor to thread MDString default construction.  MDString
is not designed to be moveable, just default-constructible, but
StringMap had stopped supporting non-moveable types.  I took
advantage of encapsulation to hack something in without exposing
it to users.

It's terribly dangerous to make this constructor public, and I
don't think it makes sense to make MDString value-like (or at
least we'd need more and/or different justification than a
micro-optimization).  Metadata is pervasively compared by
address.

IMO, you should find a way to re-encapsulate this (maybe add a
friend or two), or add API to StringMap that does what you need
(ideally, in a way that supports non-moveable types so that we
don't need this hack).

(It's embarrassing for C++ that we need the pointer back to the
StringMapEntry at all (originally I left it out), but I can't
imagine that's going to show up in a heap profile.) 

> — 
> Mehdi
> 
> 
> 
> 
>>  
>>    static MDString *get(LLVMContext &Context, StringRef Str);
>>    static MDString *get(LLVMContext &Context, const char *Str) {
>>      return get(Context, Str ? StringRef(Str) : StringRef());
>> 
>> 
>> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits