-
Notifications
You must be signed in to change notification settings - Fork 13k
RulesProvider performance improvements (alternate implementation) #15364
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
private rulesMap: RulesMap; | ||
|
||
constructor() { | ||
this.globalRules = new Rules(); | ||
const activeRules = this.createActiveRules(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit. i would inline this function, since it is only used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/services/services.ts
Outdated
@@ -31,6 +31,8 @@ namespace ts { | |||
/** The version of the language service API */ | |||
export const servicesVersion = "0.5"; | |||
|
|||
const ruleProvider: formatting.RulesProvider = new formatting.RulesProvider(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could call this in server/editorServices.ts to initialize the provider instead of doing it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also mark it as /** @internal **/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved the initialization to createLanguageService()
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; }[]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! I like this better. done
src/services/formatting/rules.ts
Outdated
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type of optionName
should be keyof FormatCodeSettings
. this will catch issues of misspelled configurations and avoids the cast to any
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks - done
src/services/formatting/rules.ts
Outdated
// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/services/formatting/rules.ts
Outdated
/// | ||
/// Contexts | ||
/// | ||
static IsOptionDisabledOrUndefined(optionName: string, options: FormatCodeSettings): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/services/services.ts
Outdated
@@ -31,6 +31,8 @@ namespace ts { | |||
/** The version of the language service API */ | |||
export const servicesVersion = "0.5"; | |||
|
|||
const ruleProvider: formatting.RulesProvider = new formatting.RulesProvider(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could call this in server/editorServices.ts to initialize the provider instead of doing it here.
src/services/services.ts
Outdated
@@ -31,6 +31,8 @@ namespace ts { | |||
/** The version of the language service API */ | |||
export const servicesVersion = "0.5"; | |||
|
|||
const ruleProvider: formatting.RulesProvider = new formatting.RulesProvider(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also mark it as /** @internal **/
f8e44ee
to
257a4dc
Compare
Fixes:
• TS: [TSServer] Formatting RulesProvider (re)initialization performance issues
• VS: Bug 373219: [TypeScript Perf] WebForms_DDRIT.0300.Typing HTML5 HTML regressed Duration_AccumulatedElapsedTime (60ms)