Draft: Bundle relay performance and reliability improvements#131
Draft: Bundle relay performance and reliability improvements#131poulcarlsen53 wants to merge 1 commit into
Conversation
|
Thanks for putting this together, and for offering to split it up front. I'd like to take you up on that. At ~7.8k additions across 62 files touching crypto, transport, session, and config, it's not realistically reviewable as one PR, and it's currently conflicting with main (likely the frame allocations work in 86ffe94 that already landed). Before splitting, a few product questions worth settling so the split PRs aren't chasing a moving target:
Happy to discuss any of these in a separate issue if easier. Once those are settled, suggested split (each rebased onto main, each independently reviewable):
The Apps Script + example-config pieces overlap with #129, which I've left review comments on. Once that lands, please rebase this branch on top to avoid re-litigating those files. Frame/crypto: 86ffe94 already reduced marshaling allocations on Thanks again, looking forward to the smaller PRs. |
|
Thanks for the detailed review and for laying out the split. I agree that this PR is too large to review safely as one bundle, especially because it mixes config, transport, session behavior, diagnostics, benchmarks, and docs. I’ll treat this PR as superseded and will open smaller PRs instead. Please feel free to close this one. On the product questions:
For upstream, I think the safer direction is to prioritize raw knobs first, because they are explicit and easier to reason about in review. Presets are useful for users, but they encode opinions that may need real deployment data before becoming stable defaults. If performance_mode stays, I think it should be only a convenience layer:
So my current preference for upstream is: land the individual knobs with conservative defaults first, then discuss performance_mode separately once the behavior is proven.
I agree this is easy to turn into scope creep. Auto-tune can be useful, but it adds support/debugging complexity because users may not understand why latency or throughput changes over time. For upstream, I think auto_tune should not be part of the first split PRs. It should be deferred until after the static knobs, metrics, and diagnostics are in place. If it comes back later, it should be:
So for now I’ll keep auto_tune out of the next PRs unless you want to discuss it in a separate issue.
My intent was:
So yes, direct_stream_urls is WebSocket-only in intent and does something different from existing relay_urls. That said, I agree the naming can be confusing because both are “direct relay” paths. I’m open to whichever naming you prefer before I split that work. Possible options:
For the split PRs, I’ll avoid introducing stream config until we settle the naming/design. Planned split:
On frame/crypto: agreed. Since 86ffe94 already landed allocation improvements, I’ll diff against current main before proposing any frame/crypto work and avoid redoing already-merged changes. I’ll stop updating this large bundle and move the work into smaller reviewable PRs. |
Summary
This draft PR bundles performance, reliability, diagnostics, transport, and configuration improvements from an optimized GooseRelayVPN fork.
Major areas:
Verification
go test -count=1 ./...go vet ./...go build ./cmd/client ./cmd/server ./bench/harness ./bench/sink ./bench/diffNotes
This is intentionally opened as a draft because it is a large change set. I am happy to split pieces into smaller PRs if that is easier to review.