-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
perf: Improve performance of toTitleCase, register with lower and tit… #6148
Conversation
5af4492
to
81995b8
Compare
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.
Do we need any additional tests for getChild/registerComponent?
@@ -358,8 +359,6 @@ class Component { | |||
return; | |||
} | |||
|
|||
name = toTitleCase(name); |
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.
so, basically, this is the big gain here? Because we register the component both as component
and as Component
, we can just assume it'll be available and we don't need to toTitleCase the name?
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.
exactly!
src/js/component.js
Outdated
@@ -432,7 +431,8 @@ class Component { | |||
componentName = componentName || (component.name && toTitleCase(component.name())); | |||
|
|||
if (componentName) { | |||
this.childNameIndex_[componentName] = component; | |||
this.childNameIndex_[toTitleCase(componentName)] = component; |
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.
Is this needed? The behavior related to this isn't changing. Same with line 484.
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.
We need this because, getChild
used to toTitleCase
the name
that it was given. Now we won't do that because we register both possible names during addChild
.
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.
oh, didn't realize that getChild used this property. However, it sounds like we don't need to call toTitleCase
again because it sounds like componentName
is always toTitleCase
d per lines 430 and 388. So, we would only need to register the toLowerCase version.
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.
👍
ToTileCase is one of our biggest startup functions, taking about 12ms to run. This pull request speeds that up by:
toTitleCase
function fastertoLowerCase
function so that we can register title and lower case tech namestoTitleCase
in most common scenarioswith all this done
toTitleCase
+toLowerCase
take about0.6ms
combined!