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

⚡ zvariant, zbus: use borrowed clone if possible #407

Merged
merged 3 commits into from
Jul 16, 2023
Merged

⚡ zvariant, zbus: use borrowed clone if possible #407

merged 3 commits into from
Jul 16, 2023

Conversation

Kijewski
Copy link
Contributor

This PR implements Signature::as_ref() to get a borrowed clone of a Signature. The code is a copy of Str::as_ref().

Even though cloning an Arc is cheap, it's not exactly for free. An atomic increment has some inherent overhead, and Arc::clone() also panic!()s if there are excessively many strong references to an Arc. For every panic!() some code to unwind the stack has to be generated, which hurts cache locality, and bloats the generated code.

This change implements `Signature::as_ref()` to get a borrowed clone of
a `Signature`. The code is a copy of `Str::as_ref()`.
The variants `MessageField::Path` and `MessageField::Signature` might
own their data. Both variants wrap `Str` and `Bytes`, resp., which use
`Arc<[_]>` for their owned data.

Even though cloning an `Arc` is cheap, it's not exactly for free. An
atomic increment has some inherent overhead, and `Arc::clone()` also
`panic!()`s if there are excessively many strong references to an `Arc`.
For every `panic!()` some code to unwind the stack has to be generated,
which hurts cache locality, and bloats the generated code.
This change replaces `Signature::clone()` with `Signature::as_ref()`
where possible. This makes the generated code slightly more dense,
because there can be no `panic!()`s, and slightly faster, because no
exclusive access to the strong reference counter has the be acquired,
e.g. through a `lock` prefix on amd64.

No function signatures were changed.
Copy link
Contributor

@zeenix zeenix left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good to me and kudos for providing nice atomic commits with details in the log. 👍

@zeenix zeenix enabled auto-merge July 16, 2023 10:10
@zeenix zeenix merged commit fe457ea into dbus2:main Jul 16, 2023
7 checks passed
@Kijewski Kijewski deleted the pr-as-ref branch July 16, 2023 16:05
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.

2 participants