reduce heap allocations #8

Summary: We don't need to recreate a new "process" every time we want to collect stats

Reviewed By: abulimov

Differential Revision: D58419351

fbshipit-source-id: 36b7e07d55270fc2ce2ef4aadbbdcea1b634a35e

cmd/sptp/main.go

@@ -30,9 +30,12 @@ import (
 )
 
 func doWork(cfg *client.Config) error {
-	stats := client.NewJSONStats()
+	stats, err := client.NewJSONStats()
+	if err != nil {
+		return err
+	}
 	go stats.Start(cfg.MonitoringPort, cfg.MetricsAggregationWindow)
-	p, err := client.NewSPTP(cfg, stats)
+	p, err := client.NewSPTP(cfg, *stats)
 	if err != nil {
 		return err
 	}

ptp/sptp/client/json_stats.go

@@ -27,23 +27,22 @@ import (
 
 // JSONStats is what we want to report as stats via http
 type JSONStats struct {
-	Stats
+	*Stats
 }
 
 // NewJSONStats returns a new JSONStats
-func NewJSONStats() *JSONStats {
-	return &JSONStats{Stats: *NewStats()}
+func NewJSONStats() (*JSONStats, error) {
+	stats, err := NewStats()
+	return &JSONStats{Stats: stats}, err
 }
 
 // Start runs http server and initializes maps
-func (s *JSONStats) Start(monitoringport int, interval time.Duration) {
+func (s JSONStats) Start(monitoringport int, interval time.Duration) {
 	// collect stats forever
 	go func() {
 		for range time.Tick(interval) {
 			// update stats on every tick
-			if err := s.CollectSysStats(); err != nil {
-				log.Warningf("failed to get system metrics %s", err)
-			}
+			s.CollectSysStats()
 		}
 	}()
 
@@ -58,7 +57,7 @@ func (s *JSONStats) Start(monitoringport int, interval time.Duration) {
 }
 
 // handleRootRequest is a handler used for all http monitoring requests
-func (s *JSONStats) handleRootRequest(w http.ResponseWriter, _ *http.Request) {
+func (s JSONStats) handleRootRequest(w http.ResponseWriter, _ *http.Request) {
 	js, err := json.Marshal(s.GetGMStats())
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusInternalServerError)
@@ -71,7 +70,7 @@ func (s *JSONStats) handleRootRequest(w http.ResponseWriter, _ *http.Request) {
 }
 
 // handleCountersRequest is a handler used for all http monitoring requests
-func (s *JSONStats) handleCountersRequest(w http.ResponseWriter, _ *http.Request) {
+func (s JSONStats) handleCountersRequest(w http.ResponseWriter, _ *http.Request) {
 	js, err := json.Marshal(s.GetCounters())
 	if err != nil {
 		http.Error(w, err.Error(), http.StatusInternalServerError)

ptp/sptp/client/json_stats_test.go

@@ -42,7 +42,8 @@ func getFreePort() (int, error) {
 }
 
 func TestJSONStats(t *testing.T) {
-	stats := NewJSONStats()
+	stats, err := NewJSONStats()
+	require.NoError(t, err)
 	port, err := getFreePort()
 	require.Nil(t, err, "Failed to allocate port")
 	url := fmt.Sprintf("http://localhost:%d", port)

ptp/sptp/client/stats.go

@@ -39,7 +39,7 @@ type StatsServer interface {
 	IncTXDelayReq()
 	IncUnsupported()
 	SetGMStats(stat *gmstats.Stat)
-	CollectSysStats() error
+	CollectSysStats()
 }
 
 // Stats is an implementation of
@@ -52,6 +52,7 @@ type Stats struct {
 	snapshot      gmstats.Stats
 	procStartTime time.Time
 	memstats      runtime.MemStats
+	proc          *process.Process
 }
 
 // clientStats is just a grouping, don't use directly
@@ -78,12 +79,14 @@ type sysStats struct {
 }
 
 // NewStats created new instance of Stats
-func NewStats() *Stats {
+func NewStats() (*Stats, error) {
+	proc, err := process.NewProcess(int32(os.Getpid()))
 	return &Stats{
 		gmStats:       gmstats.Stats{},
 		snapshot:      gmstats.Stats{},
 		procStartTime: time.Now(),
-	}
+		proc:          proc,
+	}, err
 }
 
 // SetGmsTotal atomically sets the gmsTotal
@@ -178,24 +181,18 @@ func (s *Stats) SetGMStats(stat *gmstats.Stat) {
 }
 
 // CollectSysStats gathers cpu, mem, gc statistics
-func (s *Stats) CollectSysStats() error {
+func (s *Stats) CollectSysStats() {
 	s.Lock()
 	defer s.Unlock()
 
 	runtime.ReadMemStats(&s.memstats)
-
-	// Process metrics
-	proc, err := process.NewProcess(int32(os.Getpid()))
-	if err != nil {
-		return err
-	}
 	s.uptimeSec = time.Now().Unix() - s.procStartTime.Unix()
 
-	if val, err := proc.Percent(0); err == nil {
+	if val, err := s.proc.Percent(0); err == nil {
 		s.cpuPCT = int64(val * 100)
 	}
 
-	if val, err := proc.MemoryInfo(); err == nil {
+	if val, err := s.proc.MemoryInfo(); err == nil {
 		s.rss = int64(val.RSS)
 	}
 
@@ -204,8 +201,6 @@ func (s *Stats) CollectSysStats() error {
 	// Diff between current and previous where s.gcPauseTotal acts as a previous
 	s.gcPauseNs = int64(s.memstats.PauseTotalNs) - s.gcPauseTotalNs
 	s.gcPauseTotalNs = int64(s.memstats.PauseTotalNs)
-
-	return nil
 }
 
 func runResultToGMStats(address string, r *RunResult, p3 int, selected bool) *gmstats.Stat {

ptp/sptp/client/stats_test.go

@@ -116,7 +116,8 @@ func TestSetGMStats(t *testing.T) {
 		GMAddress: "192.168.0.10",
 		Error:     "mymy",
 	}
-	s := NewStats()
+	s, err := NewStats()
+	require.NoError(t, err)
 	s.SetGMStats(gm)
 	want := gmstats.Stats{
 		gm,
@@ -125,7 +126,8 @@ func TestSetGMStats(t *testing.T) {
 }
 
 func TestInc(t *testing.T) {
-	s := NewStats()
+	s, err := NewStats()
+	require.NoError(t, err)
 	s.rxAnnounce = 42
 	s.rxSync = 43
 	s.rxDelayReq = 44
@@ -147,18 +149,19 @@ func TestInc(t *testing.T) {
 }
 
 func TestSysStats(t *testing.T) {
-	stats := NewStats()
-	time.Sleep(time.Second)
-	err := stats.CollectSysStats()
+	stats, err := NewStats()
 	require.NoError(t, err)
+	time.Sleep(time.Second)
+	stats.CollectSysStats()
 	// Sys counters are set and above 0
 	require.Less(t, int64(0), stats.goRoutines)
 	require.Less(t, int64(0), stats.rss)
 	require.Equal(t, int64(1), stats.uptimeSec)
 }
 
 func TestGetCounters(t *testing.T) {
-	stats := NewStats()
+	stats, err := NewStats()
+	require.NoError(t, err)
 	m := stats.GetCounters()
 	require.Contains(t, m, "ptp.sptp.gms.total")
 	require.Contains(t, m, "ptp.sptp.gms.available_pct")