RulesProvider performance improvements (alternate implementation) by jramsay · Pull Request #15364 · microsoft/TypeScript

jramsay · 2017-04-25T00:52:00Z

Add all rules to the RulesProvider’s RulesMap regardless of user options
Pass FormatCodeSettings to the FormattingContext
Update RuleOperationContext's InContext to check if a rule should be applied based on the FormattingCodeOptions
Update rules to consume new RuleOperationContext options check
compareDataObjects fix to check if the object has the same number of keys

Fixes:
• TS: [TSServer] Formatting RulesProvider (re)initialization performance issues
• VS: Bug 373219: [TypeScript Perf] WebForms_DDRIT.0300.Typing HTML5 HTML regressed Duration_AccumulatedElapsedTime (60ms)

mhegazy · 2017-04-26T23:10:31Z


        constructor() {
            this.globalRules = new Rules();
+            const activeRules = this.createActiveRules();


nit. i would inline this function, since it is only used here.

mhegazy · 2017-04-26T23:12:58Z

    /** The version of the language service API */
    export const servicesVersion = "0.5";

+    const ruleProvider: formatting.RulesProvider = new formatting.RulesProvider();


this will force users of the services, even ones who never use formatting, to allocate this large data structure. how about doing on first request instead?

We could call this in server/editorServices.ts to initialize the provider instead of doing it here.

also mark it as /** @internal **/

moved the initialization to createLanguageService()

mhegazy · 2017-04-26T23:20:53Z

+        static Create1(...funcs: { (context: FormattingContext): boolean; }[]) {
+            return new RuleOperationContext(undefined, undefined, funcs);
+        }
+        static Create2(optionName: string, checkApplyRuleOperation: { (optionName: string, options: FormatCodeSettings): boolean }, ...funcs: { (context: FormattingContext): boolean; }[]) {


consider not adding the new two options here, and leaving it as a set of functions as it was.

Under this assumption, your input for:

Create2("insertSpaceAfterOpeningAndBeforeClosingNonemptyBraces", Rules.IsOptionEnabledOrUndefined, Rules.IsBraceWrappedContext)

would be:

new RuleOperationContext(Rules.IsOptionEnabledOrUndefined("insertSpaceAfterOpeningAndBeforeClosingNonemptyBraces"), Rules.IsBraceWrappedContext)

where IsOptionEnabledOrUndefined is defined as:

static IsOptionEnabledOrUndefined(optionName: string): (option) => boolean { return (options) => !options.hasOwnProperty(optionName) || (<any>options)[optionName]; }

nice! I like this better. done

mhegazy · 2017-04-26T23:22:12Z

-            // Insert space after function keyword for anonymous functions
-            this.SpaceAfterAnonymousFunctionKeyword = new Rule(RuleDescriptor.create1(SyntaxKind.FunctionKeyword, SyntaxKind.OpenParenToken), RuleOperation.create2(new RuleOperationContext(Rules.IsFunctionDeclContext), RuleAction.Space));
-            this.NoSpaceAfterAnonymousFunctionKeyword = new Rule(RuleDescriptor.create1(SyntaxKind.FunctionKeyword, SyntaxKind.OpenParenToken), RuleOperation.create2(new RuleOperationContext(Rules.IsFunctionDeclContext), RuleAction.Delete));
+        static IsOptionEnabled(optionName: string, options: FormatCodeSettings): boolean {


Type of optionName should be keyof FormatCodeSettings. this will catch issues of misspelled configurations and avoids the cast to any.

thanks - done

mhegazy · 2017-04-26T23:22:21Z

-            // No space after type assertion
-            this.NoSpaceAfterTypeAssertion = new Rule(RuleDescriptor.create3(SyntaxKind.GreaterThanToken, Shared.TokenRange.Any), RuleOperation.create2(new RuleOperationContext(Rules.IsNonJsxSameLineTokenContext, Rules.IsTypeAssertionContext), RuleAction.Delete));
-            this.SpaceAfterTypeAssertion = new Rule(RuleDescriptor.create3(SyntaxKind.GreaterThanToken, Shared.TokenRange.Any), RuleOperation.create2(new RuleOperationContext(Rules.IsNonJsxSameLineTokenContext, Rules.IsTypeAssertionContext), RuleAction.Space));
+        static IsOptionDisabled(optionName: string, options: FormatCodeSettings): boolean {


and here too

mhegazy · 2017-04-26T23:22:26Z

-        ///
-        /// Contexts
-        ///
+        static IsOptionDisabledOrUndefined(optionName: string, options: FormatCodeSettings): boolean {


mhegazy · 2017-04-26T23:26:39Z

    /** The version of the language service API */
    export const servicesVersion = "0.5";

+    const ruleProvider: formatting.RulesProvider = new formatting.RulesProvider();


We could call this in server/editorServices.ts to initialize the provider instead of doing it here.

mhegazy · 2017-04-26T23:26:53Z

    /** The version of the language service API */
    export const servicesVersion = "0.5";

+    const ruleProvider: formatting.RulesProvider = new formatting.RulesProvider();


also mark it as /** @internal **/

RulesProvider performance improvements

1f9bdef

msftclas added the cla-already-signed label Apr 25, 2017

jramsay requested review from billti and mhegazy April 25, 2017 00:52

mhegazy suggested changes Apr 26, 2017

View reviewed changes

Changes from CR feedback

257a4dc

jramsay force-pushed the RulesProviderPerformance branch from f8e44ee to 257a4dc Compare April 27, 2017 18:41

mhegazy approved these changes Apr 27, 2017

View reviewed changes

jramsay merged commit aa11ab1 into master Apr 27, 2017

mhegazy deleted the RulesProviderPerformance branch November 2, 2017 21:04

microsoft locked and limited conversation to collaborators Jun 14, 2018

Conversation

jramsay commented Apr 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jramsay Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jramsay Apr 26, 2017 •

edited

Loading