Skip to content

Commit

Permalink
perf(icon): avoid unnecessarily parsing icon sets
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@crisbeto @mmalerba
crisbeto authored and mmalerba committed Apr 9, 2020
1 parent 1921df3 commit f4bfa84
Showing 1 changed file with 52 additions and 62 deletions.
114 changes: 52 additions & 62 deletions src/material/icon/icon-registry.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,22 +79,17 @@ export interface IconOptions {
* @docs-private
*/
class SvgIconConfig {
url: SafeResourceUrl | null;
svgElement: SVGElement | null;

constructor(url: SafeResourceUrl, options?: IconOptions);
constructor(svgElement: SVGElement, options?: IconOptions);
constructor(data: SafeResourceUrl | SVGElement, public options?: IconOptions) {
// Note that we can't use `instanceof SVGElement` here,
// because it'll break during server-side rendering.
if (!!(data as any).nodeName) {
this.svgElement = data as SVGElement;
} else {
this.url = data as SafeResourceUrl;
}
}
constructor(
public url: SafeResourceUrl,
public svgText: string | null,
public options?: IconOptions) {}
}

/** Icon configuration whose content has already been loaded. */
type LoadedSvgIconConfig = SvgIconConfig & {svgText: string};

/**
* Service to register and display icons used by the `<mat-icon>` component.
* - Registers icon URLs by namespace and name.
Expand Down Expand Up @@ -168,7 +163,7 @@ export class MatIconRegistry implements OnDestroy {
*/
addSvgIconInNamespace(namespace: string, iconName: string, url: SafeResourceUrl,
options?: IconOptions): this {
return this._addSvgIconConfig(namespace, iconName, new SvgIconConfig(url, options));
return this._addSvgIconConfig(namespace, iconName, new SvgIconConfig(url, null, options));
}

/**
Expand All @@ -179,14 +174,14 @@ export class MatIconRegistry implements OnDestroy {
*/
addSvgIconLiteralInNamespace(namespace: string, iconName: string, literal: SafeHtml,
options?: IconOptions): this {
const sanitizedLiteral = this._sanitizer.sanitize(SecurityContext.HTML, literal);
const cleanLiteral = this._sanitizer.sanitize(SecurityContext.HTML, literal);

if (!sanitizedLiteral) {
if (!cleanLiteral) {
throw getMatIconFailedToSanitizeLiteralError(literal);
}

const svgElement = this._createSvgElementForSingleIcon(sanitizedLiteral, options);
return this._addSvgIconConfig(namespace, iconName, new SvgIconConfig(svgElement, options));
return this._addSvgIconConfig(namespace, iconName,
new SvgIconConfig('', cleanLiteral, options));
}

/**
Expand All @@ -211,7 +206,7 @@ export class MatIconRegistry implements OnDestroy {
* @param url
*/
addSvgIconSetInNamespace(namespace: string, url: SafeResourceUrl, options?: IconOptions): this {
return this._addSvgIconSetConfig(namespace, new SvgIconConfig(url, options));
return this._addSvgIconSetConfig(namespace, new SvgIconConfig(url, null, options));
}

/**
Expand All @@ -221,14 +216,13 @@ export class MatIconRegistry implements OnDestroy {
*/
addSvgIconSetLiteralInNamespace(namespace: string, literal: SafeHtml,
options?: IconOptions): this {
const sanitizedLiteral = this._sanitizer.sanitize(SecurityContext.HTML, literal);
const cleanLiteral = this._sanitizer.sanitize(SecurityContext.HTML, literal);

if (!sanitizedLiteral) {
if (!cleanLiteral) {
throw getMatIconFailedToSanitizeLiteralError(literal);
}

const svgElement = this._svgElementFromString(sanitizedLiteral);
return this._addSvgIconSetConfig(namespace, new SvgIconConfig(svgElement, options));
return this._addSvgIconSetConfig(namespace, new SvgIconConfig('', cleanLiteral, options));
}

/**
Expand Down Expand Up @@ -292,7 +286,7 @@ export class MatIconRegistry implements OnDestroy {
return observableOf(cloneSvg(cachedIcon));
}

return this._loadSvgIconFromConfig(new SvgIconConfig(safeUrl)).pipe(
return this._loadSvgIconFromConfig(new SvgIconConfig(safeUrl, null)).pipe(
tap(svg => this._cachedIconsByUrl.set(url!, svg)),
map(svg => cloneSvg(svg)),
);
Expand Down Expand Up @@ -335,15 +329,12 @@ export class MatIconRegistry implements OnDestroy {
* Returns the cached icon for a SvgIconConfig if available, or fetches it from its URL if not.
*/
private _getSvgFromConfig(config: SvgIconConfig): Observable<SVGElement> {
if (config.svgElement) {
if (config.svgText) {
// We already have the SVG element for this icon, return a copy.
return observableOf(cloneSvg(config.svgElement));
return observableOf(cloneSvg(this._svgElementFromConfig(config as LoadedSvgIconConfig)));
} else {
// Fetch the icon from the config's URL, cache it, and return a copy.
return this._loadSvgIconFromConfig(config).pipe(
tap(svg => config.svgElement = svg),
map(svg => cloneSvg(svg)),
);
return this._loadSvgIconFromConfig(config).pipe(map(svg => cloneSvg(svg)));
}
}

Expand All @@ -370,11 +361,11 @@ export class MatIconRegistry implements OnDestroy {

// Not found in any cached icon sets. If there are icon sets with URLs that we haven't
// fetched, fetch them now and look for iconName in the results.
const iconSetFetchRequests: Observable<SVGElement | null>[] = iconSetConfigs
.filter(iconSetConfig => !iconSetConfig.svgElement)
const iconSetFetchRequests: Observable<string | null>[] = iconSetConfigs
.filter(iconSetConfig => !iconSetConfig.svgText)
.map(iconSetConfig => {
return this._loadSvgIconSetFromConfig(iconSetConfig).pipe(
catchError((err: HttpErrorResponse): Observable<SVGElement | null> => {
catchError((err: HttpErrorResponse) => {
const url = this._sanitizer.sanitize(SecurityContext.RESOURCE_URL, iconSetConfig.url);

// Swallow errors fetching individual URLs so the
Expand Down Expand Up @@ -414,8 +405,14 @@ export class MatIconRegistry implements OnDestroy {
// Iterate backwards, so icon sets added later have precedence.
for (let i = iconSetConfigs.length - 1; i >= 0; i--) {
const config = iconSetConfigs[i];
if (config.svgElement) {
const foundIcon = this._extractSvgIconFromSet(config.svgElement, iconName, config.options);

// Parsing the icon set's text into an SVG element can be expensive. We can avoid some of
// the parsing by doing a quick check using `indexOf` to see if there's any chance for the
// icon to be in the set. This won't be 100% accurate, but it should help us avoid at least
// some of the parsing.
if (config.svgText && config.svgText.indexOf(iconName) > -1) {
const svg = this._svgElementFromConfig(config as LoadedSvgIconConfig);
const foundIcon = this._extractSvgIconFromSet(svg, iconName, config.options);
if (foundIcon) {
return foundIcon;
}
Expand All @@ -429,38 +426,22 @@ export class MatIconRegistry implements OnDestroy {
* from it.
*/
private _loadSvgIconFromConfig(config: SvgIconConfig): Observable<SVGElement> {
return this._fetchIcon(config)
.pipe(map(svgText => this._createSvgElementForSingleIcon(svgText, config.options)));
return this._fetchIcon(config).pipe(
tap(svgText => config.svgText = svgText),
map(() => this._svgElementFromConfig(config as LoadedSvgIconConfig))
);
}

/**
* Loads the content of the icon set URL specified in the SvgIconConfig and creates an SVG element
* from it.
* Loads the content of the icon set URL specified in the
* SvgIconConfig and attaches it to the config.
*/
private _loadSvgIconSetFromConfig(config: SvgIconConfig): Observable<SVGElement> {
// If the SVG for this icon set has already been parsed, do nothing.
if (config.svgElement) {
return observableOf(config.svgElement);
private _loadSvgIconSetFromConfig(config: SvgIconConfig): Observable<string | null> {
if (config.svgText) {
return observableOf(null);
}

return this._fetchIcon(config).pipe(map(svgText => {
// It is possible that the icon set was parsed and cached by an earlier request, so parsing
// only needs to occur if the cache is yet unset.
if (!config.svgElement) {
config.svgElement = this._svgElementFromString(svgText);
}

return config.svgElement;
}));
}

/**
* Creates a DOM element from the given SVG string, and adds default attributes.
*/
private _createSvgElementForSingleIcon(responseText: string, options?: IconOptions): SVGElement {
const svg = this._svgElementFromString(responseText);
this._setSvgAttributes(svg, options);
return svg;
return this._fetchIcon(config).pipe(tap(svgText => config.svgText = svgText));
}

/**
Expand Down Expand Up @@ -596,8 +577,6 @@ export class MatIconRegistry implements OnDestroy {
return inProgressFetch;
}

// TODO(jelbourn): for some reason, the `finalize` operator "loses" the generic type on the
// Observable. Figure out why and fix it.
const req = this._httpClient.get(url, {responseType: 'text', withCredentials}).pipe(
finalize(() => this._inProgressUrlFetches.delete(url)),
share(),
Expand Down Expand Up @@ -634,6 +613,17 @@ export class MatIconRegistry implements OnDestroy {

return this;
}

/** Parses a config's text into an SVG element. */
private _svgElementFromConfig(config: LoadedSvgIconConfig): SVGElement {
if (!config.svgElement) {
const svg = this._svgElementFromString(config.svgText);
this._setSvgAttributes(svg, config.options);
config.svgElement = svg;
}

return config.svgElement;
}
}

/** @docs-private */
Expand Down

0 comments on commit f4bfa84

Please sign in to comment.