NVIDIA / aistore

AIStore: scalable storage for AI applications
https://aistore.nvidia.com
MIT License
1.26k stars 172 forks source link

About `fmt.Sprintf` or `+` as string concat optimization proposal #86

Closed Liberxue closed 2 years ago

Liberxue commented 2 years ago

hey folks:

I am currently working on HPC high performance computing

and aistore is used in the storage deployment of our project.

I found that the aistore source code uses a large number of fmt.Sprintf or + as string concat performance is slower performance, because it needs to open up a new space every time

A Builder is used to efficiently build a string using Write methods. It minimizes memory copying.

I would like to submit a PR to improve this question, would you please accept it?

VirrageS commented 2 years ago

We indeed use a lot of fmt.Sprintf or +. But most of these cases are in error-paths in which we don't really care about performance or rather, we don't care as much as we do in happy-paths/hot-paths. If we are using fmt.Sprintf in hot-paths then that's not optimal and indeed we should look at something more efficient.

Also I don't think strings.Builder is any more efficient than + in most of the cases - if you have benchmarks that state otherwise please let me know. But I agree that strings.Builder might be faster than fmt.Sprintf because it doesn't do extrapolation which fmt.Sprintf must do.

We are of course open to MRs/PRs and if it will have sensible performance improvements we definitely are going to accept. But please keep in mind that we still value readability in cases when the performance is not critical.

Liberxue commented 2 years ago

THX @VirrageS, good to hear from you:

I use strings.Builder to wrap a Sprintf Function instead of fmt.Sprintf What do you think? This does not affect readability, nor does it affect performance.

VirrageS commented 2 years ago

Sounds good, but then you have zillion functions which are just concatenating strings.

I think I would suggest only sticking to places where the performance is really important/relevant - in those cases whatever works is fine as it is justified.

Liberxue commented 2 years ago

I create a unit test in the original code, and then replace it with Sprintf, and use the created unit test to ensure that the test of the code before and after the modification is the same. Avoid errors in concatenating strings.

Is it possible to create a new branch, I will submit a PR,

VirrageS commented 2 years ago

Looks awesome :) !

Is it possible to create a new branch, I will submit a PR

You don't need to create a new branch. You can create PR from your fork, see: https://docs.github.com/en/github-ae@latest/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request-from-a-fork