control/controlclient: remove optimization that was more convoluted than useful #14514
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bradfitz
force-pushed
the
bradfitz/ptr_to_view
branch
from
4750dc4 to
6dd8157
Compare
bradfitz
added a commit
that referenced
this pull request
Jan 3, 2025
Importing the ~deprecated golang.org/x/exp/maps as "xmaps" to not shadow the std "maps" was getting ugly. And using slices.Collect on an iterator is verbose & allocates more. So copy (x)maps.Keys+Values into our slicesx package instead. Updates #cleanup Updates #12912 Updates #14514 (pulled out of that change) Change-Id: I5e68d12729934de93cf4a9cd87c367645f86123a Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
andrew-d
approved these changes
Jan 3, 2025
sailorfrag
approved these changes
Jan 3, 2025
util/slicesx/slicesx.go
Outdated
Comment on lines
158
to
179
| // the std "maps" package. This version exists for clarity | ||
| // when reading call sites. |
There was a problem hiding this comment.
I feel like this is a weak argument -- it's easy enough to import it as xmaps. The allocation optimization seems more compelling, so maybe that should be mentioned first?
There was a problem hiding this comment.
It's actually not that easy to allocate it as xmaps... goimports doesn't help (as much / as quickly) in that case. And you're back to doing it manually like a caveperson. That's often what bugs me more than the alloc.
bradfitz
force-pushed
the
bradfitz/ptr_to_view
branch
from
6dd8157 to
45bbd13
Compare
raggi
approved these changes
Jan 3, 2025
| @@ -778,14 +753,19 @@ func peerChangeDiff(was tailcfg.NodeView, n *tailcfg.Node) (_ *tailcfg.PeerChang | |||
| return ret, true | |||
| } | |||
|
|
|||
| func (ms *mapSession) sortedPeers() []tailcfg.NodeView { | |||
There was a problem hiding this comment.
If we wanted to keep the memorized version of this, we could put it in a lazy value and clear it any time the peer list is written to
There was a problem hiding this comment.
That's the sort of accounting (invalidating X on all changes) I'm trying to remove to simplify the code.
Turns out there's exactly one caller of this func that I want to eliminate entirely anyway, so memoizing it wouldn't pay off.
bradfitz
added a commit
that referenced
this pull request
Jan 3, 2025
Importing the ~deprecated golang.org/x/exp/maps as "xmaps" to not shadow the std "maps" was getting ugly. And using slices.Collect on an iterator is verbose & allocates more. So copy (x)maps.Keys+Values into our slicesx package instead. Updates #cleanup Updates #12912 Updates #14514 (pulled out of that change) Change-Id: I5e68d12729934de93cf4a9cd87c367645f86123a Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz
force-pushed
the
bradfitz/ptr_to_view
branch
from
45bbd13 to
a545684
Compare
…han useful While working on #13390, I ran across this non-idiomatic pointer-to-view and parallel-sorted-map accounting code that was all just to avoid a sort later. But the sort later when building a new netmap.NetworkMap is already a drop in the bucket of CPU compared to how much work & allocs mapSession.netmap and LocalBackend's spamming of the full netmap (potentially tens of thousands of peers, MBs of JSON) out to IPNBus clients for any tiny little change (node changing online status, etc). Removing the parallel sorted slice let everything be simpler to reason about, so this does that. The sort might take a bit more CPU time now in theory, but in practice for any netmap size for which it'd matter, the quadratic netmap IPN bus spam (which we need to fix soon) will overshadow that little sort. Updates #13390 Updates #1909 Change-Id: I3092d7c67dc10b2a0f141496fe0e7e98ccc07712 Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
bradfitz
force-pushed
the
bradfitz/ptr_to_view
branch
from
a545684 to
6c69e76
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on #13390, I ran across this non-idiomatic
pointer-to-view and parallel-sorted-map accounting code that was all
just to avoid a sort later.
But the sort later when building a new netmap.NetworkMap is already a
drop in the bucket of CPU compared to how much work & allocs
mapSession.netmap and LocalBackend's spamming of the full netmap
(potentially tens of thousands of peers, MBs of JSON) out to IPNBus
clients for any tiny little change (node changing online status, etc).
Removing the parallel sorted slice let everything be simpler to reason
about, so this does that. The sort might take a bit more CPU time now
in theory, but in practice for any netmap size for which it'd matter,
the quadratic netmap IPN bus spam (which we need to fix soon) will
overshadow that little sort.
Updates #13390
Updates #1909