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

@@ -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
 	}
 

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: "/d3"}, IsLocal: true},
-				Endpoint{URL: &url.URL{Path: "d4"}, 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{

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 {
+		for i, endpoint := range s.endpoints {
-			if s.isConnected(endpoint) {
+			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 {
+	for i, endpoint := range s.endpoints {
-		if s.isConnected(endpoint) {
+		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 s.endpointStrings[i] == v.Endpoint {
-				if v.Endpoint == endpoint.Path {
+				foundBefore = true
-					foundBefore = true
-				}
-			} else {
-				if v.Endpoint == endpoint.String() {
-					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 s.endpointStrings[i] == v.Endpoint {
-				if v.Endpoint == endpoint.Path {
+				foundAfter = true
-					foundAfter = true
-				}
-			} else {
-				if v.Endpoint == endpoint.String() {
-					foundAfter = true
-				}
 			}
 		}
 		if !foundAfter {
 			result.After.Drives = append(result.After.Drives, madmin.HealDriveInfo{
 				UUID:     "",
-				Endpoint: endpoint.String(),
+				Endpoint: s.endpointStrings[i],
 				State:    madmin.DriveStateOffline,
 			})
 		}