Page MenuHomePhabricator

Save a word in every StringSet entry
ClosedPublic

Authored by jordan_rose on Oct 7 2019, 11:40 AM.

Details

Summary

Add a specialization to StringMap (actually StringMapEntry) for a value type of NoneType (the type of llvm::None), and use it for StringSet. This'll save us a word from every entry in a StringSet, used for alignment with the size_t that stores the string length.

I could have gone all the way to some kind of empty base class optimization, but that seemed like overkill.

Diff Detail

Event Timeline

jordan_rose created this revision.Oct 7 2019, 11:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 11:40 AM

Hm, doesn't quite work yet but I'll get there.

Fixed bad uses of StringSet, changed a friend from StringMapEntry to StringMapEntryStorage. The fact that I only had to do this in one place (and that one place is definitely doing something tricky) makes me still feel confident enough to make this change.

dblaikie accepted this revision.Oct 10 2019, 11:59 AM

Looks good to me - thanks!

If you like, maybe leave a comment/hint that this could be generalized to a full EBO if someone has a need in the future.

Any idea why MDString is friending an implementation detail like this? Should it be? Could we make it an actual private implementation detail so people can't do this?

This revision is now accepted and ready to land.Oct 10 2019, 11:59 AM

Any idea why MDString is friending an implementation detail like this? Should it be? Could we make it an actual private implementation detail so people can't do this?

It's funky but I think reasonable: MDString is a move-none type since there will be direct pointers to it, and the canonical instance lives in the StringMap. Disallowing this friending would mean making MDString move-only and just assuming it'll never be misused.

jordan_rose closed this revision.Oct 10 2019, 1:20 PM

Committed in rL374440. I split the difference and put the EBO comment in the commit message.

Any idea why MDString is friending an implementation detail like this? Should it be? Could we make it an actual private implementation detail so people can't do this?

It's funky but I think reasonable: MDString is a move-none type since there will be direct pointers to it, and the canonical instance lives in the StringMap. Disallowing this friending would mean making MDString move-only and just assuming it'll never be misused.

Figured something like that. I think for standard containers, at least, that can now be achieved through a custom allocator ( https://en.cppreference.com/w/cpp/memory/allocator/construct - not sure why the non-default version was deprecated... well, one possible reason (below))

The other way to do it is with a private tagged parameter - have a private type in MDString, have a public ctor that takes an instance of that type as a parameter, and then no public user can call it because they can't name the type, but MDString's implementation can call into other code including templates that can handle that.