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

*: Move to parking_lot and thus make owning_ref obsolete #78

Merged
merged 2 commits into from
Aug 16, 2022

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Aug 6, 2022

Before proemtheus-client would use the owning_ref crate to map the target
of a std::sync::RwLockReadGuard. owning_ref has multiple unsoundness
issues, see https://rustsec.org/advisories/RUSTSEC-2022-0040.html. Instead of
replacing owning_ref with a similar crate, we switch to locking via
parking_lot which supports the above mapping natively.

Fixes #77

Before `proemtheus-client` would use the `owning_ref` crate to map the target
of a `std::sync::RwLockReadGuard`. `owning_ref` has multiple unsoundness
issues, see https://rustsec.org/advisories/RUSTSEC-2022-0040.html. Instead of
replacing `owning_ref` with a similar crate, we switch to locking via
`parking_lot` which supports the above mapping natively.

Signed-off-by: Max Inden <mail@max-inden.de>
@mxinden
Copy link
Member Author

mxinden commented Aug 6, 2022

I wonder whether moving to parking_lot has any implications on this crate's support for WASM.

Gathering the WASM experts here. Do you have any opinions on this? Do we need to pipe through any feature flags?

@PhilippGackstatter, @thomaseizinger, @mriise, @Stebalien

@mxinden
Copy link
Member Author

mxinden commented Aug 6, 2022

For the record, this patch does not seem to have a significant impact on benchmarks:

➜  critcmp master parking_lot
group                                                  master                                 parking_lot
-----                                                  ------                                 -----------
counter family with Vec<(String, String)> label set    1.00    150.9±1.46ns        ? ?/sec    1.02    153.9±3.00ns        ? ?/sec
counter family with custom type label set              1.00     40.4±0.30ns        ? ?/sec    1.11     44.9±4.67ns        ? ?/sec
encode                                                 1.07     26.5±0.16ms        ? ?/sec    1.00     24.8±1.10ms        ? ?/sec

➜  grep -m 1 'model name' /proc/cpuinfo
model name      : Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz

That said, I don't think the benchmarks are at a point, where I would say that they represent real world use-cases.

@thomaseizinger
Copy link
Contributor

I wonder whether moving to parking_lot has any implications on this crate's support for WASM.

Gathering the WASM experts here. Do you have any opinions on this? Do we need to pipe through any feature flags?

@PhilippGackstatter, @thomaseizinger, @mriise, @Stebalien

Not sure if I'd consider myself a WASM expert 🙈

Do we currently support compiling to WASM? Looking through the CI config, it doesn't seem to be or at least it is not verified? Adding wasm32-unknown-unknown should at least give us some confidence here although we should bear in mind that often, things compile to wasm but don't actually work at runtime. To be really sure, we would have to add wasm_bindgen_test based tests.

@thomaseizinger
Copy link
Contributor

If WASM support is something we want to guarantee, I think we should make a separate PR that adds the target in the CI cross-compile config, merge that first and see if this PR is affected by it.

@AbhijithGanesh
Copy link

@mxinden and @thomaseizinger , I am a new contributor to this project, I am interested in knowing why we should or shouldn't support WASM ? Why WASM for a client that exports metrics? I am not able to understand the usage of WASM here, can you explain!

@thomaseizinger
Copy link
Contributor

@mxinden and @thomaseizinger , I am a new contributor to this project, I am interested in knowing why we should or shouldn't support WASM ? Why WASM for a client that exports metrics? I am not able to understand the usage of WASM here, can you explain!

WASM is becoming a more and more relevant deployment target and no longer necessarily means "browser" or "web".

See for example:

It is completely feasible that users would want to collect metrics in applications written for either of these two platforms in which case they have to compile the Rust client for prometheus to WASM.

Copy link

@AbhijithGanesh AbhijithGanesh left a comment

Choose a reason for hiding this comment

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

LGTM

@Stebalien
Copy link

Note: It looks like parking_lot actually has WASM support. When compiled without (experimental) WASM-atomic support, it just panics when trying to park, but that's fine.

@thomaseizinger
Copy link
Contributor

If WASM support is something we want to guarantee, I think we should make a separate PR that adds the target in the CI cross-compile config, merge that first and see if this PR is affected by it.

I've made a PR for that here: #79

@mxinden
Copy link
Member Author

mxinden commented Aug 10, 2022

Note: It looks like parking_lot actually has WASM support. When compiled without (experimental) WASM-atomic support, it just panics when trying to park, but that's fine.

Adding to this:

The wasm32-unknown-unknown target is only fully supported on nightly with -C target-feature=+atomics in RUSTFLAGS and -Z build-std passed to cargo. parking_lot will work mostly fine on stable, the only difference is it will panic instead of block forever if you hit a deadlock. Just make sure not to enable -C target-feature=+atomics on stable as that will allow wasm to run with multiple threads which will completely break parking_lot's concurrency guarantees.

https://github.com/Amanieu/parking_lot#nightly-vs-stable

@mxinden
Copy link
Member Author

mxinden commented Aug 13, 2022

Unless there are any objections, I will merge and release this tomorrow or next week. Thanks for the input everyone!

@mxinden mxinden merged commit 818c5c8 into prometheus:master Aug 16, 2022
ackintosh pushed a commit to ackintosh/client_rust that referenced this pull request Aug 27, 2022
…eus#78)

Before `proemtheus-client` would use the `owning_ref` crate to map the target
of a `std::sync::RwLockReadGuard`. `owning_ref` has multiple unsoundness
issues, see https://rustsec.org/advisories/RUSTSEC-2022-0040.html. Instead of
replacing `owning_ref` with a similar crate, we switch to locking via
`parking_lot` which supports the above mapping natively.

Signed-off-by: Max Inden <mail@max-inden.de>
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.

Replace owning_ref
4 participants