Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(icon): large SVG files can cause icon caching to hang #11653

Merged
merged 1 commit into from
Mar 6, 2019

Conversation

Splaktar
Copy link
Contributor

@Splaktar Splaktar commented Feb 25, 2019

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Large SVGs (170KB+) can cause the current caching RegEx code to hang and use 100% CPU for a long amount of time, freezing the app.

Issue Number:
Fixes #11651. Relates to #8689. Relates to #11342. Relates to #11635.

What is the new behavior?

No more hangs processing large cached SVGs

  • use querySelectorAll as much as possible
  • reduce the use of RegEx on large chunks of DOM

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

The reporter added a demo full of various SVGs in a dialog. I verified that these changes fix all of those SVGs.

@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Feb 25, 2019
@Splaktar Splaktar self-assigned this Feb 25, 2019
@Splaktar Splaktar added this to the 1.1.14 milestone Feb 25, 2019
@Splaktar Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs P2: required Issues that must be fixed. type: bug severity: regression This issue is related to a regression labels Feb 25, 2019
@Splaktar Splaktar force-pushed the svgIcon-largeFileRegexPerf branch from a4f6ab5 to 46563a5 Compare February 25, 2019 06:21
use querySelectorAll as much as possible
reduce the use of RegEx on large chunks of DOM

Fixes #11651. Relates to #8689. Relates to #11342. Relates to #11635.
@Splaktar Splaktar force-pushed the svgIcon-largeFileRegexPerf branch from 132a575 to 63ba492 Compare March 1, 2019 06:06
@Splaktar
Copy link
Contributor Author

Splaktar commented Mar 1, 2019

I've finished my testing on IE11.

I had to tweak it to use referencingElement.textContent = angular.element(svgElement)[0].innerHTML; instead of referencingElement= angular.element(svgElement)[0]; as this wasn't updating the DOM.

There seem to be some other issues, possibly SVGs that don't well support IE11, with 5 of the supplied SVGs in the CodePen above. The first time that many of them are displayed in IE11, they are black silhouettes. The second time when loaded from the cache (after this fix), they render properly with their full textures and colors. I compared the SVG structure in the DOM and there were no changes before/after that would explain this behavior.

I tested this with 1.1.10 and the same images were black. However, the second time they are displayed, they remained black.

So as far as I can tell, md-icon SVG support in IE11 will be improved by this change 😄

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Mar 1, 2019
@josephperrott josephperrott merged commit 6a68c96 into master Mar 6, 2019
@Splaktar Splaktar deleted the svgIcon-largeFileRegexPerf branch March 7, 2019 06:43
@Splaktar Splaktar assigned josephperrott and unassigned jelbourn Mar 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P2: required Issues that must be fixed. pr: merge ready This PR is ready for a caretaker to review severity: regression This issue is related to a regression type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

icon(svg): large icon from cache causes 100% CPU during RegEx replacement
4 participants