Re: fxl::WeakPtr in SDK

[Hunter Freyer]

+eng-c...@fuchsia.dev +Dave Schuyler +Ruby Zhuang 

Reviving this for a related situation: this CL is adding an input_report_reader helper library to the SDK, which internally uses a RingBuffer. In fact, it uses FBL's RingBuffer implementation, but due to the aforementioned prohibition on FBL in the SDK, it copy-pastes the implementation into its own namespace. What should we do in situations where there is no acceptable alternative in stl?

Yifei's suggestion to "expose these types under a namespace such as `sdk_internal::`" sounds reasonable to me, especially given the alternative here is to fork the implementation. Is there a way to do that without making all FBL types accessible through the internal namespace?

Thanks,

Hunter

On Fri, Jan 27, 2023 at 6:31 PM 'Yifei Teng' via api-council <api-c...@fuchsia.dev> wrote:

Got it. That's very helpful insight! std::weak_ptr and friends works for us today.

On Fri, Jan 27, 2023 at 3:16 PM Adam Barth <aba...@google.com> wrote:

That's fine if the SDK libraries are prebuilt, but if they're source libraries, there's no way to prevent people from taking dependencies in supposedly "internal" namespaces.  We've had to clean up examples in the past where SDK customers had taken dependencies on "internal" parts of the headers.

Someday we'll have C++ modules and we can actually enforce some of these things.

Adam

On Fri, Jan 27, 2023 at 8:58 PM Yifei Teng <yif...@google.com> wrote:

That makes sense. Would folks consider a subset where these types are published to the SDK, but only allowed to be used by SDK libraries themselves internally? The use cases I had in mind won't expose `fxl::WeakPtr` per se to their public API -- they would internally replace the `std::weak_ptr`s with `fxl::WeakPtr`. One way I imagine this could work is to expose these types under a namespace such as `sdk_internal::`, and maybe with build allowlist/visibility support.

On Fri, Jan 27, 2023 at 12:54 PM Jaeheon Yi <jae...@google.com> wrote:

Thank you Brett and Adam, these are good insights to keep in mind.

Jaeheon

On Fri, Jan 27, 2023 at 12:51 PM 'Adam Barth' via api-council <api-c...@fuchsia.dev> wrote:

+1

That's a big reason we've avoided putting FXL and FBL into the SDK up to this point.

We had to go the other way with fit::function versus std::function because of our extensive use of move-only types, which don't play well with std::function's copy constructor.  However, that's required a lot of ongoing investment and has posed exactly these kinds of integration costs on our customers.

Adam

On Fri, Jan 27, 2023 at 8:48 PM 'Brett Wilson' via api-council <api-c...@fuchsia.dev> wrote:

