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.

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: remove MoveWrapper #349

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Conversation

chloro-pn
Copy link
Collaborator

@chloro-pn chloro-pn commented Dec 11, 2023

Why

Close #24

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

What is changing

Example

@ChuanqiXu9
Copy link
Collaborator

Thanks for looking into this!

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

@chloro-pn
Copy link
Collaborator Author

Thanks for looking into this!

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

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

@chloro-pn chloro-pn force-pushed the remove_MoveWrapper branch 3 times, most recently from 531d8a9 to bd57dfd Compare December 11, 2023 15:30
@chloro-pn
Copy link
Collaborator Author

Thanks for looking into this!

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

done

return *static_cast<const T*>(_m_access());
}

_no_copy_types _m_unused;
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个没有用到的话,为什么要保留呢?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

因为有 alignment requirement

Comment on lines 57 to 71
class _undefined_class;

union _no_copy_types {
void* _m_object;
const void* _m_const_object;
void (*_m_function_pointer)();
void (_undefined_class::*_m_member_pointer)();
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里 _no_copy_types 里的成员完全没有被用到吧?我理解 _no_copy_types 这个类的作用就是为 sizeof 提供 to member pointer 的大小的。这种情况的话,我觉得可以把 _no_copy_types 删去,然后在需要该 size 的地方直接 sizeof(to member pointer). 然后加上注释的话会比较好看懂。

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

同上,这里是为了提供 alignment requirement和size。emm我感觉原作者这种写法是代码即注释,因为c++可调用对象就这么几种,原始函数指针、成员函数指针、functor、const functor。_no_copy_types提供了可以容纳这些指针对象的满足要求的内存区域。我后面加一下注释吧。

alignof(Functor) <= _m_max_align &&
(_m_max_align % alignof(Functor) == 0);

typedef std::integral_constant<bool, _stored_locally> _local_storage;
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里可以增加个注释:当 _local_storage 为 true 时,在 _any_data 内部存储 Functor,否则在堆上存储 Functor,在 _any_data 中存储到 Functor 的指针。

return _source._m_access<Functor*>();
}

static void _m_destroy(_any_data& _victim, std::true_type) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
static void _m_destroy(_any_data& _victim, std::true_type) {
static void _m_destroy(_any_data& _victim, /*Local Storage*/std::true_type) {

在其他类中也加上这种注释吧,对可读性帮助比较大

struct Callable : public RetTypeCheck<Res2, RetType>::type {};

template <typename T>
struct Callable<move_only_function, T> : public std::false_type {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里 Callable 改成 NotMoveOnlyCallable 比较好一些吧

return;
}
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

把这上面的声明都放到 detail namespace 感觉好一些

Copy link
Collaborator

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@ChuanqiXu9 ChuanqiXu9 merged commit 5cf47ad into alibaba:main Dec 12, 2023
14 checks passed
@RainMark
Copy link
Collaborator

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

@chloro-pn
Copy link
Collaborator Author

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

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

@ChuanqiXu9
Copy link
Collaborator

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

@RainMark
Copy link
Collaborator

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

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

@RainMark
Copy link
Collaborator

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

@chloro-pn
Copy link
Collaborator Author

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

@RainMark
Copy link
Collaborator

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

@ChuanqiXu9
Copy link
Collaborator

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

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

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

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

@chloro-pn
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@ChuanqiXu9
Copy link
Collaborator

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

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

@RainMark
Copy link
Collaborator

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

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

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

@chloro-pn
Copy link
Collaborator Author

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

@ChuanqiXu9
Copy link
Collaborator

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

@RainMark
Copy link
Collaborator

RainMark commented Dec 21, 2023

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

@ChuanqiXu9
Copy link
Collaborator

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

@chloro-pn
Copy link
Collaborator Author

done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove MoveWrapper
3 participants