Page MenuHomePhabricator

[MsgPack] New MsgPackDocument class
ClosedPublic

Authored by tpr on Jan 21 2019, 9:28 AM.

Details

Summary

A class that exposes a simple in-memory representation of a document of
MsgPack objects, that can be read from and written to MsgPack, read from
and written to YAML, and inspected and modified in memory. This is
intended to be a lighter-weight (in terms of memory allocations)
replacement for MsgPackTypes.

Two subsequent changes will:

  1. switch AMDGPU HSA metadata to using MsgPackDocument instead of MsgPackTypes;
  2. add MsgPack AMDGPU PAL metadata via MsgPackDocument.

Change-Id: Ie15a054831d5a6467c5867c064c8f8f6b80270e1

Diff Detail

Event Timeline

tpr created this revision.Jan 21 2019, 9:28 AM
tpr added a subscriber: dstuttard.

Probably best to have someone else from AMDGPU land review this for your uses there - I'm just giving some high level at-a-glance stylistic coverage here.

include/llvm/BinaryFormat/MsgPackDocument.h
238

Any chance of using unique_ptrs in this data structure so manual cleanup isn't required?

Or change it to a vector of std::string?

lib/BinaryFormat/MsgPackDocument.cpp
31

Prefer non-member overloads when possible - it ensures equal conversion handling for the left and right operands. (the operator can still be implemented as a friend, so using all the implementation details, etc)

74–78

Could you use insert (& check the bool in the pair returned from insert to determine if it was newly inserted, then do the expensive (getDocument()->getNode()) initialization only in that case) instead of find+lookup? That way the code doesn't do two map lookups in the new case, only one.

tpr updated this revision to Diff 183148.Jan 23 2019, 12:07 PM

V2: Addressed review comments.

tpr marked 3 inline comments as done.Jan 23 2019, 12:08 PM
scott.linder accepted this revision.Feb 1 2019, 11:42 AM

LGTM, sorry for the delay in reviewing this.

This changes how tags are handled and removes the ability to "ignore tags" completely, but it achieves the same result of not littering the YAML with annotations in the usual case. I can see some corner cases where a stray tag will now appear in the YAML where it would not have before, but I think your solution is cleaner.

This revision is now accepted and ready to land.Feb 1 2019, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 11:42 AM
This revision was automatically updated to reflect the committed changes.