From 813e0fc1a86359a4b1a2c93b046f730cb13b27d3 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 24 Mar 2020 18:53:24 -0700 Subject: [PATCH] fix: optimize isConnected to avoid url.String() conversions (#9202) Stringifying in a loop can tax the system, avoid this and convert the endpoints to strings early on and remember them for the lifetime of the server. --- cmd/endpoint.go | 6 +++- cmd/endpoint_test.go | 24 +++------------- cmd/xl-sets.go | 65 +++++++++++++++++++------------------------- 3 files changed, 37 insertions(+), 58 deletions(-) diff --git a/cmd/endpoint.go b/cmd/endpoint.go index e2c2b983b..9429925ad 100644 --- a/cmd/endpoint.go +++ b/cmd/endpoint.go @@ -171,7 +171,11 @@ func NewEndpoint(arg string) (ep Endpoint, e error) { if isHostIP(arg) { return ep, fmt.Errorf("invalid URL endpoint format: missing scheme http or https") } - u = &url.URL{Path: path.Clean(arg)} + absArg, err := filepath.Abs(arg) + if err != nil { + return Endpoint{}, fmt.Errorf("absolute path failed %s", err) + } + u = &url.URL{Path: path.Clean(absArg)} isLocal = true } diff --git a/cmd/endpoint_test.go b/cmd/endpoint_test.go index d828b06f3..2efa9eea3 100644 --- a/cmd/endpoint_test.go +++ b/cmd/endpoint_test.go @@ -37,19 +37,7 @@ func TestNewEndpoint(t *testing.T) { expectedType EndpointType expectedErr error }{ - {"foo", Endpoint{URL: &url.URL{Path: "foo"}, IsLocal: true}, PathEndpointType, nil}, {"/foo", Endpoint{URL: &url.URL{Path: "/foo"}, IsLocal: true}, PathEndpointType, nil}, - {`\foo`, Endpoint{URL: &url.URL{Path: `\foo`}, IsLocal: true}, PathEndpointType, nil}, - {"C", Endpoint{URL: &url.URL{Path: `C`}, IsLocal: true}, PathEndpointType, nil}, - {"C:", Endpoint{URL: &url.URL{Path: `C:`}, IsLocal: true}, PathEndpointType, nil}, - {"C:/", Endpoint{URL: &url.URL{Path: "C:"}, IsLocal: true}, PathEndpointType, nil}, - {`C:\`, Endpoint{URL: &url.URL{Path: `C:\`}, IsLocal: true}, PathEndpointType, nil}, - {`C:\foo`, Endpoint{URL: &url.URL{Path: `C:\foo`}, IsLocal: true}, PathEndpointType, nil}, - {"C:/foo", Endpoint{URL: &url.URL{Path: "C:/foo"}, IsLocal: true}, PathEndpointType, nil}, - {`C:\\foo`, Endpoint{URL: &url.URL{Path: `C:\\foo`}, IsLocal: true}, PathEndpointType, nil}, - {"http:path", Endpoint{URL: &url.URL{Path: "http:path"}, IsLocal: true}, PathEndpointType, nil}, - {"http:/path", Endpoint{URL: &url.URL{Path: "http:/path"}, IsLocal: true}, PathEndpointType, nil}, - {"http:///path", Endpoint{URL: &url.URL{Path: "http:/path"}, IsLocal: true}, PathEndpointType, nil}, {"https://example.org/path", Endpoint{URL: u2, IsLocal: false}, URLEndpointType, nil}, {"http://192.168.253.200/path", Endpoint{URL: u4, IsLocal: false}, URLEndpointType, nil}, {"", Endpoint{}, -1, fmt.Errorf("empty or root endpoint is not supported")}, @@ -101,7 +89,6 @@ func TestNewEndpoints(t *testing.T) { args []string expectedErr error }{ - {[]string{"d1", "d2", "d3", "d4"}, nil}, {[]string{"/d1", "/d2", "/d3", "/d4"}, nil}, {[]string{"http://localhost/d1", "http://localhost/d2", "http://localhost/d3", "http://localhost/d4"}, nil}, {[]string{"http://example.org/d1", "http://example.com/d1", "http://example.net/d1", "http://example.edu/d1"}, nil}, @@ -225,20 +212,17 @@ func TestCreateEndpoints(t *testing.T) { // FS Setup {"localhost:9000", [][]string{{"http://localhost/d1"}}, "", Endpoints{}, -1, fmt.Errorf("use path style endpoint for FS setup")}, - {":443", [][]string{{"d1"}}, ":443", Endpoints{Endpoint{URL: &url.URL{Path: "d1"}, IsLocal: true}}, FSSetupType, nil}, + {":443", [][]string{{"/d1"}}, ":443", Endpoints{Endpoint{URL: &url.URL{Path: "/d1"}, IsLocal: true}}, FSSetupType, nil}, {"localhost:10000", [][]string{{"/d1"}}, "localhost:10000", Endpoints{Endpoint{URL: &url.URL{Path: "/d1"}, IsLocal: true}}, FSSetupType, nil}, - {"localhost:10000", [][]string{{"./d1"}}, "localhost:10000", Endpoints{Endpoint{URL: &url.URL{Path: "d1"}, IsLocal: true}}, FSSetupType, nil}, - {"localhost:10000", [][]string{{`\d1`}}, "localhost:10000", Endpoints{Endpoint{URL: &url.URL{Path: `\d1`}, IsLocal: true}}, FSSetupType, nil}, - {"localhost:10000", [][]string{{`.\d1`}}, "localhost:10000", Endpoints{Endpoint{URL: &url.URL{Path: `.\d1`}, IsLocal: true}}, FSSetupType, nil}, {"localhost:9000", [][]string{{"https://127.0.0.1:9000/d1", "https://localhost:9001/d1", "https://example.com/d1", "https://example.com/d2"}}, "", Endpoints{}, -1, fmt.Errorf("path '/d1' can not be served by different port on same address")}, // XL Setup with PathEndpointType - {":1234", [][]string{{"/d1", "/d2", "d3", "d4"}}, ":1234", + {":1234", [][]string{{"/d1", "/d2", "/d3", "/d4"}}, ":1234", Endpoints{ Endpoint{URL: &url.URL{Path: "/d1"}, IsLocal: true}, Endpoint{URL: &url.URL{Path: "/d2"}, IsLocal: true}, - Endpoint{URL: &url.URL{Path: "d3"}, IsLocal: true}, - Endpoint{URL: &url.URL{Path: "d4"}, IsLocal: true}, + Endpoint{URL: &url.URL{Path: "/d3"}, IsLocal: true}, + Endpoint{URL: &url.URL{Path: "/d4"}, IsLocal: true}, }, XLSetupType, nil}, // DistXL Setup with URLEndpointType {":9000", [][]string{{"http://localhost/d1", "http://localhost/d2", "http://localhost/d3", "http://localhost/d4"}}, ":9000", Endpoints{ diff --git a/cmd/xl-sets.go b/cmd/xl-sets.go index d2635efc3..05147e524 100644 --- a/cmd/xl-sets.go +++ b/cmd/xl-sets.go @@ -84,6 +84,11 @@ type xlSets struct { // List of endpoints provided on the command line. endpoints Endpoints + // String version of all the endpoints, an optimization + // to avoid url.String() conversion taking CPU on + // large disk setups. + endpointStrings []string + // Total number of sets and the number of disks per set. setCount, drivesPerSet int @@ -104,7 +109,7 @@ type xlSets struct { } // isConnected - checks if the endpoint is connected or not. -func (s *xlSets) isConnected(endpoint Endpoint) bool { +func (s *xlSets) isConnected(endpointStr string) bool { s.xlDisksMu.RLock() defer s.xlDisksMu.RUnlock() @@ -113,12 +118,6 @@ func (s *xlSets) isConnected(endpoint Endpoint) bool { if s.xlDisks[i][j] == nil { continue } - var endpointStr string - if endpoint.IsLocal { - endpointStr = endpoint.Path - } else { - endpointStr = endpoint.String() - } if s.xlDisks[i][j].String() != endpointStr { continue } @@ -175,8 +174,8 @@ func findDiskIndex(refFormat, format *formatXLV3) (int, int, error) { func (s *xlSets) connectDisksWithQuorum() { var onlineDisks int for onlineDisks < len(s.endpoints)/2 { - for _, endpoint := range s.endpoints { - if s.isConnected(endpoint) { + for i, endpoint := range s.endpoints { + if s.isConnected(s.endpointStrings[i]) { continue } disk, format, err := connectEndpoint(endpoint) @@ -197,15 +196,15 @@ func (s *xlSets) connectDisksWithQuorum() { } // Sleep for a while - so that we don't go into // 100% CPU when half the disks are online. - time.Sleep(500 * time.Millisecond) + time.Sleep(100 * time.Millisecond) } } // connectDisks - attempt to connect all the endpoints, loads format // and re-arranges the disks in proper position. func (s *xlSets) connectDisks() { - for _, endpoint := range s.endpoints { - if s.isConnected(endpoint) { + for i, endpoint := range s.endpoints { + if s.isConnected(s.endpointStrings[i]) { continue } disk, format, err := connectEndpoint(endpoint) @@ -237,18 +236,13 @@ func (s *xlSets) connectDisks() { // endpoints by reconnecting them and making sure to place them into right position in // the set topology, this monitoring happens at a given monitoring interval. func (s *xlSets) monitorAndConnectEndpoints(ctx context.Context, monitorInterval time.Duration) { - - ticker := time.NewTicker(monitorInterval) - // Stop the timer. - defer ticker.Stop() - for { select { case <-ctx.Done(): return case <-s.disksConnectDoneCh: return - case <-ticker.C: + case <-time.After(monitorInterval): s.connectDisks() } } @@ -277,12 +271,21 @@ const defaultMonitorConnectEndpointInterval = time.Second * 10 // Set to 10 secs // Initialize new set of erasure coded sets. func newXLSets(endpoints Endpoints, format *formatXLV3, setCount int, drivesPerSet int) (*xlSets, error) { + endpointStrings := make([]string, len(endpoints)) + for _, endpoint := range endpoints { + if endpoint.IsLocal { + endpointStrings = append(endpointStrings, endpoint.Path) + } else { + endpointStrings = append(endpointStrings, endpoint.String()) + } + } // Initialize the XL sets instance. s := &xlSets{ sets: make([]*xlObjects, setCount), xlDisks: make([][]StorageAPI, setCount), xlLockers: make([][]dsync.NetLocker, setCount), endpoints: endpoints, + endpointStrings: endpointStrings, setCount: setCount, drivesPerSet: drivesPerSet, format: format, @@ -1575,42 +1578,30 @@ func (s *xlSets) HealBucket(ctx context.Context, bucket string, dryRun, remove b result.After.Drives = append(result.After.Drives, healResult.After.Drives...) } - for _, endpoint := range s.endpoints { + for i := range s.endpoints { var foundBefore bool for _, v := range result.Before.Drives { - if endpoint.IsLocal { - if v.Endpoint == endpoint.Path { - foundBefore = true - } - } else { - if v.Endpoint == endpoint.String() { - foundBefore = true - } + if s.endpointStrings[i] == v.Endpoint { + foundBefore = true } } if !foundBefore { result.Before.Drives = append(result.Before.Drives, madmin.HealDriveInfo{ UUID: "", - Endpoint: endpoint.String(), + Endpoint: s.endpointStrings[i], State: madmin.DriveStateOffline, }) } var foundAfter bool for _, v := range result.After.Drives { - if endpoint.IsLocal { - if v.Endpoint == endpoint.Path { - foundAfter = true - } - } else { - if v.Endpoint == endpoint.String() { - foundAfter = true - } + if s.endpointStrings[i] == v.Endpoint { + foundAfter = true } } if !foundAfter { result.After.Drives = append(result.After.Drives, madmin.HealDriveInfo{ UUID: "", - Endpoint: endpoint.String(), + Endpoint: s.endpointStrings[i], State: madmin.DriveStateOffline, }) }