Some small optimisations #3261
Some small optimisations #3261
Conversation
|
What benchmark are you running in your before/after? |
| @@ -108,10 +108,19 @@ func resolvePluginValue(values []string, subrepo string) []string { | |||
| continue // I guess it wasn't a build label. Leave it alone. | |||
| } | |||
| // Force the full build label including empty subrepo so this is portable | |||
| v = fmt.Sprintf("///%v//%v:%v", l.Subrepo, l.PackageName, l.Name) | |||
| var buf strings.Builder | |||
There was a problem hiding this comment.
This is pretty unwieldy. Are we really calling this so frequently that it's worth making the code that much more complicated?
I'd expect under most circumstances the vast majority of wall-clock time to be spent on the build steps themselves, rather than anything to do with parsing BUILD files?
There was a problem hiding this comment.
Or even just using string concatenation:
v = `///`+l.Subrepo+`//`+l.PackageName+`:`+l.Name
But this won't call into the Stringer interface if any of these variables aren't a string.
There was a problem hiding this comment.
It's called a lot more often than you'd expect. The way subrepos + plugins work with go_repo each being their own subrepo, it's called often.
I'd expect under most circumstances the vast majority of wall-clock time to be spent on the build steps themselves, rather than anything to do with parsing BUILD files?
Well it depends what you're doing. Things like plz query changes are usually blocked by parsing files.
The performance one we run after merge ( |
| @@ -108,10 +108,19 @@ func resolvePluginValue(values []string, subrepo string) []string { | |||
| continue // I guess it wasn't a build label. Leave it alone. | |||
| } | |||
| // Force the full build label including empty subrepo so this is portable | |||
| v = fmt.Sprintf("///%v//%v:%v", l.Subrepo, l.PackageName, l.Name) | |||
| var buf strings.Builder | |||
There was a problem hiding this comment.
Add a comment here (or at the top of the file) to explain why you're not using fmt.Sprintf
Reduces one allocation in
provideForand a few other things in paths of varying temperatures (notably getting rid offmt.Sprintfwhich isn't the most efficient way to put strings together, although it is fairly concise).Before:
After: