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

cop: Support variable number of arguments #5007

Merged
merged 8 commits into from Jul 3, 2019

Conversation

Projects
None yet
3 participants
@breeswish
Copy link
Member

commented Jul 2, 2019

What have you changed? (mandatory)

This PR adds vargs support to the RPN framework.

It also includes the implementation of coalesce_xxx as a demonstration of the vargs support.

What are the type of the changes? (mandatory)

  • New feature (change which adds functionality)

How has this PR been tested? (mandatory)

New unit tests.

breeswish added some commits Jun 28, 2019

rename `func` to `func_meta`
Signed-off-by: Breezewish <breezewish@pingcap.com>
Support vargs
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

/run-integration-tests

@breeswish breeswish requested review from sticnarf and lonng Jul 2, 2019

/// The accepted argument length of this RPN function.
///
/// Currently we do not support variable arguments.
pub args_len: usize,

This comment has been minimized.

Copy link
@sticnarf

sticnarf Jul 2, 2019

Contributor

The args_len in RpnFnMeta was designed for validation. Now, if I understand it correctly, it's going to be included in the (TODO) validator?

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 3, 2019

Author Member

Yes.

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 3, 2019

Author Member

And the validator shall also check eval types. The validator should be auto generated in the #[rpn_fn] according to arg_type and ret_type in the NormalRpnFn and VargsRpnFn.

};
let ret = (func.fn_ptr)(context, &call_info)?;
let ret = (func_meta.fn_ptr)(

This comment has been minimized.

Copy link
@sticnarf

sticnarf Jul 2, 2019

Contributor

These arguments are passed fairly deep in stacks. So it can be expensive to copy these arguments between stack frames if the compiler is not smart enough. Perhaps we'd better still use a payload struct to wrap them all.

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 3, 2019

Author Member

I changed it because according to AMD64 ABI these parameters should be kept in the registers when the function is invoked. Also if there is demand to store these parameters onto stack, a single pushq should be enough for each parameter. Notice that this may behave differently compared to the RpnFnCallPayload previously since RpnFnCallPayload is a integral structure while compiler is very likely to generate simple mov instructions for these scattered parameters.


impl VargsRpnFn {
pub fn new(attr: RpnFnAttr, item_fn: ItemFn) -> Result<Self> {
if item_fn.decl.inputs.len() != attr.captures.len() + 1 {

This comment has been minimized.

Copy link
@sticnarf

sticnarf Jul 3, 2019

Contributor

Are we assuming that vargs functions only have arguments of the same type? Will there be functions like foo(Int, Real, &[Decimal])? Perhaps we can unify normal and vargs functions.

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 3, 2019

Author Member

Yes. I took a look at existing varg functions and they accept same argument types. We can add support for the multiple type varg function later if they exist. The change should be pretty simple, the signature is going to be &[ScalarValueRef] and there will be no performance difference. However I didn't choose this signature since this kind of flexibility is unnecessary, while on the other hand a signature like &[&Option<T>] looks to be very clear.

breeswish added some commits Jul 3, 2019

Address comments
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

@H-ZeX You can also review this PR :)

if let Expr::Array(ExprArray { elems, .. }) = *right {
captures = elems.into_iter().collect();
} else {
return Err(Error::new_spanned(right, "Expect array expression"));

This comment has been minimized.

Copy link
@lonng

lonng Jul 3, 2019

Contributor
Suggested change
return Err(Error::new_spanned(right, "Expect array expression"));
return Err(Error::new_spanned(right, "Expect array expression for `capture`"));
match left_str.as_ref() {
"capture" => {
if let Expr::Array(ExprArray { elems, .. }) = *right {
captures = elems.into_iter().collect();

This comment has been minimized.

Copy link
@lonng

lonng Jul 3, 2019

Contributor

Should we check the captures? e.g: Duplicated items in #[rpn_fn(varg, captures=[ctx,ctx,output_rows] should be removed.

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 3, 2019

Author Member

It could be a valid syntax if user defined function accept duplicate arguments. The framework won't intervene.


/// Parses a function signature parameter like `val: &Option<T>`.
struct RpnFnSignatureParam {
_pat: Pat,

This comment has been minimized.

Copy link
@lonng

lonng Jul 3, 2019

Contributor

Why not remove this field?

This comment has been minimized.

Copy link
@breeswish

breeswish Jul 3, 2019

Author Member

Maybe useful in future, removing it looks to be wired because it accepts much info but only stores part of it.

breeswish added some commits Jul 3, 2019

Address comments
Signed-off-by: Breezewish <breezewish@pingcap.com>
@breeswish

This comment has been minimized.

Copy link
Member Author

commented Jul 3, 2019

/run-integration-tests

@lonng

lonng approved these changes Jul 3, 2019

@lonng

lonng approved these changes Jul 3, 2019

@sticnarf
Copy link
Contributor

left a comment

LGTM

@breeswish breeswish merged commit 1853b95 into tikv:master Jul 3, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details

@breeswish breeswish deleted the breeswish:__batch/dyn_args branch Jul 3, 2019

breeswish added a commit to breeswish/tikv that referenced this pull request Jul 9, 2019

Cherry pick tikv#5007
Signed-off-by: Breezewish <breezewish@pingcap.com>

breeswish added a commit to breeswish/tikv that referenced this pull request Jul 9, 2019

Cherry pick tikv#5007
Signed-off-by: Breezewish <breezewish@pingcap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.