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

Experiment with using raw pointers for objects. #366

Closed
wants to merge 3 commits into from
Closed

Conversation

rfk
Copy link
Collaborator

@rfk rfk commented Dec 21, 2020

Opening for early visibility.

As noted in SDK-157, the way we use a ConcurrentHandleMap for managing objects means that we foist certain threading/locking behaviors on all API conumers. It's currently only possible for a single thread to be operating on a given Rust object at any one time, even if there are operations that could safely be performed without synchronization. This is making it awkward to create the desired API for the Nimbus SDK, where we want to offer a synchronous getExperimentBranch method that will return immediately, even if there is a concurrent updateExperiments call running on a another thread.

A little while back, @thomcc posted the excellent #244 discussing sources of overhead in uniffi, and one thing he suggested there was that we could consider using raw pointers instead of a HandleMap. Many of the features of HandleMap are designed to guard against programmer error, so they're more useful in a manually-written FFI layer than in generated bindings.

This PR is me experimenting with using raw pointers instead of a ConcurrentHandleMap. I plan to use it to see whether the handlemap is indeed the source of the blocking behaviour seen in SDK-157.

This is emphatically not suitable for merging, and in fact it contains known very-dangerous-completely-unsafe behaviours (such as just assuming we can get an &mut T from a pointer interpreted as a Box<T> without worrying about a little thing called locking). I need to get my head around what would be required to make this safe, but the unsafe version is suitable for my investigations in the meantime.

@thomcc
Copy link

thomcc commented Dec 21, 2020

such as just assuming we can get an &mut T from a pointer interpreted as a Box without worrying about a little thing called locking

FYI just having aliased Box<T> is already UB, regardless if lock it or turn it into a &mut T or &T, see rust-lang/unsafe-code-guidelines#258

@@ -24,3 +24,9 @@ Hello everyone
# Swift

- [Integrating with XCode](./swift/xcode.md)

# Internals
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Much like our lifting/lowering/serialization story, the way we currently use handlemaps is under-documented. Let's add a new section to the manual that we can use for narrative docs about such things.

```

# Concurrent Access
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Based on this experiment and subsequent conversations, here's a concrete proposal for how to expose uniffi interfaces in a way that gives us more control over the concurrency story. Feedback most welcome at this stage! /cc @mhammond @jhugman @dmose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Note that the PR doesn't actually implement proposal yet, although I'm confident that it can be implemented without much fuss)

Copy link
Member

@mhammond mhammond Jan 12, 2021

Choose a reason for hiding this comment

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

This seems awesome. While it doesn't handle the "multiple bindings" use-case, I wonder if we can avoid uniffi knowing about that kind of setup? Eg, I wonder if we can arrange so that instead of lib.rs having, say:

    pub fn get_active_experiments(&self) -> Result<Vec<EnrolledExperiment>> {
        get_enrollments(self.db()?)
    }

instead it does something like:

    pub fn get_active_experiments(&self) -> Result<Vec<EnrolledExperiment>> {
       handwavey_get_static_instance().get_active_experiments()
    }

(ie, that it's OK if there are multiple instances of the bindings, because they all just delegate). I'd need to play a little to see if that might work in practice though. Did you have anything in mind for that use-case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(We discussed this a little in meetings and slack, but commenting here for posterity)

I'd be interested in exposing the singleton concept more directly through the IDL. For example, imagine a Nimbus API definition that looks like this:

namespace nimbus {
  // Returns the global NimbusClient singleton, or null if it has not been initialized yet.
  NimbusClient? get_global_nimbus_client();

  // Sets the global NimbusClient singleton, or throws if it has already been set.
  [Throws=Error]
  void set_global_nimbus_client(NimbusClient client);
};

interface NimbusClient {
  // all the usual methods here
}

Uniffi doesn't support this today. Specifically, we don't let you pass object references as arguments (#40) or return them from functions (#197), so you wouldn't be able to declare the getter/setter for the singleton.

However, I think it's clear that we want to support such functionality at some point, and I don't think there's any deep technical reason why we couldn't do so, there's just a bunch of details to carefully work through (as described in the linked issues).


```idl
[Threadsafe]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Bikeshedding also welcome on how to expose this in the UDL...

@rfk
Copy link
Collaborator Author

rfk commented Jan 14, 2021

This was a useful experiment, but as discussed in #368 I believe we're going to stick with using a handlemap for now. I've migrated the docs proposed here to a standalone PR at #371, included a link to this experiment in the ADR doc for future reference, and will close this PR as having served its purpose.

@rfk rfk closed this Jan 14, 2021
@rfk rfk deleted the raw-object-pointers branch June 7, 2021 06:25
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.

3 participants