Make protoName unCamelCase lazy
#606
Conversation
In a large 1p Flutter android application, package:protobuf accounts for ~220ms (8%) of the CPU time during startup. This `_uncamelCase` operation takes up ~25ms, or 11% of this 220ms which occurs whenever a proto is constructed. Looking through the code, it appears that this `protoName` field is only needed for conversion to JSON which is less common. As such, make it a lazy getter to avoid this. Internal link to pivot profile view which highlights this method (top half shows callers, bottom half shows callees): pprof/user-profile?id=d8d43f484e3e7b4064a29737cbdabf77&tab=flame&flamesearch=unCamel&pivot=Precompiled_____unCamelCase_19070143_4307
There was a problem hiding this comment.
Thanks for the PR @jiahaog.
This _uncamelCase operation takes up ~25ms, or 11% of this 220ms which occurs whenever a proto is constructed.
What are the numbers like with this PR? I'm wondering how much difference this change makes.
LGTM assuming it makes a difference in the numbers.
|
I ran the startup benchmark with this change and we see a 17ms or ~0.3% in one of the tracked metrics. I also collected a profile locally and noticed that the uncamelcase no longer appears. It's a small improvement but I'm hoping that we can find more things separately which might add up to have an eventual noticeable impact on latency. |
Co-authored-by: Ömer Sinan Ağacan <[email protected]>
Just trying to understand the benchmark here -- in the PR description it says "~220ms (8%)", so the entire startup is 2750ms. If we reduce that 220ms to 17ms the startup time becomes 2547ms, and 17ms of 2547ms is 0.6%, not 0.3%. Am I missing anything? Not that it matters too much, the result looks great..
|
|
Sorry for being unclear. I'll try and elaborate further: The original profile for startup I captured locally (aggregated across 3 local runs) showed that I then patched this PR into Google3 and ran a similar benchmark in the device lab which is supposedly more accurate (and is what we use to track the historic performance of the app across different CLs). This showed an improvement of 17ms or ~0.3% against another run of the same benchmark without this change patched for a related metric. I used the device lab instead of doing it locally because the performance of my uncalibrated local device can fluctuate quite a bit and the device lab provides much more precision. Hope this clears things up, also happy to share links to the internal benchmark runs internally 🙂 |
|
Ah, OK, so this PR improves runtime 0.3% in a benchmark. Thanks for the clarification. I will document this in the commit message so that we will know why it was made lazy. I'd like to wait for @sigurdm or @mraleph's approval, but I think they're not available until next week so this may have to wait a litte bit. |
|
Sure, thanks! |
|
@jiahaog feel free to create CL internally with the same change, package is currently out of sync with internal copy so you have to do backport manually. |
In a large 1p Flutter android application, package:protobuf accounts for ~220ms (8%) of the CPU time during startup. This
_uncamelCaseoperation takes up ~25ms, or 11% of this 220ms which occurs whenever a proto is constructed.Looking through the code, it appears that this
protoNamefield is only needed for conversion to JSON which is less common. As such, make it a lazy getter to avoid this.Internal link to pivot profile view which highlights this method (top half shows callers, bottom half shows callees)