Skip to content

refactor(noq-proto)!: Remove identity_hash from public API#646

Open
Frando wants to merge 4 commits into
mainfrom
Frando/remove-identity-hash-from-api
Open

refactor(noq-proto)!: Remove identity_hash from public API#646
Frando wants to merge 4 commits into
mainfrom
Frando/remove-identity-hash-from-api

Conversation

@Frando
Copy link
Copy Markdown
Member

@Frando Frando commented May 11, 2026

Description

Based on #643

Removes identity_hash from the public API of noq_proto.

Breaking Changes

  • changed: noq_proto::PathId no longer implementsidentity_hash::IdentityHashable

Notes & open questions

Not sure if it's worth it?

Change checklist

  • Self-review.
  • All breaking changes documented.

@Frando Frando force-pushed the Frando/remove-identity-hash-from-api branch from e87b1e3 to 1e533fb Compare May 11, 2026 09:54
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 11, 2026

Performance Comparison Report

e87b1e3a859b249f44202ccc9853470dc361263b - artifacts

No results available

---
1e533fb14fc82166fb2e0c48a34369eed42773b2 - artifacts

No results available

---
33ed91ca2a087c8b359d3ceb396c1c16f25728f1 - artifacts

Raw Benchmarks (localhost)

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

@Frando Frando mentioned this pull request May 11, 2026
1 task
@Frando Frando force-pushed the Frando/remove-identity-hash-from-api branch from 1e533fb to 33ed91c Compare May 11, 2026 09:55
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 11, 2026

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

@n0bot n0bot Bot added this to iroh May 11, 2026
@github-project-automation github-project-automation Bot moved this to 🚑 Needs Triage in iroh May 11, 2026
Copy link
Copy Markdown
Member

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

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.

@flub
Copy link
Copy Markdown
Collaborator

flub commented May 11, 2026

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.

Base automatically changed from Frando/check-external-types to main May 11, 2026 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🚑 Needs Triage

Development

Successfully merging this pull request may close these issues.

4 participants