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

Add support for proxy_view and complementary infrastructure #218

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

mingxwa
Copy link
Collaborator

@mingxwa mingxwa commented Dec 14, 2024

Changes

  • Added class template observer_facade<F> for observing a proxy<F> via a raw pointer.
  • Added alias template proxy_view<F> = proxy<observer_facade<F>>.
  • Added alias template add_view<F> in basic_facade_builder to allow implicit conversion from proxy<F> to proxy_view<F> or proxy_view<const F>.
  • Added unit tests accordingly.

@mingxwa mingxwa requested review from tian-lt and guominrui December 14, 2024 06:37
@mingxwa mingxwa marked this pull request as ready for review December 14, 2024 06:43
Copy link

@Shuenhoy Shuenhoy left a comment

Choose a reason for hiding this comment

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

Thanks for the implementation!

In general, this is what I expected.

I think there can be some additional tests on the behaviors about const:

  • const view from const proxy
const pro::proxy<TestFacade> p1 = ...;
pro::proxy_view<const TestFacade> p2 = p1;
  • const view from non const view
pro::proxy_view<TestFacade> p1 = ...;
pro::proxy_view<const TestFacade> p2 = p1;
  • const view from const raw
const int a = 0;
pro::proxy_view<const TestFacade> p = &a;

A small question: what is the consideration that the view needs to be explicitly added for each facade? Reducing compilation time and binary size? Avoiding misuses?

@mingxwa
Copy link
Collaborator Author

mingxwa commented Dec 15, 2024

@Shuenhoy Thank you for the comments. I added another 2 cases to cover the independent use of proxy_view and added a bunch of std::as_const() calls to force the construction of a proxy_view<const F> from a const proxy<F>.

However, I am hesitant to implement conversion from a non-const view to a const view. Consider the following case:

#include <cstdio>

#include "proxy.h"

PRO_DEF_MEM_DISPATCH(MemFoo, Foo);

struct FooFacade : pro::facade_builder
    ::add_convention<MemFoo, void(), void() const>
    ::add_view<FooFacade>
    ::add_view<const FooFacade>
    ::build {};

struct MyFoo {
  void Foo() {}
};

int main() {
  MyFoo foo;
  pro::proxy<FooFacade> p1 = &foo;  // OK
  pro::proxy_view<FooFacade> p2 = p1;  // OK, equivalent to p2 = &foo
  pro::proxy_view<const FooFacade> p3 = std::as_const(p1);  // OK, equivalent to p3 = &foo

  // If p3 = p2 is defined to be equivalent to the following line of code, it won't compile.
  // Therefore, I prefer not to define the conversion from proxy_view<F> proxy_view<const F>.
  // p3 = &std::as_const(foo);
}

Requiring explicit add_view follows the zero-overhead principle. Because allowing conversion from proxy to proxy_view adds additional metadata to the vtable, further increasing binary size.

@Shuenhoy
Copy link

Thanks for the clarification.

// If p3 = p2 is defined to be equivalent to the following line of code, it won't compile.
// Therefore, I prefer not to define the conversion from proxy_view proxy_view.
// p3 = &std::as_const(foo);

My understanding is that the root issue stems from that proxy<const Facade> is not a thing, which would be something that behaves like ignoring the non-const dispatches (therefore accepting a const raw pointer). But that will probably be out of scope of the "view" topic. So I think this pr can skip const view from non-const view conversion for now, and maybe implement it if you would want to add something like proxy<const Facade> in the future.

@tian-lt
Copy link
Collaborator

tian-lt commented Dec 16, 2024

Suggest adding benchmark cases to see the perf of proxy_view

@mingxwa
Copy link
Collaborator Author

mingxwa commented Dec 16, 2024

@tian-lt I added benchmarks for proxy_view vs virtual functions. The result matches my expectations. Compared to proxy, proxy_view is a little bit slower when the underlying type is small (because it needs to dereference the pointer), and faster when the underlying type is big. Compared to virtual functions, proxy_view is still generally faster. See the generated report from the CI build.

@mingxwa
Copy link
Collaborator Author

mingxwa commented Dec 16, 2024

@Shuenhoy

// If p3 = p2 is defined to be equivalent to the following line of code, it won't compile.
// Therefore, I prefer not to define the conversion from proxy_view<F> to proxy_view<const F>.
// p3 = &std::as_const(foo);

I think the gap here is that for a raw pointer rp2 (the contained value of p2) deduced from dereferencing another pointer rp1 (the contained value of p1) via std::to_address(*rp1), there is no way to deduce the type of std::to_address(*std::as_const(rp1)) to create p3 without knowing the actual type of rp1 in C++. Although in this case rp1 and rp2 are both of type MyFoo*, but proxy allows them to be different. For example,

  • If p1 is created via pro::make_proxy(), *rp1 and *std::as_const(rp1) will be two different types, similar to std::optional. In this case, although rp2 is still MyFoo*, the type of std::to_address(*std::as_const(rp1)) becomes const MyFoo*.
  • If p1 is created from a std::shared_ptr, the situation will be similar to the raw pointer case.

In general, I do not think there is a very good way to define the conversion from proxy_view<F> to proxy_view<const F> without breaking the existing semantics of proxy. Please let us know if you have more thoughts on this.

tests/proxy_view_tests.cpp Show resolved Hide resolved
tests/proxy_view_tests.cpp Outdated Show resolved Hide resolved
@mingxwa mingxwa merged commit 016ec22 into microsoft:main Dec 17, 2024
7 checks passed
@mingxwa mingxwa deleted the user/mingxwa/view branch December 17, 2024 06:40
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.

4 participants