-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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.
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
Refactor shared memory data layout to use named identifiers #4975
Conversation
c10eded
to
76f88a4
Compare
99976aa
to
21f759e
Compare
cd326db
to
995a09a
Compare
995a09a
to
00395bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯 looks good!
include/storage/shared_datatype.hpp
Outdated
class DataLayout | ||
{ | ||
public: | ||
enum BlockID | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
src/storage/storage.cpp
Outdated
@@ -402,15 +313,15 @@ void Storage::PopulateData(const DataLayout &layout, char *memory_ptr) | |||
{ | |||
const auto name_blocks_ptr = | |||
layout.GetBlockPtr<extractor::NameTableView::IndexedData::BlockReference, true>( | |||
memory_ptr, DataLayout::NAME_BLOCKS); | |||
memory_ptr, "/common/names/blocks"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good to move initialization into a free function as
return util::vector_view<BlockT> (layout.GetBlockPtr<BlockT, true>(memory_ptr, name), layout.GetBlockEntries(name));
to avoid duplication of type-name pairs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh great idea. 👍
include/storage/shared_datatype.hpp
Outdated
inline std::string trimName(const std::string &name_prefix, const std::string &name) | ||
{ | ||
// list directory and | ||
if (name_prefix.back() == '/') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!name_prefix.empty() && ...
@oxidase Could you review this again? I refactored creating the views so it can be shared by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I don't think that canary blocks are needed since layout and data blocks were merged into a single one. imho ✂️ is 🆗
Issue
This works towards #4952 by using the string identifiers everywhere to reference to the data. This makes
DataLayout
quite "stupid" as it does not know about the data anymore and can have a variable number of entries.Tasklist
Requirements / Relations
This PR is based on #4960.