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(icon): avoid unnecessarily parsing icon sets #18654
perf(icon): avoid unnecessarily parsing icon sets #18654
Conversation
556d09a
to
32f07f1
Compare
} | ||
|
||
/** Icon configuration whose content has already been loaded. */ | ||
type LoadedSvgIconConfig = SvgIconConfig & {svgText: string}; |
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.
It's cool to see how techniques like this develop and apply to code written when we were all first starting to use TypeScript.
32f07f1
to
143dd01
Compare
I've addressed the feedback @jelbourn. |
@crisbeto Thanks for the quick fix! |
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.
LGTM
Some internal refactoring to improve the performance of the icon registry by avoiding unnecessary work: * When an icon set is registered as a string literal, we parse it immediately into an SVG element. This is inefficient since the set may not be required immediately. These changes make it so that we store the SVG text and we only parse it when it's required. * When an icon is requested, we try to find it in all any of the icon sets which requires us to parse them since we don't know which one is supposed to have it. These changes add an inexpensive `indexOf` check before parsing so that we can quickly rule out the sets that absolutely can't have the icon. The check won't exclude 100% of the unnecessary parsing, but it should help with the majority. Once this is in we could make it slightly smarter by doing something like `svgText.indexOf(`id="${iconName}"`)`, but that's a bit more risky. Fixes angular#18644.
143dd01
to
f4bfa84
Compare
Some internal refactoring to improve the performance of the icon registry by avoiding unnecessary work: * When an icon set is registered as a string literal, we parse it immediately into an SVG element. This is inefficient since the set may not be required immediately. These changes make it so that we store the SVG text and we only parse it when it's required. * When an icon is requested, we try to find it in all any of the icon sets which requires us to parse them since we don't know which one is supposed to have it. These changes add an inexpensive `indexOf` check before parsing so that we can quickly rule out the sets that absolutely can't have the icon. The check won't exclude 100% of the unnecessary parsing, but it should help with the majority. Once this is in we could make it slightly smarter by doing something like `svgText.indexOf(`id="${iconName}"`)`, but that's a bit more risky. Fixes #18644. (cherry picked from commit 5526ab9)
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Some internal refactoring to improve the performance of the icon registry by avoiding unnecessary work:
indexOf
check before parsing so that we can quickly rule out the sets that absolutely can't have the icon. The check won't exclude 100% of the unnecessary parsing, but it should help with the majority. Once this is in we could make it slightly smarter by doing something likesvgText.indexOf('id="${iconName}"')
, but that's a bit more risky.Fixes #18644.