Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy attachments buffer when parsing summary [20078] #100

Merged
merged 2 commits into from
Dec 12, 2023
Merged

Copy attachments buffer when parsing summary [20078] #100

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c941084
Copy attachments buffer when parsing summary
juanlofer-eprosima Nov 30, 2023
91da985
Apply suggestion
juanlofer-eprosima Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions thirdparty/mcap/mcap/reader.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,7 @@ class MCAP_PUBLIC McapReader final {
bool parsedSummary_ = false;

void reset_();
void clear_attachments_();
Status readSummarySection_(IReadable& reader);
Status readSummaryFromScan_(IReadable& reader);
};
Expand Down
18 changes: 15 additions & 3 deletions thirdparty/mcap/mcap/reader.inl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,6 +351,7 @@ void McapReader::reset_() {
statistics_ = std::nullopt;
chunkIndexes_.clear();
attachmentIndexes_.clear();
clear_attachments_();
schemas_.clear();
channels_.clear();
dataStart_ = 0;
Expand All @@ -360,6 +361,13 @@ void McapReader::reset_() {
parsedSummary_ = false;
}

void McapReader::clear_attachments_() {
for (auto& [attachment_name, attachment] : attachments_) {
std::free((void*)attachment.data);
}
attachments_.clear();
}

Copy link
Contributor

@Tempate Tempate Dec 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function .clear() calls the destructor of each element inside the map. Wouldn't it make more sense to override the destructor of the Attachment object to free attachment.data? In that way you would avoid having a clear_attachments_ function and simply call attachments_.clear() like with every other object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that would be valid, because elsewhere (every place other than when destroying our added map) it would be calling a free that is already called somewhere else (there is no way of checking if already freed easily, perhaps adding an attribute, but too involved and risky).

Another option would be to inherit from Attachment and add that destructor only there (and have a map of children). I would not go for that, our changes to the mcap library should be minimal.

Status McapReader::readSummary(ReadSummaryMethod method, const ProblemCallback& onProblem) {
if (!input_) {
const Status status{StatusCode::NotOpen};
Expand Down Expand Up @@ -427,7 +435,7 @@ Status McapReader::readSummarySection_(IReadable& reader) {
}

attachmentIndexes_.clear();
attachments_.clear();
clear_attachments_();
metadataIndexes_.clear();
metadata_.clear();
chunkIndexes_.clear();
Expand Down Expand Up @@ -488,7 +496,7 @@ Status McapReader::readSummaryFromScan_(IReadable& reader) {
schemas_.clear();
channels_.clear();
attachmentIndexes_.clear();
attachments_.clear();
clear_attachments_();
metadataIndexes_.clear();
metadata_.clear();
chunkIndexes_.clear();
Expand All @@ -503,7 +511,11 @@ Status McapReader::readSummaryFromScan_(IReadable& reader) {
typedReader.onAttachment = [&](const Attachment& attachment, ByteOffset fileOffset) {
AttachmentIndex attachmentIndex{attachment, fileOffset};
attachmentIndexes_.emplace(attachment.name, attachmentIndex);
attachments_.emplace(attachment.name, attachment);
// Copy buffer for storage as original is freed when leaving scope
Attachment attachment_copy = attachment;
attachment_copy.data = (std::byte*)std::malloc(attachment.dataSize);
std::memcpy((void*)attachment_copy.data, attachment.data, attachment.dataSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the (void *) casting necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that is the case, I suppose I added it for a reason.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. It's required.

attachments_.emplace(attachment_copy.name, attachment_copy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider having std::move(attachment_copy) to avoid copying the attachment_copy unnecessarily when emplicing it in the attachment_.

};
typedReader.onMetadata = [&](const Metadata& metadata, ByteOffset fileOffset) {
MetadataIndex metadataIndex{metadata, fileOffset};
Expand Down
Loading