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

feat: Add Delegate type #1059

Merged
merged 6 commits into from
Nov 9, 2021
Merged

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Oct 29, 2021

This type is intended as a sort-of replacement of std::function without the virtual function overhead of that type. This is ultimately meant to support the new extension mechanism for the KF and CKF, where these delegates will be used for the calibrator, smoother, and so on.

Delegate type that allows type erasure of a callable without allocation and with a single level of indirection. This type can support:

  • a free function pointer
    • a pointer to a member function alongside an instance pointer
      Note: Delegate does not assume ownership of the instance. You need to ensure that the lifetime of the callable instance is longer than that of the @c Delegate.
      Currently Delegate only supports callables that are const.

The usage of this type looks like this for a free function:

int sumImplementation(int a, int b) {
  return a + b;
}

Delegate<int(int, int)> sum;
sum.connect<&sumImplementation>();

int c = sum(2, 2); // = 4

and like this for a member pointer with instance:

struct Subtractor {
  int x;

  int subtract(int v) const {
    return v - x;
  }
};

Delegate<int(int)> subtract;

Subtractor instance{5};

subtract.connect<&Subtractor::subtract>(&instance);

int y = subtract(10); // = 5

@paulgessinger paulgessinger added this to the next milestone Oct 29, 2021
@codecov
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

Merging #1059 (7a97db2) into main (2e48e5c) will increase coverage by 0.04%.
The diff coverage is 85.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1059      +/-   ##
==========================================
+ Coverage   48.54%   48.58%   +0.04%     
==========================================
  Files         339      340       +1     
  Lines       17380    17400      +20     
  Branches     8221     8224       +3     
==========================================
+ Hits         8437     8454      +17     
  Misses       3190     3190              
- Partials     5753     5756       +3     
Impacted Files Coverage Δ
Core/include/Acts/Utilities/Delegate.hpp 85.00% <85.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e48e5c...7a97db2. Read the comment docs.

@timadye
Copy link
Contributor

timadye commented Nov 2, 2021

I've also been playing with this in Godbolt. First a couple of minor tweaks:

  1. Delegate.hpp needs #include <cassert>
  2. in your example above (and your slides) need to declare Subtractor::subtract() const (took me a long time to work that one out!).

I compared Delegate with a couple of implementations using std::function.

  1. With GCC 11, Delegate generates obviously much more efficient code than std::function. If the implementation is known (uncomment definitions on Godbolt) the Delegate example can be fully resolved to a literal number (9)!
  2. Clang 13 is even cleverer and can inline everything. the Delegate and std::function give identical object code, whether the function definitions are known or not.

Anyway, since GCC isn't yet clever enough to optimise the std::function call away, Delegate looks like a useful optimisation.

To my eye, Delegate::connect (eg. s.connect<&Subtractor::subtract>(&inst)) seems a little counterintuitive. Is there an (equally efficient and compact) way to provide a constructor and assignment operator to do the same thing. I guess the problem is passing the member function address as a type, but maybe there's a way.

Thanks and sorry for the random comments,
Tim.

@paulgessinger
Copy link
Member Author

Hey @timadye, no worries I'm super happy about comments like this! I hadn't looked at the objects generated via std::function, to be honest, only at descriptions of how it's implemented. I guess in the long run, this wouldn't be needed then. You're right that I forgot about the const qualifiers.

In terms of the interface: yes, I'm not sure this works with an assignment operator, since the callable has to be given as a template parameter. Lambas with captures cannot be converted to a function pointer, so the only reason I'm able to "capture" the target callable is because it's a compile time parameters. So even an assignment operator which gives the function pointer at runtime wouldn't work, since I can't generate the wrapping lambda that discards the empty instance pointer.

Now, I have one overload

void connect(function_type callable);

already added which takes a callable at runtime, but that callable explicitly has to accept an const void* as the first argument. So this pattern is already not necessarily great, but if you feel like it improves clarity, I can replace/supplement this with an assignment operator that accepts the same callable type.

