3 ms·

The thing that's missing from this (and most) articles about the described kinds of code optimizations is context: when is it appropriate to make these kinds of

by kiitos 3y ago

The thing that's missing from this (and most) articles about the described kinds of code optimizations is context: when is it appropriate to make these kinds of optimizing changes, and when is it not? > [with] this change ... the speed improved significantly [but] the readability and ease of coding decreased quite a bit The speed of this specific function, measured with the described (micro) benchmark, improved, yes, no doubt. But that's not (by itself) relevant! If you want to use an optimized implementation of some code, it has to represent a significant improvement in the overall system as measured by meaningful, agreed-upon, high-level performance metrics. If this function is called many times per user request, and the optimized version has a measurable impact on request latency, then sure, go for it! But, otherwise, making this kind of change is the textbook definition of premature optimization, never a good idea.

masklinn 3y ago
OTOH note that this optimisation can be bundled into a helper function so it's not quite as bad as some others. In fact the helper function is in the stdlib: https://pkg.go.dev/strings#Cut https://pkg.go.dev/strings#Cut This is most useful for the "split once" version, which is genuinely improved: scoreWithColor := strings.Split(score, " ") scoreStr := scoreWithColor[0] color := scoreWithColor[1] becomes scoreStr, color, _ := strings.Cut(score, " ") The iteration is worse, but not to an irredeemable degree, rounds := strings.Split(game, "; ") isPossible := false for _, round := range rounds { red, green, blue := findScores(round) if isRoundPossible(red, green, blue) { isPossible = true } else { isPossible = false // If only one round is not possible, there is // no need to process the other ones. break } } becomes: isPossible := false for { round, tail, hasNext := strings.Cut(game, ";") game = tail red, green, blue := findScores(round) if isRoundPossible(red, green, blue) { isPossible = true } else { isPossible = false // If only one round is not possible, there is // no need to process the other ones. break } if !hasNext { break } }
rivo 3y ago
When I see these kind of readability-decreasing optimizations in code reviews, I always push back. Almost always, that same Go code queries some other service over the network or loads data from the database, or similar, and nobody will ever notice the speed improvement of IndexOf over Split at the bottom line. But everyone working with that code at a later time will spend extra time (sometimes hours) just trying to understand what's going on. That's much more expensive than a few extra CPU cycles here and there. Sure, if you're writing an inner loop for some heavy workload, e.g. in graphics or maybe parsing terabytes of JSON, it makes sense. But outside of these rarely occurring situations, you should always prefer readability over low-level optimizations.

by kiitos 3y ago

masklinn 3y ago

OTOH note that this optimisation can be bundled into a helper function so it's not quite as bad as some others. In fact the helper function is in the stdlib: https://pkg.go.dev/strings#Cut https://pkg.go.dev/strings#Cut This is most useful for the "split once" version, which is genuinely improved: scoreWithColor := strings.Split(score, " ") scoreStr := scoreWithColor[0] color := scoreWithColor[1] becomes scoreStr, color, _ := strings.Cut(score, " ") The iteration is worse, but not to an irredeemable degree, rounds := strings.Split(game, "; ") isPossible := false for _, round := range rounds { red, green, blue := findScores(round) if isRoundPossible(red, green, blue) { isPossible = true } else { isPossible = false // If only one round is not possible, there is // no need to process the other ones. break } } becomes: isPossible := false for { round, tail, hasNext := strings.Cut(game, ";") game = tail red, green, blue := findScores(round) if isRoundPossible(red, green, blue) { isPossible = true } else { isPossible = false // If only one round is not possible, there is // no need to process the other ones. break } if !hasNext { break } }

rivo 3y ago

When I see these kind of readability-decreasing optimizations in code reviews, I always push back. Almost always, that same Go code queries some other service over the network or loads data from the database, or similar, and nobody will ever notice the speed improvement of IndexOf over Split at the bottom line. But everyone working with that code at a later time will spend extra time (sometimes hours) just trying to understand what's going on. That's much more expensive than a few extra CPU cycles here and there. Sure, if you're writing an inner loop for some heavy workload, e.g. in graphics or maybe parsing terabytes of JSON, it makes sense. But outside of these rarely occurring situations, you should always prefer readability over low-level optimizations.