Page MenuHomePhabricator

[BinaryFormat] Add MsgPackTypes
ClosedPublic

Authored by scott.linder on Jun 14 2018, 9:41 AM.

Details

Summary

Add data structure to represent MessagePack "documents" and convert to/from both MessagePack and YAML encodings.

This patch is part of a series required for AMDGPU to support it's new MessagePack metadata.

Diff Detail

Repository
rL LLVM

Event Timeline

scott.linder created this revision.Jun 14 2018, 9:41 AM

Rebase and ping

dblaikie added inline comments.Jul 30 2018, 7:28 PM
include/llvm/BinaryFormat/MsgPackTypes.h
32–35 ↗(On Diff #158010)

What's the need for this?

I suspect it's probably best to have the APIs always return unique_ptrs - which are implicitly convertible to shared_ptrs if the users of the API need that.

50 ↗(On Diff #158010)

Looks like this probably needs a virtual dtor? Unless there's something to prevent polymorphic ownership (ie: to prevent someone taking, say, a unique_ptr<ArrayNode> to a unique_ptr<Node>) - such as having a protected dtor.

unittests/BinaryFormat/MsgPackTypesTest.cpp
133 ↗(On Diff #158010)

Looks like this could be a direct value rather than dynamically allocated:

MapNode<> M;
134–137 ↗(On Diff #158010)

Drop extra parens around these RHS, so it's like this:

... = makePtr<...>(...);
scott.linder marked 2 inline comments as done.Aug 28 2018, 8:09 AM
scott.linder added inline comments.
include/llvm/BinaryFormat/MsgPackTypes.h
32–35 ↗(On Diff #158010)

The reason for this is that the methods to stream in MessagePack/YAML have to know which type to construct the document tree with. A single unique_ptr is implicitly convertible, but with a tree of them the conversions are not useful if you are trying to share parts of the tree. Unfortunately this does complicate the YAML trait specialization a bit.

50 ↗(On Diff #158010)

Thank you for the catch, fixed.

Add virtual destructor to base class and clean up tests.

dblaikie added inline comments.Aug 28 2018, 10:27 AM
include/llvm/BinaryFormat/MsgPackTypes.h
32–35 ↗(On Diff #158010)

Is there a significant win to using unique_ptr rather than using shared_ptr always, then?

& what's the sharing situation? Perhaps we could discuss that & see if there's a reasonable single ownership model that'd work.

I've updated the patch to just support shared_ptr; I think the more generic option is right for now.

I've updated the patch to just support shared_ptr; I think the more generic option is right for now.

I'd still be curious to understand the usage you're picturing that creates the requirement for shared ownership, if you could paint a bit of a picture around that, that'd be super handy!

The use I have in mind is actually an external component which depends on LLVM. We currently implement reading MessagePack metadata there, but when we land the changes to generate it in LLVM it would be nice to avoid duplicating the code there.

The rationale for a shared_ptr tree is that our API hands out opaque handles to sub-trees in the metadata which have their own lifetime independent of the root. With shared_ptr this just naturally falls out, but with unique_ptr it requires some book-keeping.

I do not see a scenario where the shared_ptr will be needed in LLVM, so if you prefer I am happy to update the patch to use unique_ptr.

The use I have in mind is actually an external component which depends on LLVM. We currently implement reading MessagePack metadata there, but when we land the changes to generate it in LLVM it would be nice to avoid duplicating the code there.

The rationale for a shared_ptr tree is that our API hands out opaque handles to sub-trees in the metadata which have their own lifetime independent of the root. With shared_ptr this just naturally falls out, but with unique_ptr it requires some book-keeping.

I do not see a scenario where the shared_ptr will be needed in LLVM, so if you prefer I am happy to update the patch to use unique_ptr.

Difficult call, but seems OK to me since you'll mostly be the only folks using the data structure anyway.

Could you include a representative unit test of this ownership situation? (can be fairly simple, but demonstrate that the root may be built, a subtree taken, then the tree destroyed and the subtree still used).

Added test for shared ownership. I've run it locally with ASan and UBSan enabled.

And thank you, this will simplify our use-case considerably :)

dblaikie accepted this revision.Sep 4 2018, 2:54 PM

Looks good - thanks

This revision is now accepted and ready to land.Sep 4 2018, 2:54 PM
This revision was automatically updated to reflect the committed changes.