refactor(noq-proto)!: Remove identity_hash from public API#646
Conversation
e87b1e3 to
1e533fb
Compare
Performance Comparison Report
|
| Scenario | noq | upstream | Delta | CPU (avg/max) |
|---|---|---|---|---|
| large-single | 5916.4 Mbps | 7845.8 Mbps | -24.6% | 97.7% / 98.9% |
| medium-concurrent | 5508.6 Mbps | 7736.5 Mbps | -28.8% | 96.3% / 97.8% |
| medium-single | 4122.3 Mbps | 4744.2 Mbps | -13.1% | 95.9% / 98.3% |
| small-concurrent | 3870.3 Mbps | 5317.5 Mbps | -27.2% | 97.4% / 99.6% |
| small-single | 3576.0 Mbps | 4857.6 Mbps | -26.4% | 96.4% / 98.6% |
Netsim Benchmarks (network simulation)
| Condition | noq | upstream | Delta |
|---|---|---|---|
| ideal | 3079.3 Mbps | 3911.2 Mbps | -21.3% |
| lan | 782.5 Mbps | 810.3 Mbps | -3.4% |
| lossy | 69.8 Mbps | 55.9 Mbps | +25.0% |
| wan | 83.8 Mbps | 83.8 Mbps | ~0% |
Summary
noq is 23.6% slower on average
1e533fb to
33ed91c
Compare
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/noq/pr/646/docs/noq/ Last updated: 2026-05-11T09:58:44Z |
matheus23
left a comment
There was a problem hiding this comment.
I'm not sure. identity-hash has had only exactly one release that is three years old.
It has zero dependencies and otherwise only depends on stable rust std traits.
It only uses core imports (not std) and is no_std-compatible.
I really see no reason to not depend on it in this niche way. Even if we end up removing IntMap, we can keep the identity hash impl for PathId without almost any added cost.
|
Meh, I'm really conflicted. I'm tempted to lean to +1. Mostly I think rust isn't great here. OTOH the newtype is essentially free. But I can see this gets annoying if we have to do it for lots of reason. I think I'd rather do this now so we have it as a pattern and then see how it plays out and if we want to expose it anyway. |
Description
Based on #643
Removes
identity_hashfrom the public API ofnoq_proto.Breaking Changes
noq_proto::PathIdno longer implementsidentity_hash::IdentityHashableNotes & open questions
Not sure if it's worth it?
Change checklist