This is an archive of the discontinued LLVM Phabricator instance.

ADT: Sink the guts of StringMapEntry::Create into StringMapEntryBase
ClosedPublic

Authored by dexonsmith on Jan 28 2021, 5:46 PM.

Details

Summary

Sink the interesting parts of StringMapEntry::Create into a new function
StringMapEntryBase::allocateWithKey that's only templated on the
allocator, taking the entry size and alignment as parameters.

I double-checked this doesn't impact clang's compile time performance here:
https://llvm-compile-time-tracker.com/compare.php?from=2d430f902d72b8a1d3bc036a80273ca80af1e338&to=63e5ea3bf2a2b59508e452aee6a9dc0eb369f527&stat=instructions

Diff Detail

Event Timeline

dexonsmith created this revision.Jan 28 2021, 5:46 PM
dexonsmith requested review of this revision.Jan 28 2021, 5:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 28 2021, 5:46 PM
dblaikie accepted this revision.Jan 28 2021, 8:10 PM

Sounds good - some idle optional thoughts.

llvm/include/llvm/ADT/StringMapEntry.h
30–34

Took me a minute to figure out why this was a member function - since it doesn't need access to any members, etc. Perhaps it could be a free function in a detail or implementation namespace? Not sure if we have a strong convention of how that's done in LLVM.

48–55

Guess this half could be pulled out into a non-template function, but maybe that doesn't tilt the inliner enough & hurts performance?

This revision is now accepted and ready to land.Jan 28 2021, 8:10 PM
dexonsmith added inline comments.Jan 29 2021, 8:39 AM
llvm/include/llvm/ADT/StringMapEntry.h
30–34

I have a tendency to hide things as static class members to avoid having to think of good generic names (the context of StringMapEntryBase makes allocateWithKey clear enough).

I'll think for a bit about the name for the detail/implementation namespace; feel free to recommend one; if I don't find one after a bit of thought I'll probably commit as-is.

One thought is that this could be done in a mnore generic way; MemoryBuffer.cpp has some similar code (it tail allocates the null-terminated identifier, and then also the buffer contents if owned).

48–55

Good point; I bet it's worth it.

Along similar lines, we could add a MallocAllocator overload (unlike BumpPtrAllocator::Allocate, inlining MallocAllocator::Allocate doesn't seem valuable) and define it in StringMap.cpp. I *think* this wouldn't require writing the code twice, since the overload could explicitly call the template:

void *StringMapEntryBase::allocateWithKey(
    size_t EntrySize, size_t EntryAlign, SringRef Key,
    MallocAllocator &Allocator) {
  // Reuse the templated code but instantiate it in the .cpp to avoid code
  // duplication.
  return allocateWithKey<MallocAllocator>(
      EntrySize, EntryAlign, Key, Allocator);
}

I'll look at these as follow ups.

This revision was landed with ongoing or failed builds.Apr 8 2021, 5:59 PM
This revision was automatically updated to reflect the committed changes.