Make modifierFlagsCache mandatory for Node objects

[bmeurer]

The modifierFlagsCache property is among the top properties that cause
megamorphic stub cache misses when having tsc build itself. This is
because it's added sort of randomly to a whole lot of different objects,
at arbitrary points after their initialization.

This also means that modifierFlagsCache is often not present on the
instances, which means that when missing on the megamorphic stub cache,
Node/V8 has to compute NonExistent handlers for the relevant inline
caches.

It's a lot cheaper to just make sure that the property is always
existent on the instance, and ideally also at the same offset. This
change ensures exactly that.

Drive-by-fix: Also make sure that transformFlags (and parent)
is always at the same offset for Node objects.

[msftclas]

All CLA requirements met.

[RyanCavanaugh]

@rbuckton can you grab some perf numbers for this?

[rbuckton]

In general this makes sense, we already do this in utilities.ts for compiler for the same reason. I can run a benchmark comparison, though it seems there are test failures currently.

[bmeurer]

Unfortunately this is not easy to assess, due to the nature of the megamorphic stub cache in V8. I'm currently investigating class performance in various applications, especially ones generated with TypeScript (I wrote an explainer regarding the megamorphic stub cache as part of that).

This change reduces the number of megamorphic stub cache misses for modifierFlagsCache by more than 75% on average. And also get's down stub cache misses for other keys. But it's only part of the problem, since there are still too many other stub cache misses (some of them are from real megamorphic sites, but a couple others are only due to subclassing).

The modifierFlagsCache stood out, since it seemed easy to add and in line with what you already do for other fields. Do that make sense?

[rbuckton]

@bmeurer: Since these modifications are mostly in services, I take it your comparisons were against the language server and not the command-line compiler?

@RyanCavanaugh: Our benchmarks don't differ here because the benchmarks only test command-line compiler performance. However, this is a good change as it helps to align the language server object allocators for our ASTs to align with the behavior of the object allocators for tsc.

[bmeurer]

@rbuckton I'm using the typescript runner from the web-tooling-benchmark, which uses ts.transpileModule() entry point.

[rbuckton]

That unfortunately loads the whole language service, which has its own AST factory (the objectAllocator). The objectAllocator we use for TSC creates far smaller objects and we spent some time over a year ago to reduce polymorphism and hidden shape transitions. For example, https://github.com/Microsoft/TypeScript/blob/master/src/compiler/utilities.ts#L6972 already has this change, but it had not yet been propagated to the Node class in services.

I'm not certain why your change has caused test failures. Most seem to be related to JSDoc parsing. @sandersn: Any idea what's going on here?

[bmeurer]

Any idea regarding the JSDoc test failures? I'm a bit puzzled here TBH.

[RyanCavanaugh]

@bmeurer the test failures are from tests that baseline the parser output (example below). You should run gulp baseline-accept and commit the resulting changes

 {
     "kind": "TupleType",
     "pos": 1,
     "end": 9,
     "flags": "JSDoc",
+    "modifierFlagsCache": 0,
     "elementTypes": {
         "0": {
             "kind": "NumberKeyword",
             "pos": 2,
             "end": 8,
-            "flags": "JSDoc"
+            "flags": "JSDoc",
+            "modifierFlagsCache": 0,
+            "transformFlags": 0
         },
         "length": 1,
         "pos": 2,
         "end": 8
     }

[bmeurer]

@RyanCavanaugh Ah, thanks. Did that 👍

(BTW gulp complained that the tasks didn't call async completion?)

[rbuckton]

This is a gulp4 thing. Apparently it doesn't like streams merged with merge2 anymore.

[rbuckton]

It seems like there are still failing baslines.

[bmeurer]

@rbuckton Ups. Should be fixed now.

[j-oliveras]

I think you should run jake lint or gulp lint and fix the reported warnings it. From azure node-10 build:

2019-02-07T19:13:39.7315894Z ERROR: /home/vsts/work/1/s/src/compiler/utilities.ts:3795:13 no-unnecessary-type-assertion This assertion is unnecessary since it does not change the type of the expression.
2019-02-07T19:13:39.7316622Z ERROR: /home/vsts/work/1/s/src/compiler/utilities.ts:3796:20 no-unnecessary-type-assertion This assertion is unnecessary since it does not change the type of the expression.

These are consequences to remove optionality from modifierFlagsCache.

[bmeurer]

@j-oliveras done, thanks

[rbuckton]

Thanks for the contribution!

[bmeurer]

Awesome, thanks a lot! 👍

So do you think the typescript testing in the web-tooling-benchmark is actually a useful, representative test? Or am I looking at the wrong problem?

[rbuckton]

That can depend on whether your benchmark is intended to measure the execution time of a specific downlevel representation of native JavaScript, or to measure the execution of the compiler itself. The tsc command-line compiler is much smaller and has significantly less overhead than using the language service (either directly via require("typescript") or indirectly via tsserver), however the language service is often used in conjunction with build tools such as grunt or gulp.

Internally, the TypeScript team runs regular benchmarks (multiple per day) against the tsc compiler in our master branch to keep track of performance regressions in core functionality (parse, bind, check, and emit), against several medium to large repositories that stress different parts of our compiler. In these cases, however, we are measuring the execution time of our own compiler, which may not necessarily cover all cases of our transpiled emit.

We have, on occasion, chosen to focus on downlevel emit that has better performance characteristics at runtime at the expense of non-spec-compliant runtime semantics. For example, our for..of emit when targeting ES5/3 is converted to a regular for loop against the length of the array as it is significantly faster than the more spec-compliant behavior used when the --downlevelIteration compiler option is set.

[bmeurer]

The benchmark was supposed to give an idea of the general performance of TypeScript compiler on any given JavaScript engine. So ideally improvements on the benchmark should show up as improvements in the wild for users. There are two main scenarios I see here:

1. TypeScript as part of the compilation pipeline when building a large application.
2. TypeScript integrated with Visual Studio Code doing interactive type checking as you type.

Ideally both of these should be fast.

cc @mathiasbynens