Page MenuHomePhabricator

Query the StringMap only once when creating MDString (NFC)
ClosedPublic

Authored by mehdi_amini on Mar 6 2016, 2:28 PM.

Details

Summary

Loading IR with debug info improves MDString::get() from 19ms to 10ms.
This is a rework of D16597 with adding an "emplace" method on the StringMap
to avoid requiring the MDString move ctor to be public.

Diff Detail

Event Timeline

mehdi_amini updated this revision to Diff 49925.Mar 6 2016, 2:28 PM
mehdi_amini retitled this revision from to Query the StringMap only once when creating MDString (NFC).
mehdi_amini updated this object.
mehdi_amini added a reviewer: dexonsmith.
mehdi_amini added a subscriber: llvm-commits.
dexonsmith edited edge metadata.Mar 6 2016, 5:03 PM
dexonsmith added a subscriber: dexonsmith.

+dblaikie

If you rename the method emplace_second (since you're only emplacing the
second element, not the full value), then I'm happy with this. However,
David didn't like this patch when I proposed it a year or so ago.

IIRC, at the time David wanted a std::pair-style emplace in
StringMapEntry complete with support for std::piecewise_construct_t. Then

you could call:

auto I = Store.emplace(std::piecewise_construct,
                       std::make_tuple(Str),
                       std::make_tuple());

I still think emplace_second is sufficiently clear since StringMap is so
specialized (and the above looks like boilerplate to me), but this would
be an uncontentious path forward.

mehdi_amini edited edge metadata.

Rename emplace() into emplace_second()

Separate unit test for the StringMap changes might be nice.

Could you remind me what the performance problem was that this is saving? Copying the empty MDString into the StringMapEntry? (what's in an MDString that makes it expensive to copy?) Oh it's the extra map lookup, I see.

std::unordered_map::emplace returns the classic <iterator, bool> pair. Perhaps emplace_second should do the same? Rather than relying on being able to detect the invalid default constructed state of the key?

include/llvm/IR/Metadata.h
594–595

Why do we need a move ctor at all if we're supporting emplace? Can we just delete it? (even if we omit it, it should be removed implicitly - same with the move assignment above, I think)

std::unordered_map::emplace returns the classic <iterator, bool> pair. Perhaps emplace_second should do the same? Rather than relying on being able to detect the invalid default constructed state of the key?

I don't understand what you mean: std::pair<iterator, bool> emplace_second(StringRef Key, ArgsTy &&... Args)

include/llvm/IR/Metadata.h
594–595

StringMap requires movable types (for std::pair I think).

Add a test for emplace_second

Remove the need for movable type in StringMap (had to remove an overload for Create())

dblaikie added inline comments.Mar 22 2016, 1:23 PM
unittests/ADT/StringMapTest.cpp
366

Sign of the parameter doesn't match the sign of the member (& we usually write "unsigned int" as just "unsigned" I think?)

402

Can we do it with a type that cannot be copied or moved? (ie: is this a static property, or a dynamic one?)

Use a NonMoveableNonCopyableType with the StringMap.

LGTM once David is happy.

dblaikie accepted this revision.Mar 22 2016, 3:09 PM
dblaikie added a reviewer: dblaikie.

Looks good - thanks for your patience

This revision is now accepted and ready to land.Mar 22 2016, 3:09 PM