Skip to content

Commit 11cd37f

Browse files
crisbetothePunderWoman
authored andcommitted
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
1 parent 1de59d2 commit 11cd37f

File tree

4 files changed

+163
-1
lines changed

4 files changed

+163
-1
lines changed

packages/core/src/render3/util/stringify_utils.ts

+4-1
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,14 @@
1010
* Used for stringify render output in Ivy.
1111
* Important! This function is very performance-sensitive and we should
1212
* be extra careful not to introduce megamorphic reads in it.
13+
* Check `core/test/render3/perf/render_stringify` for benchmarks and alternate implementations.
1314
*/
1415
export function renderStringify(value: any): string {
1516
if (typeof value === 'string') return value;
1617
if (value == null) return '';
17-
return '' + value;
18+
// Use `String` so that it invokes the `toString` method of the value. Note that this
19+
// appears to be faster than calling `value.toString` (see `render_stringify` benchmark).
20+
return String(value);
1821
}
1922

2023

packages/core/test/acceptance/text_spec.ts

+42
Original file line numberDiff line numberDiff line change
@@ -129,4 +129,46 @@ describe('text instructions', () => {
129129

130130
expect(div.innerHTML).toBe('function foo() { }');
131131
});
132+
133+
it('should stringify an object using its toString method', () => {
134+
class TestObject {
135+
toString() {
136+
return 'toString';
137+
}
138+
valueOf() {
139+
return 'valueOf';
140+
}
141+
}
142+
143+
@Component({template: '{{object}}'})
144+
class App {
145+
object = new TestObject();
146+
}
147+
148+
TestBed.configureTestingModule({declarations: [App]});
149+
const fixture = TestBed.createComponent(App);
150+
fixture.detectChanges();
151+
152+
expect(fixture.nativeElement.textContent).toBe('toString');
153+
});
154+
155+
it('should stringify a symbol', () => {
156+
// This test is only valid on browsers that support Symbol.
157+
if (typeof Symbol === 'undefined') {
158+
return;
159+
}
160+
161+
@Component({template: '{{symbol}}'})
162+
class App {
163+
symbol = Symbol('hello');
164+
}
165+
166+
TestBed.configureTestingModule({declarations: [App]});
167+
const fixture = TestBed.createComponent(App);
168+
fixture.detectChanges();
169+
170+
// Note that this uses `toContain`, because a polyfilled `Symbol` produces something like
171+
// `Symbol(hello)_p.sc8s398cplk`, whereas the native one is `Symbol(hello)`.
172+
expect(fixture.nativeElement.textContent).toContain('Symbol(hello)');
173+
});
132174
});

packages/core/test/render3/perf/BUILD.bazel

+13
Original file line numberDiff line numberDiff line change
@@ -255,3 +255,16 @@ ng_benchmark(
255255
name = "view_destroy_hook",
256256
bundle = ":view_destroy_hook_lib",
257257
)
258+
259+
ng_rollup_bundle(
260+
name = "render_stringify_lib",
261+
entry_point = ":render_stringify/index.ts",
262+
deps = [
263+
":perf_lib",
264+
],
265+
)
266+
267+
ng_benchmark(
268+
name = "render_stringify",
269+
bundle = ":render_stringify_lib",
270+
)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
/**
2+
* @license
3+
* Copyright Google LLC All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
import {createBenchmark} from '../micro_bench';
9+
10+
// These benchmarks compare various implementations of the `renderStringify` utility
11+
// which vary in subtle ways which end up having an effect on performance.
12+
13+
/** Uses string concatenation to convert a value into a string. */
14+
function renderStringifyConcat(value: any): string {
15+
if (typeof value === 'string') return value;
16+
if (value == null) return '';
17+
return '' + value;
18+
}
19+
20+
/** Uses `toString` to convert a value into a string. */
21+
function renderStringifyToString(value: any): string {
22+
if (typeof value === 'string') return value;
23+
if (value == null) return '';
24+
return value.toString();
25+
}
26+
27+
/** Uses the `String` constructor to convert a value into a string. */
28+
function renderStringifyConstructor(value: any): string {
29+
if (typeof value === 'string') return value;
30+
if (value == null) return '';
31+
return String(value);
32+
}
33+
34+
const objects: any[] = [];
35+
const objectsWithToString: any[] = [];
36+
37+
// Allocate a bunch of objects with a unique structure.
38+
for (let i = 0; i < 1000000; i++) {
39+
objects.push({['foo_' + i]: i});
40+
objectsWithToString.push({['foo_' + i]: i, toString: () => 'x'});
41+
}
42+
const max = objects.length - 1;
43+
let i = 0;
44+
45+
const benchmarkRefresh = createBenchmark('renderStringify');
46+
const renderStringifyConcatTime = benchmarkRefresh('concat');
47+
const renderStringifyConcatWithToStringTime = benchmarkRefresh('concat with toString');
48+
const renderStringifyToStringTime = benchmarkRefresh('toString');
49+
const renderStringifyToStringWithToStringTime = benchmarkRefresh('toString with toString');
50+
const renderStringifyConstructorTime = benchmarkRefresh('constructor');
51+
const renderStringifyConstructorWithToStringTime = benchmarkRefresh('constructor with toString');
52+
const renderStringifyToStringMonoTime = benchmarkRefresh('toString mono');
53+
const renderStringifyToStringWithToStringMonoTime = benchmarkRefresh('toString with toString mono');
54+
55+
// Important! This code is somewhat repetitive, but we can't move it out into something like
56+
// `benchmark(name, stringifyFn)`, because passing in the function as a parameter breaks inlining.
57+
58+
// String concatenation
59+
while (renderStringifyConcatTime()) {
60+
renderStringifyConcat(objects[i]);
61+
i = i < max ? i + 1 : 0;
62+
}
63+
64+
while (renderStringifyConcatWithToStringTime()) {
65+
renderStringifyConcat(objectsWithToString[i]);
66+
i = i < max ? i + 1 : 0;
67+
}
68+
/////////////
69+
70+
// String()
71+
while (renderStringifyConstructorTime()) {
72+
renderStringifyConstructor(objects[i]);
73+
i = i < max ? i + 1 : 0;
74+
}
75+
76+
while (renderStringifyConstructorWithToStringTime()) {
77+
renderStringifyConstructor(objectsWithToString[i]);
78+
i = i < max ? i + 1 : 0;
79+
}
80+
/////////////
81+
82+
// toString
83+
while (renderStringifyToStringTime()) {
84+
renderStringifyToString(objects[i]);
85+
i = i < max ? i + 1 : 0;
86+
}
87+
88+
while (renderStringifyToStringWithToStringTime()) {
89+
renderStringifyToString(objectsWithToString[i]);
90+
i = i < max ? i + 1 : 0;
91+
}
92+
/////////////
93+
94+
// toString mono
95+
while (renderStringifyToStringMonoTime()) {
96+
renderStringifyToString(objects[0]);
97+
}
98+
99+
while (renderStringifyToStringWithToStringMonoTime()) {
100+
renderStringifyToString(objectsWithToString[0]);
101+
}
102+
/////////////
103+
104+
benchmarkRefresh.report();

0 commit comments

Comments
 (0)