alibaba / async_simple

Simple, light-weight and easy-to-use asynchronous components
Apache License 2.0
1.69k stars 251 forks source link

refactor: remove MoveWrapper #349

Closed chloro-pn closed 11 months ago

chloro-pn commented 11 months ago

Why

Close #24

参考gcc11.1.0 std::function的一个move_only_function实现(简化版本,非标准接口)

What is changing

Example

ChuanqiXu9 commented 11 months ago

Thanks for looking into this!

还挺复杂的,一下子不太看得完。先看下这里几个 CI 失败的 case?

chloro-pn commented 11 months ago

Thanks for looking into this!

还挺复杂的,一下子不太看得完。先看下这里几个 CI 失败的 case?

嗯嗯,这个不急可以看仔细点。我明天修一下失败的CI,看了下应该是不同编译器的兼容性问题。

chloro-pn commented 11 months ago

Thanks for looking into this!

还挺复杂的,一下子不太看得完。先看下这里几个 CI 失败的 case?

done

RainMark commented 11 months ago

感谢,可能还需要修改executor的schedule参数std::function 为move only版本

chloro-pn commented 11 months ago

感谢,可能还需要修改executor的schedule参数std::function 为move only版本

这个改倒是能改,不过executor的Func不是公开接口么,这样能保证前向兼容嘛

ChuanqiXu9 commented 11 months ago

是的,我也感觉避免修改公开接口好一些

RainMark commented 11 months ago

但是这样去掉MoveWrapper就有点问题了,如果schedule的function捕获了没有copy构造函数的object就编译不过了。之前还可以用MoveWrapper绕,现在就没了

不兼容的问题是不是可以让move only fucntion支持从std::function 隐私构造就行

RainMark commented 11 months ago

毕竟MoveWrapper的出现就是解决这个问题,现在关键问题没解就去掉MoveWrapper的话反而是不兼容的

chloro-pn commented 11 months ago

这个问题,首先要明确一下MoveWrapper是否属于公开接口,如果不是那么就不对用户提供不会修改/去除的保证,也就是说之前async_simple就不提供内置的支持调度move_only functor的功能。 如果是,那么当初计划remove MoveWrapper就是不兼容的改动,一个新版本。这个新版本需要提供代替MoveWrapper的工具,这样的话可以像你说的支持从std::function隐式构造(不前向兼容),或者新加一个接口(前向兼容)。

RainMark commented 11 months ago

是向用户提供的,但是这种使用方式不友好。所以期望用新的方式替代掉。直接删除的MoveWrapper肯定是对用户不兼容的,但是需要有替代方案。发布版本可能需要说明这一点

ChuanqiXu9 commented 11 months ago

但是这样去掉MoveWrapper就有点问题了,如果schedule的function捕获了没有copy构造函数的object就编译不过了。之前还可以用MoveWrapper绕,现在就没了

hmmm, 我没太 get 到这点,现在 move_only_function 本来就不会掉用 copy ctor 吧。

之前引入 MoveWrapper 的原因是 std::function 要求 std::decay::type 是 copy constructable 的,但我们情况不满足才提供的 move wrapper。我理解引入 move_only_function 之后就没这问题了

MoveWrapper 不是对用户暴露的接口,我看了下应该不会有 break change 吧?

chloro-pn commented 11 months ago

emm..RainMark的意思是之前schedule的参数是std::function,加上MoveWrapper可以支持调度move_only functor,但是现在schedule的接口没有改,MoveWrapper被删了,且不管兼不兼容的问题,现在是没有办法调用move_only functor的(因为move_only_function不能用来构造std::function,同时这也意味着move_only_function也不是提供给用户的接口,仅是为了保证内部实现更优雅。)

两位现在一个是对MoveWrapper的定位不统一(公开接口or非空开接口,这涉及到向前兼容的问题),一个是对schedule的接口定位不统一,async_simple是否支持调度用户提供的move_only functor? 我有点凌乱了。。需要同步一下。反正我这边改起来都挺方便的。

ChuanqiXu9 commented 11 months ago

emm..RainMark的意思是之前schedule的参数是std::function,加上MoveWrapper可以支持调度move_only functor,但是现在schedule的接口没有改,MoveWrapper被删了,且不管兼不兼容的问题,现在是没有办法调用move_only functor的(因为move_only_function不能用来构造std::function,同时这也意味着move_only_function也不是提供给用户的接口,仅是为了保证内部实现更优雅。)

我还是没太 get 到点,为什么现在不能掉用 move only 的 functor 呢?或者说这个 patch 唯一改动的地方在 FutureState 里,并没有 move only functor 构造 std::function 的动作。或者说有一个具体的例子是之前能编译过的,但现在无法编译通过的?

RainMark commented 11 months ago
std::unique_ptr<T> p;
executor->schedule([p = std::move(p)]() {
 p->xxx();
});

比如这种现在就编译不过,需要改成

std::unique_ptr<T> p;
executor->schedule([w = MoveWrapper(std::move(p))]() {
 w.get()->xxx()
});
ChuanqiXu9 commented 11 months ago
std::unique_ptr<T> p;
executor->schedule([p = std::move(p)]() {
 p->xxx();
});

hmmm, 这个代码原来就编译不过吧?

比如这种现在就编译不过,需要改成

std::unique_ptr<T> p;
executor->schedule([w = MoveWrapper(std::move(p))]() {
 w.get()->xxx()
});

MoveWrapper 原来不属于 async_simple 暴露的一部分吧,所以理论上我们也不用对这种写法负责?

RainMark commented 10 months ago

其实都在用,MoveWrapper就是解决原来那种编译不过的case,要不然的话其实不用实现MoveWrapper的

ChuanqiXu9 commented 10 months ago

OK,那这种属于设计初衷与最终实践不一致的问题吧。因为 MoveWrapper 从来没在文档和测试用例里出现过,就容易出现这种情况。

我觉得我们可以在 Executor 里加一个 MoveWrapper 类,来补充这种用法吧。

RainMark commented 10 months ago

OK,那这种属于设计初衷与最终实践不一致的问题吧。因为 MoveWrapper 从来没在文档和测试用例里出现过,就容易出现这种情况。

我觉得我们可以在 Executor 里加一个 MoveWrapper 类,来补充这种用法吧。

嗯,之前主要在indexlib里在用。确实没有在lib里写单测。。 比如这里:https://github.com/alibaba/havenask/blob/main/aios/storage/indexlib/file_system/fslib/FslibCommonFileWrapper.cpp#L216

chloro-pn commented 10 months ago

https://github.com/alibaba/async_simple/pull/352 这样怎么样,恢复MoveWrapper.h保证没有break change Executor加一个新的接口,schedule_move_only 。实现就是将move_only_function用MoveWrapper转发到schedule

ChuanqiXu9 commented 10 months ago

个人感觉对外暴露的 MoveWrapper 有点丑,更倾向只暴露 schedule_move_only 接口。 @RainMark

RainMark commented 10 months ago

嗯,我也倾向于不对外暴露MoveWrapper。不过直接删除了确实不兼容之前代码。既然提供了schedule_move_only后,升级版本后业务就改成schedule_move_only就好了。要不先保留一个版本标记废弃。然后再下次就删了?

ChuanqiXu9 commented 10 months ago

我感觉这样也可以,可以先给 MoveWrapper 加一个 deprecated 属性。

chloro-pn commented 10 months ago

done.