From 11cd37fae304720d32b3f010f61ea7065d34b40b Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Sat, 28 Nov 2020 12:01:57 +0100 Subject: [PATCH] fix(core): not invoking object's toString when rendering to the DOM (#39843) Currently we convert objects to strings using `'' + value` which is quickest, but it stringifies the value using its `valueOf`, rather than `toString`. These changes switch to using `String(value)` which has identical performance and calls the `toString` method as expected. Note that another option was calling `toString` directly, but benchmarking showed it to be slower. I've included the benchmark I used to verify the performance so we have it for future reference and we can reuse it when making changes to `renderStringify` in the future. Also for reference, here are the results of the benchmark: ``` Benchmark: renderStringify concat: 2.006 ns(0%) concat with toString: 2.201 ns(-10%) toString: 237.494 ns(-11741%) toString with toString: 121.072 ns(-5937%) constructor: 2.201 ns(-10%) constructor with toString: 2.201 ns(-10%) toString mono: 14.536 ns(-625%) toString with toString mono: 9.757 ns(-386%) ``` Fixes #38839. PR Close #39843 --- .../core/src/render3/util/stringify_utils.ts | 5 +- packages/core/test/acceptance/text_spec.ts | 42 +++++++ packages/core/test/render3/perf/BUILD.bazel | 13 +++ .../render3/perf/render_stringify/index.ts | 104 ++++++++++++++++++ 4 files changed, 163 insertions(+), 1 deletion(-) create mode 100644 packages/core/test/render3/perf/render_stringify/index.ts diff --git a/packages/core/src/render3/util/stringify_utils.ts b/packages/core/src/render3/util/stringify_utils.ts index c905b122d4..872457fe9f 100644 --- a/packages/core/src/render3/util/stringify_utils.ts +++ b/packages/core/src/render3/util/stringify_utils.ts @@ -10,11 +10,14 @@ * Used for stringify render output in Ivy. * Important! This function is very performance-sensitive and we should * be extra careful not to introduce megamorphic reads in it. + * Check `core/test/render3/perf/render_stringify` for benchmarks and alternate implementations. */ export function renderStringify(value: any): string { if (typeof value === 'string') return value; if (value == null) return ''; - return '' + value; + // Use `String` so that it invokes the `toString` method of the value. Note that this + // appears to be faster than calling `value.toString` (see `render_stringify` benchmark). + return String(value); } diff --git a/packages/core/test/acceptance/text_spec.ts b/packages/core/test/acceptance/text_spec.ts index 7584c1e5e1..4d25f93333 100644 --- a/packages/core/test/acceptance/text_spec.ts +++ b/packages/core/test/acceptance/text_spec.ts @@ -129,4 +129,46 @@ describe('text instructions', () => { expect(div.innerHTML).toBe('function foo() { }'); }); + + it('should stringify an object using its toString method', () => { + class TestObject { + toString() { + return 'toString'; + } + valueOf() { + return 'valueOf'; + } + } + + @Component({template: '{{object}}'}) + class App { + object = new TestObject(); + } + + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + expect(fixture.nativeElement.textContent).toBe('toString'); + }); + + it('should stringify a symbol', () => { + // This test is only valid on browsers that support Symbol. + if (typeof Symbol === 'undefined') { + return; + } + + @Component({template: '{{symbol}}'}) + class App { + symbol = Symbol('hello'); + } + + TestBed.configureTestingModule({declarations: [App]}); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + + // Note that this uses `toContain`, because a polyfilled `Symbol` produces something like + // `Symbol(hello)_p.sc8s398cplk`, whereas the native one is `Symbol(hello)`. + expect(fixture.nativeElement.textContent).toContain('Symbol(hello)'); + }); }); diff --git a/packages/core/test/render3/perf/BUILD.bazel b/packages/core/test/render3/perf/BUILD.bazel index 22cd841d2d..144b8f5eb7 100644 --- a/packages/core/test/render3/perf/BUILD.bazel +++ b/packages/core/test/render3/perf/BUILD.bazel @@ -255,3 +255,16 @@ ng_benchmark( name = "view_destroy_hook", bundle = ":view_destroy_hook_lib", ) + +ng_rollup_bundle( + name = "render_stringify_lib", + entry_point = ":render_stringify/index.ts", + deps = [ + ":perf_lib", + ], +) + +ng_benchmark( + name = "render_stringify", + bundle = ":render_stringify_lib", +) diff --git a/packages/core/test/render3/perf/render_stringify/index.ts b/packages/core/test/render3/perf/render_stringify/index.ts new file mode 100644 index 0000000000..c8547df799 --- /dev/null +++ b/packages/core/test/render3/perf/render_stringify/index.ts @@ -0,0 +1,104 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import {createBenchmark} from '../micro_bench'; + +// These benchmarks compare various implementations of the `renderStringify` utility +// which vary in subtle ways which end up having an effect on performance. + +/** Uses string concatenation to convert a value into a string. */ +function renderStringifyConcat(value: any): string { + if (typeof value === 'string') return value; + if (value == null) return ''; + return '' + value; +} + +/** Uses `toString` to convert a value into a string. */ +function renderStringifyToString(value: any): string { + if (typeof value === 'string') return value; + if (value == null) return ''; + return value.toString(); +} + +/** Uses the `String` constructor to convert a value into a string. */ +function renderStringifyConstructor(value: any): string { + if (typeof value === 'string') return value; + if (value == null) return ''; + return String(value); +} + +const objects: any[] = []; +const objectsWithToString: any[] = []; + +// Allocate a bunch of objects with a unique structure. +for (let i = 0; i < 1000000; i++) { + objects.push({['foo_' + i]: i}); + objectsWithToString.push({['foo_' + i]: i, toString: () => 'x'}); +} +const max = objects.length - 1; +let i = 0; + +const benchmarkRefresh = createBenchmark('renderStringify'); +const renderStringifyConcatTime = benchmarkRefresh('concat'); +const renderStringifyConcatWithToStringTime = benchmarkRefresh('concat with toString'); +const renderStringifyToStringTime = benchmarkRefresh('toString'); +const renderStringifyToStringWithToStringTime = benchmarkRefresh('toString with toString'); +const renderStringifyConstructorTime = benchmarkRefresh('constructor'); +const renderStringifyConstructorWithToStringTime = benchmarkRefresh('constructor with toString'); +const renderStringifyToStringMonoTime = benchmarkRefresh('toString mono'); +const renderStringifyToStringWithToStringMonoTime = benchmarkRefresh('toString with toString mono'); + +// Important! This code is somewhat repetitive, but we can't move it out into something like +// `benchmark(name, stringifyFn)`, because passing in the function as a parameter breaks inlining. + +// String concatenation +while (renderStringifyConcatTime()) { + renderStringifyConcat(objects[i]); + i = i < max ? i + 1 : 0; +} + +while (renderStringifyConcatWithToStringTime()) { + renderStringifyConcat(objectsWithToString[i]); + i = i < max ? i + 1 : 0; +} +///////////// + +// String() +while (renderStringifyConstructorTime()) { + renderStringifyConstructor(objects[i]); + i = i < max ? i + 1 : 0; +} + +while (renderStringifyConstructorWithToStringTime()) { + renderStringifyConstructor(objectsWithToString[i]); + i = i < max ? i + 1 : 0; +} +///////////// + +// toString +while (renderStringifyToStringTime()) { + renderStringifyToString(objects[i]); + i = i < max ? i + 1 : 0; +} + +while (renderStringifyToStringWithToStringTime()) { + renderStringifyToString(objectsWithToString[i]); + i = i < max ? i + 1 : 0; +} +///////////// + +// toString mono +while (renderStringifyToStringMonoTime()) { + renderStringifyToString(objects[0]); +} + +while (renderStringifyToStringWithToStringMonoTime()) { + renderStringifyToString(objectsWithToString[0]); +} +///////////// + +benchmarkRefresh.report();