Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

process walker perfs: optimize readLimits and readStats #2491

Merged

Conversation

alban
Copy link
Contributor

@alban alban commented May 2, 2017

  • optimize readStats
  • optimize readLimits
  • cache limits and cmdline/name after parsing

Fixes #2461

}
}
for ; pos < len(buf) && buf[pos] != ' '; pos++ {
threads = threads*10 + int(buf[pos]-'0')

This comment was marked as abuse.

@2opremio
Copy link
Contributor

2opremio commented May 3, 2017

@alban I don't doubt that the code is much faster now but it's very hard to read. Please try to refactor it. I think that separating the parsing of integers from the space-counting code would already be an improvement.

Also, are you sure that parsing ints manually provides an improvement? I would be surprised if the Golang library wasn't optimized in this respect.

@alban
Copy link
Contributor Author

alban commented May 3, 2017

The issue with strconv.Atoi or strconv.ParseInt is that they don't stop parsing when finding a non-digit such as a space and they return an error.

The previous code was using strings.Fields to allocate and build a string compatible with strconv.Atoi (containing only the digits without extra spaces). The pprof graphs showed that it was strings.Fields taking the cpu rather than strconv.Atoi but we cannot use Atoi without Fields.

fmt.Sscanf("%d "...) is able to work with extra spaces but it's also slower.

I'll try to refactor this.

@alban alban force-pushed the alban/perf-proc-walker-2 branch from c540424 to b4b73df Compare May 3, 2017 15:18
@alban
Copy link
Contributor Author

alban commented May 3, 2017

Code refactored and rebased.

Now that #2456 is merged in master, I ran performance tests again to see if this PR was still bringing better perfs. I used the test scenario described in #2461 (comment)

Master branch

Up to 95660e3 (so #2456 is included)

pprof-branch-master

This PR

pprof-branch-2491

Conclusion

The function github.com/weaveworks/scope/probe/process.(*walker).Walk takes respectively:

  • 15.32s
  • 12.91s (16% faster)

Overall, the graphs' titles say respectively:

  • 36.37s
  • 31.32s (19% faster overall, probably coincidental)

}

var softLimit uint64
for ; pos < len(content) && buf[pos] != ' '; pos++ {

This comment was marked as abuse.

This comment was marked as abuse.


// parseIntWithSpaces is similar to strconv.ParseInt but stops parsing when
// reading a space instead of returning an error
func parseIntWithSpaces(buf *[]byte, pos *int) (ret int) {

This comment was marked as abuse.

This comment was marked as abuse.

@alban alban force-pushed the alban/perf-proc-walker-2 branch from b4b73df to 598c6a0 Compare May 5, 2017 11:07
@2opremio 2opremio merged commit dfb8fab into weaveworks:master May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants