TypeTable/TypeInfo optimization #5634
Conversation
- TypeInfo uses string_view for type name - TypeTable stores types in an array - TypeTable read access is lockless Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
|
CI MESSAGE: [18437492]: BUILD STARTED |
| return nullptr; | ||
| return &id_it->second; | ||
| return (*types)[idx]; |
There was a problem hiding this comment.
What happens when RegisterType allocated new vector of TypeInfo pointers because of idx >= type_info_map_->size() and we're asking for some type registered earlier? Won't we end up with nullptr here?
There was a problem hiding this comment.
OK, there was as insert that copied the previous vector, but I must have lost it somewhere along the way. This also shows, that this is likely untested. I'll add tests that cover adding new types.
|
CI MESSAGE: [18437886]: BUILD STARTED |
dali/pipeline/data/types.h
Outdated
| TypeInfoMap *type_info_map_ = nullptr; | ||
|
|
||
| spinlock lock_; | ||
| std::unordered_map<std::type_index, DALIDataType> type_map_; | ||
| // Unordered maps do not work with enums, | ||
| // so we need to use underlying type instead of DALIDataType | ||
| std::unordered_map<std::underlying_type_t<DALIDataType>, TypeInfo> type_info_map_; | ||
| int index_ = DALI_DATATYPE_END; | ||
| std::mutex insert_lock_; | ||
| std::list<TypeInfoMap> type_info_maps_; | ||
| std::list<TypeInfo> type_infos_; |
There was a problem hiding this comment.
Can you document the purpose of those members? Especially why not just a single vector (to avoid locking on read, right?).
|
CI MESSAGE: [18437492]: BUILD FAILED |
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
mzient
force-pushed
the
optimize_type_table
branch
from
5c32c0c
to
8dee0b5
Compare
|
CI MESSAGE: [18439899]: BUILD STARTED |
|
CI MESSAGE: [18439899]: BUILD FAILED |
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
|
CI MESSAGE: [18465196]: BUILD STARTED |
dali/pipeline/data/types_test.cc
Outdated
| IMPL_HAS_MEMBER(value); | ||
|
|
||
| template <typename T, decltype(type2id<T>::value) = {}> | ||
| void CheckBuiltinType(const TypeInfo *t) { | ||
| EXPECT_EQ(t->id(), type2id<T>::value); | ||
| EXPECT_EQ(TypeTable::GetTypeId<T>(), type2id<T>::value); | ||
| } | ||
|
|
||
| template <typename T> | ||
| void CheckBuiltinType(...) {} | ||
|
|
||
|
|
There was a problem hiding this comment.
This passes in a single-DLL scenario, but it's a valid test anyway, so let's keep it.
|
CI MESSAGE: [18465196]: BUILD FAILED |
Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>
|
CI MESSAGE: [18466402]: BUILD STARTED |
|
CI MESSAGE: [18466402]: BUILD PASSED |
Category:
Refactoring (Redesign of existing code that doesn't affect functionality)
Optimization
Description:
Additional information:
Affected modules and functionalities:
Key points relevant for the review:
Tests:
Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: DALI-3967