From 46ce80758d725409368d24bd976f51fe9e9b656d Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 21 Oct 2022 21:30:40 -0700 Subject: [PATCH] portlist: update some internals to use append-style APIs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In prep for reducing garbage, being able to reuse memory. So far this doesn't actually reuse much. This is just changing signatures around. But some improvement in any case: bradfitz@tsdev:~/src/tailscale.com$ ~/go/bin/benchstat before after name old time/op new time/op delta GetList-8 11.8ms ± 9% 9.9ms ± 3% -15.98% (p=0.000 n=10+10) name old alloc/op new alloc/op delta GetList-8 99.5kB ± 2% 91.9kB ± 0% -7.62% (p=0.000 n=9+9) name old allocs/op new allocs/op delta GetList-8 3.05k ± 1% 2.93k ± 0% -3.83% (p=0.000 n=8+9) More later, once parsers can reuse strings from previous parses. Updates #5958 Change-Id: I76cd5048246dd24d11c4e263d8bb8041747fb2b0 Signed-off-by: Brad Fitzpatrick --- portlist/netstat.go | 17 +++++++++++------ portlist/netstat_exec.go | 4 ++-- portlist/netstat_test.go | 5 ++++- portlist/poller.go | 11 +++++++---- portlist/portlist.go | 11 ++++++----- portlist/portlist_ios.go | 2 +- portlist/portlist_js.go | 4 ++-- portlist/portlist_linux.go | 6 +++--- portlist/portlist_linux_test.go | 4 +++- portlist/portlist_macos.go | 4 ++-- portlist/portlist_other.go | 4 ++-- portlist/portlist_test.go | 9 ++++++--- portlist/portlist_windows.go | 8 ++++---- 13 files changed, 53 insertions(+), 36 deletions(-) diff --git a/portlist/netstat.go b/portlist/netstat.go index 9951f0252..41e775b4f 100644 --- a/portlist/netstat.go +++ b/portlist/netstat.go @@ -55,7 +55,7 @@ type nothing struct{} // formats that we can parse without special detection logic. // Unfortunately, options to filter by proto or state are non-portable, // so we'll filter for ourselves. -func parsePortsNetstat(output string) List { +func appendParsePortsNetstat(base []Port, output string) []Port { m := map[Port]nothing{} lines := strings.Split(string(output), "\n") @@ -131,13 +131,18 @@ func parsePortsNetstat(output string) List { lastline = "" } - l := []Port{} + ret := base for p := range m { - l = append(l, p) + ret = append(ret, p) } - sort.Slice(l, func(i, j int) bool { - return (&l[i]).lessThan(&l[j]) + + // Only sort the part we appended. It's up to the caller to sort the whole + // thing if they'd like. In practice the caller's base will have len 0, + // though, so the whole thing will be sorted. + toSort := ret[len(base):] + sort.Slice(toSort, func(i, j int) bool { + return (&toSort[i]).lessThan(&toSort[j]) }) - return l + return ret } diff --git a/portlist/netstat_exec.go b/portlist/netstat_exec.go index 0a9911a01..45014cc41 100644 --- a/portlist/netstat_exec.go +++ b/portlist/netstat_exec.go @@ -25,7 +25,7 @@ func hideWindow(c *exec.Cmd) *exec.Cmd { return c } -func listPortsNetstat(arg string) (List, error) { +func appendListeningPortsNetstat(base []Port, arg string) ([]Port, error) { exe, err := exec.LookPath("netstat") if err != nil { return nil, fmt.Errorf("netstat: lookup: %v", err) @@ -40,5 +40,5 @@ func listPortsNetstat(arg string) (List, error) { return nil, fmt.Errorf("netstat: %v (%q)", err, stderr) } - return parsePortsNetstat(string(output)), nil + return appendParsePortsNetstat(base, string(output)), nil } diff --git a/portlist/netstat_test.go b/portlist/netstat_test.go index c6b18f0a8..04a093fd6 100644 --- a/portlist/netstat_test.go +++ b/portlist/netstat_test.go @@ -2,6 +2,9 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. +//go:build !ios && !js +// +build !ios,!js + package portlist import ( @@ -83,7 +86,7 @@ func TestParsePortsNetstat(t *testing.T) { Port{"udp", 9353, "iTunes", ""}, } - pl := parsePortsNetstat(netstatOutput) + pl := appendParsePortsNetstat(nil, netstatOutput) jgot, _ := json.MarshalIndent(pl, "", "\t") jwant, _ := json.MarshalIndent(want, "", "\t") if len(pl) != len(want) { diff --git a/portlist/poller.go b/portlist/poller.go index 2405e5e41..12a9e6e54 100644 --- a/portlist/poller.go +++ b/portlist/poller.go @@ -19,13 +19,16 @@ type Poller struct { // Run completes, after which Err can be checked. C <-chan List - c chan List - // Err is the error from the final GetList call. It is only // valid to read once C has been closed. Err is nil if Close // is called or the context is canceled. Err error + // scatch is memory for Poller.getList to reuse between calls. + scratch []Port + + c chan List // the unconstrained version of the exported C above + quitCh chan struct{} // close this to force exit prev List // most recent data } @@ -45,7 +48,7 @@ func NewPoller() (*Poller, error) { // Do one initial poll synchronously so we can return an error // early. var err error - p.prev, err = getList(nil) + p.prev, err = p.getList() if err != nil { return nil, err } @@ -76,7 +79,7 @@ func (p *Poller) Run(ctx context.Context) error { for { select { case <-tick.C: - pl, err := getList(p.prev) + pl, err := p.getList() if err != nil { p.Err = err return err diff --git a/portlist/portlist.go b/portlist/portlist.go index feda94c29..122b81c03 100644 --- a/portlist/portlist.go +++ b/portlist/portlist.go @@ -76,18 +76,19 @@ func (pl List) String() string { var debugDisablePortlist = envknob.RegisterBool("TS_DEBUG_DISABLE_PORTLIST") -func getList(prev List) (List, error) { +func (p *Poller) getList() (List, error) { if debugDisablePortlist() { return nil, nil } - pl, err := listPorts() + var err error + p.scratch, err = appendListeningPorts(p.scratch[:0]) if err != nil { return nil, fmt.Errorf("listPorts: %s", err) } - pl = sortAndDedup(pl) - if pl.sameInodes(prev) { + pl := sortAndDedup(p.scratch) + if pl.sameInodes(p.prev) { // Nothing changed, skip inode lookup - return prev, nil + return p.prev, nil } pl, err = addProcesses(pl) if err != nil { diff --git a/portlist/portlist_ios.go b/portlist/portlist_ios.go index a4a85f8b5..3970ecde2 100644 --- a/portlist/portlist_ios.go +++ b/portlist/portlist_ios.go @@ -14,7 +14,7 @@ import ( const pollInterval = 9999 * time.Hour -func listPorts() (List, error) { +func appendListeningPorts(base []Port) ([]Port, error) { return nil, errors.New("not implemented") } diff --git a/portlist/portlist_js.go b/portlist/portlist_js.go index 6e6b18ed2..3f4ce6a76 100644 --- a/portlist/portlist_js.go +++ b/portlist/portlist_js.go @@ -8,8 +8,8 @@ import "time" const pollInterval = 365 * 24 * time.Hour -func listPorts() (List, error) { - return nil, nil +func appendListeningPorts(base []Port) ([]Port, error) { + return base, nil } func addProcesses(pl []Port) ([]Port, error) { diff --git a/portlist/portlist_linux.go b/portlist/portlist_linux.go index 05e61739c..68d9c9de8 100644 --- a/portlist/portlist_linux.go +++ b/portlist/portlist_linux.go @@ -35,11 +35,11 @@ const ( v4Any = "00000000:0000" ) -func listPorts() (List, error) { +func appendListeningPorts(base []Port) ([]Port, error) { + ret := base if sawProcNetPermissionErr.Load() { - return nil, nil + return ret, nil } - var ret []Port var br *bufio.Reader for _, fname := range sockfiles { diff --git a/portlist/portlist_linux_test.go b/portlist/portlist_linux_test.go index ea5773a26..d863cea0e 100644 --- a/portlist/portlist_linux_test.go +++ b/portlist/portlist_linux_test.go @@ -132,8 +132,10 @@ func BenchmarkParsePorts(b *testing.B) { func BenchmarkListPorts(b *testing.B) { b.ReportAllocs() + var base []Port for i := 0; i < b.N; i++ { - _, err := listPorts() + var err error + base, err = appendListeningPorts(base[:0]) if err != nil { b.Fatal(err) } diff --git a/portlist/portlist_macos.go b/portlist/portlist_macos.go index 9ed541679..f79c7756f 100644 --- a/portlist/portlist_macos.go +++ b/portlist/portlist_macos.go @@ -21,8 +21,8 @@ import ( // We have to run netstat, which is a bit expensive, so don't do it too often. const pollInterval = 5 * time.Second -func listPorts() (List, error) { - return listPortsNetstat("-na") +func appendListeningPorts(base []Port) ([]Port, error) { + return appendListeningPortsNetstat(base, "-na") } var lsofFailed int64 // atomic bool diff --git a/portlist/portlist_other.go b/portlist/portlist_other.go index f18a95d2c..8f8e06706 100644 --- a/portlist/portlist_other.go +++ b/portlist/portlist_other.go @@ -12,8 +12,8 @@ import "time" // We have to run netstat, which is a bit expensive, so don't do it too often. const pollInterval = 5 * time.Second -func listPorts() (List, error) { - return listPortsNetstat("-na") +func appendListeningPorts(base []Port) ([]Port, error) { + return appendListeningPortsNetstat(base, "-na") } func addProcesses(pl []Port) ([]Port, error) { diff --git a/portlist/portlist_test.go b/portlist/portlist_test.go index 3d11df867..08d7995a6 100644 --- a/portlist/portlist_test.go +++ b/portlist/portlist_test.go @@ -14,7 +14,8 @@ import ( func TestGetList(t *testing.T) { tstest.ResourceCheck(t) - pl, err := getList(nil) + var p Poller + pl, err := p.getList() if err != nil { t.Fatal(err) } @@ -34,7 +35,8 @@ func TestIgnoreLocallyBoundPorts(t *testing.T) { defer ln.Close() ta := ln.Addr().(*net.TCPAddr) port := ta.Port - pl, err := getList(nil) + var p Poller + pl, err := p.getList() if err != nil { t.Fatal(err) } @@ -194,8 +196,9 @@ func TestSameInodes(t *testing.T) { func BenchmarkGetList(b *testing.B) { b.ReportAllocs() + var p Poller for i := 0; i < b.N; i++ { - _, err := getList(nil) + _, err := p.getList() if err != nil { b.Fatal(err) } diff --git a/portlist/portlist_windows.go b/portlist/portlist_windows.go index 4c2578bba..8218eddc5 100644 --- a/portlist/portlist_windows.go +++ b/portlist/portlist_windows.go @@ -15,11 +15,11 @@ import ( // Forking on Windows is insanely expensive, so don't do it too often. const pollInterval = 5 * time.Second -func listPorts() (List, error) { +func appendListeningPorts(base []Port) ([]Port, error) { // TODO(bradfitz): stop shelling out to netstat and use the // net/netstat package instead. When doing so, be sure to filter // out all of 127.0.0.0/8 and not just 127.0.0.1. - return listPortsNetstat("-na") + return appendListeningPortsNetstat(base, "-na") } func addProcesses(pl []Port) ([]Port, error) { @@ -31,9 +31,9 @@ func addProcesses(pl []Port) ([]Port, error) { } defer tok.Close() if !tok.IsElevated() { - return listPortsNetstat("-na") + return appendListeningPortsNetstat(nil, "-na") } - return listPortsNetstat("-nab") + return appendListeningPortsNetstat(nil, "-nab") } func init() {