Skip to content

Commit

Permalink
fix(core): not invoking object's toString when rendering to the DOM (#…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
crisbeto authored and thePunderWoman committed Nov 30, 2020
1 parent 1de59d2 commit 11cd37f
Show file tree
Hide file tree
Showing 4 changed files with 163 additions and 1 deletion.
5 changes: 4 additions & 1 deletion packages/core/src/render3/util/stringify_utils.ts
Expand Up @@ -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);
}


Expand Down
42 changes: 42 additions & 0 deletions packages/core/test/acceptance/text_spec.ts
Expand Up @@ -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)');
});
});
13 changes: 13 additions & 0 deletions packages/core/test/render3/perf/BUILD.bazel
Expand Up @@ -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",
)
104 changes: 104 additions & 0 deletions 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();

0 comments on commit 11cd37f

Please sign in to comment.