refactor(dipu): add static_value_array and simplify code #920
Conversation
wiryls
commented
Jul 26, 2024
•
wiryls
commented
- 添加了一个小工具用于生成数组，用它简化了一些代码，不再需要从 1 遍历到 16 去查找对应的 static 变量。
- 修复了清空 event pool 时，只清空当前 device 的问题。
- 不过这个修复也不太改好，它会导致全部 event pool 被初始化，还好初始化过程基本没有副作用
| } | ||
| }; | ||
|
|
||
| EventPool<deviceEvent_t>* getEventPool() { | ||
| const int index = devproxy::current_device(); |
There was a problem hiding this comment.
我是觉得 event pool 不应该调用 devproxy 命名空间的方法，所以把它移到外面了
| } | ||
|
|
||
| void releaseAllEvent() { getEventPool()->release(); } | ||
| void event_pool_clear() { | ||
| for (auto& pool : EventPoolHolderArray::value) { |
There was a problem hiding this comment.
这个改法欠佳（导致全部 pool 都被初始化），但没想到更好同时又不麻烦的方法了
| EventPool(const EventPool&) = delete; | ||
| EventPool(EventPool&&) = delete; | ||
| EventPool& operator=(const EventPool&) = delete; | ||
| EventPool& operator=(EventPool&&) = delete; |
There was a problem hiding this comment.
有锁作为成员，EventPool 自动变得无法拷贝和移动，所以我都删掉了
wiryls
changed the title
| @@ -6,10 +6,8 @@ | |||
|
|
|||
| namespace dipu { | |||
|
|
|||
| void getEventFromPool(deviceEvent_t& event); | |||
|
|
|||
There was a problem hiding this comment.
其实原来的驼峰也没哈问题，真没必要都改成下划线，纯个人习惯问题。。。
| dipu::devapis::createEvent(&event); | ||
| } | ||
|
|
||
| void release(dipu::deviceEvent_t& event) { |
| void restoreEventToPool(deviceEvent_t& event) { | ||
| getEventPool()->restore(event); | ||
| void event_pool_release(int index, deviceEvent_t& event) { | ||
| event_pool(index).release(event); |
There was a problem hiding this comment.
参数加了 index， 上下层的职责划分也比原来明确些。
| @@ -35,3 +35,7 @@ const auto DIPU_BACKEND_SPARSE_TYPE = | |||
| const auto DICL_BACKEND_NAME = "dicl"; | |||
|
|
|||
| } // namespace dipu | |||
|
|
|||
| namespace dipu { | |||
| auto inline constexpr kMaxDeviceNumber = 16; | |||
There was a problem hiding this comment.
我当时是看到两边命名风格不一样，所以隔开了一下
| @@ -0,0 +1,53 @@ | |||
| #pragma once | |||
|
|
|||
There was a problem hiding this comment.
我也在犹豫放哪里，有没有啥规范或者惯例，还有就是文件名字有没有更好的
| struct make_static_value_array<T, std::index_sequence<I...>> { | ||
| using value_type = std::decay_t<decltype(T::template value<0>)>; | ||
| using type = std::array<value_type, sizeof...(I)>; | ||
| type static inline constexpr value{T::template value<I>...}; |
There was a problem hiding this comment.
static inline constexpr 为啥要放到 type 后面？ 这是啥规范， 我咋觉得 放到前面更容易懂？
There was a problem hiding this comment.
这里是个人习惯，C++ 的修饰词太多了，auto、static、inline、constexpr、const、thread_local……通常来说我就习惯写成“类型+其它类型标识”的风格，例如“char const *” 这样。
有个好处是“从右往左”看时顺序非常清晰，比如这种情况：char const * const。能很清楚知道最右侧 const 是修饰左边的 char const *，而左侧的 const 只修饰 char
| struct make_static_value_array<T, std::index_sequence<I...>> { | ||
| using value_type = std::decay_t<decltype(T::template value<0>)>; | ||
| using type = std::array<value_type, sizeof...(I)>; | ||
| type static inline constexpr value{T::template value<I>...}; |
There was a problem hiding this comment.
类内定义方法和非特化模板函数都是隐式 inline，不需要加，下面也有几个 case 不重复提了
There was a problem hiding this comment.
这里如果不写 inline，编译器就会要求这里只声明，需要在类外定义静态成员变量，否则违反 odr
| @@ -0,0 +1,53 @@ | |||
| #pragma once | |||
|
|
|||
| namespace dipu { | ||
|
|
||
| template <typename T, typename S> | ||
| struct make_static_value_array {}; |
| static AsyncMemPoolImpl async_mem_pool; | ||
| static AllocatorImpl cache_allocator; | ||
| static int n = [&]() { | ||
| cache_allocator.set_raw_allocator(raw_allocator); |
There was a problem hiding this comment.
这个 precondition 好强啊，没有任何警示吗
There was a problem hiding this comment.
这块没想到怎么改合适……索性直接复制之前的实现