[I'm not on the API council so this is just some personal feedback.]

I think the more our code diverges from standard C++, the more difficult it is to integrate into other projects. The two areas where big projects often start diverging (sometimes with good reason, sometimes without) is in containers and these types of memory management primitives. Once you start using code with alternate implementations of these core library primitives their use starts infecting/affecting everything else.

If we want our SDK to be picked up as easily as possible by as many projects as possible, I think we should try very hard to avoid custom implementations of core library objects.

Brett

On Fri, Jan 27, 2023 at 11:24 AM 'Yifei Teng' via api-council <api-c...@fuchsia.dev> wrote:

Hi API Council,

What are the council's thoughts on adding `fxl::WeakPtr` to the partner SDK, or in general, the `fxl/memory` library? Looks like right now it's in the internal SDK, but I could think of at least two places where it could be useful:

- In the union accessors of the new C++ bindings, to detect and assert on use-after-free.

- In an upcoming async utility library, to cancel operations where the receiver has gone away.

In these cases we could use `std::weak_ptr` as a substitute, so this is not strictly required, but the std versions are less efficient and the atomic operations are not necessary in single-threaded scenarios. One also need to be more careful using `std::weak_ptr` because its API support upgrading a weak pointer to a shared one and arbitrarily prolonging the lifetime, whereas `fxl::WeakPtr` guarantees deterministic destruction by the owner of the referenced object.

Thanks,

Yifei

--
You received this message because you are subscribed to the Google Groups "api-council" group.
To unsubscribe from this group and stop receiving emails from it, send an email to api-council...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/api-council/CANbn4XDkp3zeqG1gewoxEMkc%2BPrzVTGk6g1TxgS8w4Lp4drBdA%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "api-council" group.
To unsubscribe from this group and stop receiving emails from it, send an email to api-council...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/api-council/CABiGVV_LT%2BVCWM5e5wBgHZGw6pf%2BpawwZhxdFd0cq0ymrQHzXw%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "api-council" group.
To unsubscribe from this group and stop receiving emails from it, send an email to api-council...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/api-council/CAP%3D28ceHB5wjYC1LBjRwpOqU%2BtJKyhas5hTMqC_zjhvWcrj4YQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "api-council" group.
To unsubscribe from this group and stop receiving emails from it, send an email to api-council...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/api-council/CANbn4XB%2BWQ8nSp_1R6tXCrL%3DwJFTVxDi8c2NDT7R%2BubvoSn7wg%40mail.gmail.com.

[Adam Barth]

Can we use https://en.cppreference.com/w/cpp/container/deque instead?

Adam

[Ruby Zhuang]

See David's original comment on why we'd want to use `fbl::RingBuffer` here: https://fuchsia-review.git.corp.google.com/c/fuchsia/+/684203/comment/c0bf135a_836263e3/. TL;DR is that we don't want to allocate memory when someone added a new report to keep allocations out of the InputReport pipeline so that report latency stays consistent.

Does `deque` give us this too?

[Brett Wilson]

On Tue, Jan 31, 2023 at 9:34 AM Ruby Zhuang <rdzh...@google.com> wrote:

See David's original comment on why we'd want to use `fbl::RingBuffer` here: https://fuchsia-review.git.corp.google.com/c/fuchsia/+/684203/comment/c0bf135a_836263e3/. TL;DR is that we don't want to allocate memory when someone added a new report to keep allocations out of the InputReport pipeline so that report latency stays consistent.

Does `deque` give us this too?

std::deque will only allocate when its size grows and will allocate in blocks. I'm not a fan of std::deque but I probably would have used it in your situation. There are some details about how deque works that I wrote up a few years ago here:

https://source.chromium.org/chromium/chromium/src/+/main:base/containers/README.md

I don't think std::deque will allocate very often for your use case. We use libc++ so you'll get a 4K initial buffer of reports without allocating.

I think what you have is good and we shouldn't revisit the implementation. But I also don't think this is a strong reason we need to reconsider exposing FBL code.

Brett

[Adam Barth]

Looking at the implementation of input_report_reader::RingBuffer in the CL, it looks like the bounds checks exist only in debug.  In release, pushing/emplacing too many elements will corrupt the heap...

How big is the Report object?  Looking at the FIDL, it looks like about 8 pointers and the max number of reports is 50...  8*8*50 = 3200.  If I've got these numbers correct, it sounds like std::deque won't ever expand beyond the 4k initial buffer.

Adam

[Ruby Zhuang]

The Report object is not one-to-one equivalent to the FIDL report. Examples of Reports are here:

- https://cs.opensource.google/fuchsia/fuchsia/+/main:src/devices/light-sensor/drivers/ams-light/tcs3400.h;l=25?q=Tcs3400InputReport&ss=fuchsia%2Ffuchsia

- https://cs.opensource.google/fuchsia/fuchsia/+/main:src/ui/input/drivers/gt6853/gt6853.h;l=34?q=Gt6853InputReport&ss=fuchsia%2Ffuchsia

These Reports are then translated to FIDL reports using the `ToFidlInputReport` function.

On the other hand, the FIDL reports may also grow in the future as we've only implemented support for a few common HID classes.

[Brett Wilson]

On Tue, Jan 31, 2023 at 11:21 AM Ruby Zhuang <rdzh...@google.com> wrote:

The Report object is not one-to-one equivalent to the FIDL report. Examples of Reports are here:

- https://cs.opensource.google/fuchsia/fuchsia/+/main:src/devices/light-sensor/drivers/ams-light/tcs3400.h;l=25?q=Tcs3400InputReport&ss=fuchsia%2Ffuchsia

- https://cs.opensource.google/fuchsia/fuchsia/+/main:src/ui/input/drivers/gt6853/gt6853.h;l=34?q=Gt6853InputReport&ss=fuchsia%2Ffuchsia

These Reports are then translated to FIDL reports using the `ToFidlInputReport` function.

On the other hand, the FIDL reports may also grow in the future as we've only implemented support for a few common HID classes.

Regardless I think you will want to ensure your code never corrupts memory. RingBuffer's behavior is not what I would have expected for a reusable container like this. Having the code copied here means that if/when RingBuffer is improved, your code won't benefit.

It sounds like std::deque is the way to go for your use-case. 

Brett

[Yifei Teng]

I have to admit we did the same thing for `WAVLTree` in FIDL. A copy of fbl WAVLTree was forked into the C++ bindings. The motivation was that because `std::` libraries don't offer any intrusive containers, WAVLTree lets us avoid allocations when tracking the async FIDL calls on the client side. The WAVLTree is not exposed in the public interface but it does have a risk to be used (someone could write `fidl::internal_wavl::...` in their code). If folks think the risk of exposing WAVLTree outweighs the benefits of avoiding allocation, then I'll remove the fork and replace it with perhaps `std::unordered_map`.

[James Robinson]

The C++ standard library containers are pretty bad, unfortunately. The APIs force them to be much larger and slower than more specialized containers. It seems like std::dequeue will be fine for the RingBuffer use case cited above given how it's being used. I doubt that we could afford the memory and performance overhead of std::unordered_map in the FIDL bindings, although we could measure it if we wanted to be sure.

- James

--
All posts must follow the Fuchsia Code of Conduct https://fuchsia.dev/fuchsia-src/CODE_OF_CONDUCT or may be removed.
---
You received this message because you are subscribed to the Google Groups "eng-council" group.
To unsubscribe from this group and stop receiving emails from it, send an email to eng-council...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/eng-council/CANbn4XDvUva%3DBSSvhE%2BDb%3D5THGMavM0QVrC%3DJtAx2%3DUL-j%2Bq9w%40mail.gmail.com.

[Dave Schuyler]

If we did want to include custom containers, I believe it will be best to include them in a way where they are shared (from the same source) rather than duplicate them. i.e. +1 to "Having the code copied [...] means that if/when [container] is improved, your code won't benefit."

[Ian McKellar]

On Tue, Jan 31, 2023 at 5:19 PM 'Dave Schuyler' via api-council <api-c...@fuchsia.dev> wrote:

If we did want to include custom containers, I believe it will be best to include them in a way where they are shared (from the same source) rather than duplicate them. i.e. +1 to "Having the code copied [...] means that if/when [container] is improved, your code won't benefit."

The challenge is that if we include the custom containers in our supported API & ABI surface then we're in the same boat as the C++ standard library and we can't improve them over time :-/

 

You received this message because you are subscribed to the Google Groups "api-council" group.
To unsubscribe from this group and stop receiving emails from it, send an email to api-council...@fuchsia.dev.
To view this discussion on the web visit https://groups.google.com/a/fuchsia.dev/d/msgid/api-council/CAOsj%2BY%3DL8fWj6KLw870L8omYfRa_his3%3D1jdLTi5mcFYEjcozA%40mail.gmail.com.

[Ruby Zhuang]

Re: ring buffers. Sounds good! I'll use std:deque for this usecase.

[Jaeheon Yi]

+1 to std::deque usage to reduce our API burden. 🙏