-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RUMM-2441 Optimize BaseWireframeMapper#colorAndAlphaAsStringHexa function #1077
RUMM-2441 Optimize BaseWireframeMapper#colorAndAlphaAsStringHexa function #1077
Conversation
val colorAndAlphaAsHexa = (0xffffffff and colorAndAlpha) | ||
.toString(16) | ||
.padStart(8, '0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give some details on why this algorithm is more optimized vs the String.format
one? Did you benchmark this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I profiled this with the Android Profiler...there is a big difference there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we are using this function for each tree node...I profiled the whole snapshot function. With the String.format the difference is ~10ms + which is huge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it'd be nice to add a comment there to make sure we remember why we use a three line code instead of a one liner ;)
4199cea
to
b7d11ce
Compare
b7d11ce
to
d752382
Compare
// we are going to use the `Long.toString(radius)` method to produce the hexa | ||
// representation of the color and alpha long value because is much more faster than the | ||
// String.format(..) approach. Based on our benchmarks, because String.format uses regular | ||
// expressions under the hood, this approach is at least 2 times faster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks 👍
What does this PR do?
We were using the
String.format
approach to resolve the shape color, alpha values as a hexa string property and this was quite costly in terms of performances. We are changing this relying on theInt.toString(radius)
function which is much more faster.Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)