storage: skip Errorf for a frequent error that is never read #11208
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
some CRC32 checks can cause lots of failing calls to decodeUint32, generating a new 'fmt.Errorf()', but the error is never actually read, it is only checked for nil condition. Replacing with a (reused) more generic error prevents lots of allocations leading to less stress on the GC.
sduchesneau
force-pushed
the
storage_skip_error_alloc
branch
from
95d7819 to
f77ff78
Compare
quartzmo
changed the title
There was a problem hiding this comment.
Code change seems fine to me. I am more curious about how you are seeing this error occur internally to the library so frequently. Could you file an issue and describe what kind of data you are downloading, whether you are using storage.NewReader or NewRangeReader, whether the object has a checksum present when you look at it in the console? I would expect this failure case not to be very common in general.
product-auto-label
bot
added
the
api: storage
While hunting down memory allocations, we found lots of calls to Errorf() that were useless (because the error is never actually read, it is only checked for nil condition)
Reusing the same error prevents all these allocations, while keeping a sane behavior for future use.