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: Migrate raw function pointers to std::function #4461

Merged
merged 80 commits into from
Feb 7, 2023

Conversation

byronxu99
Copy link
Collaborator

@byronxu99 byronxu99 commented Jan 18, 2023

This PR replaces several raw function pointers with std::function objects.

The biggest changes are to the learner class, which is now no longer templated. To erase the data type, learner builders will bind the data object to their input functions via a lambda capture. To erase the example type, the lambda will accept a polymorphic_ex wrapper around VW::example* and VW::multi_ex*, which implicitly converts back to the original type. The resulting lambda can be stored in a std::function object with a type which does not depend on DataT nor ExampleT.

This removes all undefined behavior around function pointer typecasting.

@rajan-chari
Copy link
Member

Yeah there are a few ways of looking at it. You should ask a few people to see what the default intuition would be. For me it is the perspective of examples since thats what the logic of reductions is designed around

Maybe it's helpful to seperate the issues when thinking about naming things. There are 2 tensions:

  1. From the stack construction perspective we move in one direction (bottom of the stack to top). When predict() is called, we move in the other direction (top of stack calls the next reduction down the stack).
  2. The word 'base' has two meanings when going down the predict/learn stack. (a) It's the next down stream reduction. (b) It's the last reduction in the stack.

Copy link
Member

@rajan-chari rajan-chari left a comment

Choose a reason for hiding this comment

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

Perf results seem good. It makes sense to pass shared_ptr by reference to avoid allocation, locking and copy overhead. I think most people here would have been watching out for that anyway. Looks good to me. Thanks!

vowpalwabbit/core/include/vw/core/learner.h Outdated Show resolved Hide resolved
@byronxu99 byronxu99 mentioned this pull request Feb 6, 2023
@byronxu99 byronxu99 merged commit 24ea2d4 into VowpalWabbit:master Feb 7, 2023
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.

5 participants