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: publish verify attestation #82

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Conversation

heilhead
Copy link
Collaborator

Description

This adds the necessary new fields for the verify attestation API.

How Has This Been Tested?

Relay integration.

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@@ -105,6 +105,7 @@ impl Client {
&self,
topic: Topic,
message: impl Into<Arc<str>>,
attestation: impl Into<Option<Arc<str>>>,
Copy link
Member

Choose a reason for hiding this comment

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

Option<impl T> is generally better for a public API, as Into<Option<A>> for Option<B> is usually not implemented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My thinking is that if you already have an Arc<str>, then you'd be able to omit Some(), but in practice that's probably never gonna happen.

I think the conversation to have here is why are we still sticking to Arc<str>, even though they likely don't have any significant performance benefits, and only hurt the ergonomics of the APIs.

Copy link
Member

Choose a reason for hiding this comment

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

What would be the alternative to Arc<str>? I think it's good to have basically free cloning

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In practice we rarely clone these strings.

The downsides to using Arc<str> is double allocation on deserialization (it essentially first deserializes into String, and then converts that to Arc<str>, if I'm not mistaken) and the ergonomics of working with it (converting String to Arc<str> is another allocation).

An alternative is using regular String. The cloning cost is mostly mitigated by using a fast allocator like jemalloc or mimalloc.

It's hard to say definitively what would be more performant for the relay, but in some cases (a lot of highly multi-threaded cloning) using String is a lot faster, since allocators would use essentially thread-local memory arenas with minimal locking, while Arc<str> is using atomics under the hood for synchronization. And even though atomic usage is cheap, it's not free.

Copy link
Member

Choose a reason for hiding this comment

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

double allocation on deserialization (it essentially first deserializes into String, and then converts that to Arc, if I'm not mistaken)

Oh wow I didn't know about that. Yeah that looks accurate

Copy link
Member

@xDarksome xDarksome Jul 31, 2024

Choose a reason for hiding this comment

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

I don't think it's the case, seems like it initially deserializes it into String, then converts it into Box<str> and then into Arc<str>

The reallocation can only happen if the underlying Vec shrinks (https://doc.rust-lang.org/std/vec/struct.Vec.html#method.shrink_to_fit) and I assume that's not the case, because the original String should have the exact size needed.

However, it may depend on the deserializer.

@heilhead heilhead merged commit d17f94b into main Jul 30, 2024
7 checks passed
@heilhead heilhead deleted the feat/publish-attestation branch July 30, 2024 14:55
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