@timadye
Copy link
Contributor

timadye commented Nov 3, 2021

Hi @paulgessinger,

Thanks for the interesting discussion. I think the connect method is OK, just thought maybe it was worth exploring perhaps-clearer interfaces - especially if Delegate becomes a widely used type. We can stop if this is delaying more important developments.

How about this idea: implement a make_delegate function (or better name?) and assignment operator for the result, so you could say something like:

subtract = make_delegate<&Subtractor::subtract>(&instance);

and maybe also the copy constructor to initialise at construction:

Delegate<int(int)> subtract = make_delegate<&Subtractor::subtract>(&instance);

That seems clearer to me, but maybe it's just needless tinkering?

I don't know whether make_delegate could return a temporary Delegate object (inferring the <int(int)>, or would have to return a transfer type. In either case the temporary should be optimised away.

Even nicer would be to have a Delegate<int(int),&Subtractor::subtract>() constructor, but I guess that's not possible. Also I suppose if we wanted to use type deduction and pass &Subtractor::subtract as an argument, we'd defeat the whole purpose of your optimisation.

What do you think?
Thanks,
Tim.

@paulgessinger
Copy link
Member Author

paulgessinger commented Nov 8, 2021

Hey @timadye, I've added a constructor and an assignment operator from runtime function pointer. I could add a make_delegate function and operators, but that to me just feels like what I already have with connect() but with different symbols. Do you feel strongly about this?

The underlying problem is that this is a value template parameter and not a type, and I'm not sure if they can be deducted somehow, and I'm pretty sure you can't provide explicit template arguments to the constructor.

@timadye
Copy link
Contributor

timadye commented Nov 8, 2021

Hi @paulgessinger,

I'm not really sure how the function_type constructor etc will be used apart from to duplcate the lambda function call done by connect(). Is there a use case - apart from to implement my make_delegate suggestion?

I suggested make_delegate (name unimportant, but following an obvious pattern) simply to allow construction to look like construction and assignment to look like assignment - see my two examples above. To me, connect isn't a pattern I'm used to, but maybe it's more familar to others.

I don't feel very strongly about any of this, just suggesting to explore some alternatives in case you found them useful. So if you prefer connect, go with that. 😄

Thanks,
Tim.

@timadye
Copy link
Contributor

timadye commented Nov 8, 2021

PS. were you planning to add the missing #include <cassert> ?

@paulgessinger
Copy link
Member Author

paulgessinger commented Nov 9, 2021

I feel like having the extra constructor and operator is a good enough tradeoff, so I think I'd stick to this version.

I just added the cassert include.

If you're happy, can you approve?

Copy link
Contributor

@timadye timadye left a comment

Choose a reason for hiding this comment

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

sorry, I didn't know if I had permission to approve anything. Let's see if this works :)

@timadye timadye self-requested a review November 9, 2021 10:31
@kodiakhq kodiakhq bot merged commit 2246d1a into acts-project:main Nov 9, 2021
@paulgessinger paulgessinger deleted the feat/delegate branch November 9, 2021 12:13
kodiakhq bot pushed a commit that referenced this pull request Nov 16, 2021
This PR changes the way the extension mechanism for the (C)KF work. It switches from templating them on all of these extension structs to using the `Delegate` type to define the interface.

At the same time this PR restructures the measurement selection and internal implementation of the `filter` method. Selection should now be more efficient  (in place as much as possible) and not make use of our variant `Measurement` type anymore, but operate directly on the track states.

This PR is dependent on

- #1059
- #1060 
- #1061
and includes their changes, so this PR's changeset will reduce once they are merged.

I'll have to do some manual rebasing to undo changes needed to make the other 3 PRs standalone, so I'll keep this WIP until that's completed.
@paulgessinger paulgessinger modified the milestones: next, v15.0.0 Nov 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants