Use strong_count instead of upgrade on weak Arcs in cache#459
Use strong_count instead of upgrade on weak Arcs in cache#459zaharidichev merged 3 commits intomasterfrom
Conversation
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
the change looks good to me! i had some questions about the benchmark setup, not blockers.
benches/Cargo.toml
Outdated
| linkerd2-error = { path = "../linkerd/error" } | ||
| linkerd2-lock = { path = "../linkerd/lock" } | ||
| linkerd2-stack = { path = "../linkerd/stack" } | ||
| bencher = "0.1.5" |
There was a problem hiding this comment.
Elsewhere, we have benchmarks that are written with the libtest benchmark framework. I'm not opposed to switching to bencher, especially since it can run on stable; however, I think it would be good to pick one and use it consistently. If we decide to adopt bencher here, we should probably open a follow-up to change other benchmarks to use bencher as well.
We may also want to consider criterion rather than bencher. Although I have my doubts about the accuracy of criterion's regression detection (it seems way too sensitive to noise IMO), I think some of the additional benchmarking APIs criterion provides, like bench_with_input and iter_custom are very useful when writing benchmarks...just a thought!
benches/Cargo.toml
Outdated
| @@ -0,0 +1,21 @@ | |||
| [package] | |||
| name = "benches" | |||
There was a problem hiding this comment.
Curious about the motivation to create a new benches crate, rather than putting the cache benchmarks in linkerd/cache/benches? Not a blocker, I just want to know if there's a reason we can't do the former.
There was a problem hiding this comment.
All other things equal, I think I'd prefer that the tests are "close" to the code they test.. I.e. rather than a crate for all benches, I think they should be within the crate they test. My workflow is to run cargo test -p $crate.
There was a problem hiding this comment.
No particular reason really. I was looking at other crates on github and how they organize things and its was mostly 50/50. Makes sense to be closer to code. I moved it and switched to libtest. Feel free to take another look. I am not sure whether the unstable feature flag is how it should be setup.
There was a problem hiding this comment.
I was looking at other crates on github and how they organize things and its was mostly 50/50
In general, I think putting benchmarks in a separate crate is something that libraries will do, usually so that projects that depend on that library don't need to download test-only dependencies. In this case, I don't think it's necessary, since there aren't any downstream consumers of linkerd-internal crates.
|
CI failure doesn't seem related, I'm giving it another try. |
|
@hawkw the test certs are expired and need to be updated on master |
hawkw
left a comment
hawkw
left a comment
There was a problem hiding this comment.
One last note on benchmark structure.
linkerd/cache/src/lib.rs
Outdated
| #[cfg(all(feature = "unstable", test))] | ||
| mod benches; |
There was a problem hiding this comment.
Instead of a module, let's put the benchmarks in a separate benches directory inside the cache crate? That's generally considered more conventional, and would let ets us run them with cargo bench, as well as removing the necessity of an unstable feature flag.
Signed-off-by: Zahari Dichev <[email protected]>
Signed-off-by: Zahari Dichev <[email protected]>
zaharidichev
force-pushed
the
zd/use-strong-count
branch
from
62a1ca1 to
db51d6d
Compare
Signed-off-by: Zahari Dichev <[email protected]>
zaharidichev
force-pushed
the
zd/use-strong-count
branch
from
db51d6d to
46a8a74
Compare
This release restores the `route_actual_response_total` metric, which is needed for `linkerd routes -o wide`. --- * Update test certificates (linkerd/linkerd2-proxy#460) * Use strong_count instead of upgrade on weak Arcs in cache (linkerd/linkerd2-proxy#459) * Wire authority override coming from discovery (linkerd/linkerd2-proxy#462) * Update integration tests certs (linkerd/linkerd2-proxy#465) * Add a `mock-orig-dst` feature flag (linkerd/linkerd2-proxy#466) * http-metrics: Make latency export optional (linkerd/linkerd2-proxy#467) * Restore the route_actual_response_total metric (linkerd/linkerd2-proxy#468)
This release restores the `route_actual_response_total` metric, which is needed for `linkerd routes -o wide`. --- * Update test certificates (linkerd/linkerd2-proxy#460) * Use strong_count instead of upgrade on weak Arcs in cache (linkerd/linkerd2-proxy#459) * Wire authority override coming from discovery (linkerd/linkerd2-proxy#462) * Update integration tests certs (linkerd/linkerd2-proxy#465) * Add a `mock-orig-dst` feature flag (linkerd/linkerd2-proxy#466) * http-metrics: Make latency export optional (linkerd/linkerd2-proxy#467) * Restore the route_actual_response_total metric (linkerd/linkerd2-proxy#468)
3 participants
I was trying to understand the caching code a bit more today. Unless I am missing something, it seems to me that we can use
strong_countinstead of trying to upgrade aWeakto determine eviction eligibility. At least in the case where we are not evicting, we swap a CAS loop and a few branches for aSeqCstload, which seems to be making a difference. Heare are the bench results. Let me know if I am missing something obviousSigned-off-by: Zahari Dichev [email protected]