Skip to content

Commit

Permalink
Perf improvements, cache/reuse to reduce i/o (palantir#2126)
Browse files Browse the repository at this point in the history
  • Loading branch information
@nchen63
nchen63 authored Jan 31, 2017
1 parent abaa138 commit 3554fb2
Show file tree
Hide file tree
Showing 6 changed files with 89 additions and 42 deletions.
1 change: 1 addition & 0 deletions src/configuration.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ function getHomeDir() {
}
}

// returns the absolute path (contrary to what the name implies)
export function getRelativePath(directory?: string | null, relativeTo?: string) {
if (directory != null) {
const basePath = relativeTo || process.cwd();
Expand Down
8 changes: 4 additions & 4 deletions src/linter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,10 @@ class Linter {
}

public lint(fileName: string, source: string, configuration: IConfigurationFile = DEFAULT_CONFIG): void {
const enabledRules = this.getEnabledRules(fileName, source, configuration);
let sourceFile = this.getSourceFile(fileName, source);
const isJs = /\.jsx?$/i.test(fileName);

const enabledRules = this.getEnabledRules(sourceFile, configuration, isJs);
let hasLinterRun = false;
let fileFailures: RuleFailure[] = [];

Expand Down Expand Up @@ -174,9 +176,7 @@ class Linter {
return fileFailures;
}

private getEnabledRules(fileName: string, source: string, configuration: IConfigurationFile = DEFAULT_CONFIG): IRule[] {
const sourceFile = this.getSourceFile(fileName, source);
const isJs = /\.jsx?$/i.test(fileName);
private getEnabledRules(sourceFile: ts.SourceFile, configuration: IConfigurationFile = DEFAULT_CONFIG, isJs: boolean): IRule[] {
const configurationRules = isJs ? configuration.jsRules : configuration.rules;

// walk the code first to find all the intervals where rules are disabled
Expand Down
83 changes: 54 additions & 29 deletions src/ruleLoader.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@
import * as fs from "fs";
import * as path from "path";

import {getRulesDirectories} from "./configuration";
import {IDisabledInterval, IRule} from "./language/rule/rule";
import {camelize, dedent} from "./utils";
import { getRelativePath } from "./configuration";
import { AbstractRule } from "./language/rule/abstractRule";
import { IDisabledInterval, IRule } from "./language/rule/rule";
import { arrayify, camelize, dedent } from "./utils";

const moduleDirectory = path.dirname(module.filename);
const CORE_RULES_DIRECTORY = path.resolve(moduleDirectory, ".", "rules");
const shownDeprecations: string[] = [];
const cachedRules = new Map<string, typeof AbstractRule | null>(); // null indicates that the rule was not found

export interface IEnableDisablePosition {
isEnabled: boolean;
Expand All @@ -42,20 +44,22 @@ export function loadRules(ruleConfiguration: {[name: string]: any},
for (const ruleName in ruleConfiguration) {
if (ruleConfiguration.hasOwnProperty(ruleName)) {
const ruleValue = ruleConfiguration[ruleName];
const Rule = findRule(ruleName, rulesDirectories);
if (Rule == null) {
notFoundRules.push(ruleName);
} else {
if (isJs && Rule.metadata && Rule.metadata.typescriptOnly != null && Rule.metadata.typescriptOnly) {
notAllowedInJsRules.push(ruleName);
if (AbstractRule.isRuleEnabled(ruleValue) || enableDisableRuleMap.hasOwnProperty(ruleName)) {
const Rule: (typeof AbstractRule) | null = findRule(ruleName, rulesDirectories);
if (Rule == null) {
notFoundRules.push(ruleName);
} else {
const ruleSpecificList = (ruleName in enableDisableRuleMap ? enableDisableRuleMap[ruleName] : []);
const disabledIntervals = buildDisabledIntervalsFromSwitches(ruleSpecificList);
rules.push(new Rule(ruleName, ruleValue, disabledIntervals));

if (Rule.metadata && Rule.metadata.deprecationMessage && shownDeprecations.indexOf(Rule.metadata.ruleName) === -1) {
console.warn(`${Rule.metadata.ruleName} is deprecated. ${Rule.metadata.deprecationMessage}`);
shownDeprecations.push(Rule.metadata.ruleName);
if (isJs && Rule.metadata && Rule.metadata.typescriptOnly != null && Rule.metadata.typescriptOnly) {
notAllowedInJsRules.push(ruleName);
} else {
const ruleSpecificList = (ruleName in enableDisableRuleMap ? enableDisableRuleMap[ruleName] : []);
const disabledIntervals = buildDisabledIntervalsFromSwitches(ruleSpecificList);
rules.push(new (Rule as any)(ruleName, ruleValue, disabledIntervals));

if (Rule.metadata && Rule.metadata.deprecationMessage && shownDeprecations.indexOf(Rule.metadata.ruleName) === -1) {
console.warn(`${Rule.metadata.ruleName} is deprecated. ${Rule.metadata.deprecationMessage}`);
shownDeprecations.push(Rule.metadata.ruleName);
}
}
}
}
Expand Down Expand Up @@ -89,26 +93,22 @@ export function loadRules(ruleConfiguration: {[name: string]: any},

export function findRule(name: string, rulesDirectories?: string | string[]) {
const camelizedName = transformName(name);
let Rule: typeof AbstractRule | null;

// first check for core rules
let Rule = loadRule(CORE_RULES_DIRECTORY, camelizedName);
if (Rule != null) {
return Rule;
}
Rule = loadCachedRule(CORE_RULES_DIRECTORY, camelizedName);

const directories = getRulesDirectories(rulesDirectories);

for (const rulesDirectory of directories) {
if (Rule == null) {
// then check for rules within the first level of rulesDirectory
if (rulesDirectory != null) {
Rule = loadRule(rulesDirectory, camelizedName);
for (const dir of arrayify(rulesDirectories)) {
Rule = loadCachedRule(dir, camelizedName, true);
if (Rule != null) {
return Rule;
break;
}
}
}

return undefined;
return Rule;
}

function transformName(name: string) {
Expand All @@ -127,17 +127,42 @@ function transformName(name: string) {
*/
function loadRule(directory: string, ruleName: string) {
const fullPath = path.join(directory, ruleName);

if (fs.existsSync(fullPath + ".js")) {
const ruleModule = require(fullPath);
if (ruleModule && ruleModule.Rule) {
return ruleModule.Rule;
}
}

return undefined;
}

function loadCachedRule(directory: string, ruleName: string, isCustomPath = false) {
// use cached value if available
const fullPath = path.join(directory, ruleName);
const cachedRule = cachedRules.get(fullPath);
if (cachedRule !== undefined) {
return cachedRule;
}

// get absolute path
let absolutePath: string | undefined = directory;
if (isCustomPath) {
absolutePath = getRelativePath(directory);
if (absolutePath != null) {
if (!fs.existsSync(absolutePath)) {
throw new Error(`Could not find custom rule directory: ${directory}`);
}
}
}

let Rule: typeof AbstractRule | null = null;
if (absolutePath != null) {
Rule = loadRule(absolutePath, ruleName);
}
cachedRules.set(fullPath, Rule);
return Rule;
}

/**
* creates disabled intervals for rule based on list of switchers for it
* @param ruleSpecificList - contains all switchers for rule states sorted top-down and strictly alternating between enabled and disabled
Expand Down
11 changes: 9 additions & 2 deletions src/runner.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
CONFIG_FILENAME,
DEFAULT_CONFIG,
findConfiguration,
IConfigurationFile,
} from "./configuration";
import { FatalError } from "./error";
import * as Linter from "./linter";
Expand Down Expand Up @@ -213,6 +214,8 @@ export class Runner {
rulesDirectory: this.options.rulesDirectory || "",
}, program);

let lastFolder: string | undefined;
let configFile: IConfigurationFile | undefined;
for (const file of files) {
if (!fs.existsSync(file)) {
console.error(`Unable to open file: ${file}`);
Expand All @@ -237,8 +240,12 @@ export class Runner {
}

const contents = fs.readFileSync(file, "utf8");
const configLoad = findConfiguration(possibleConfigAbsolutePath, file);
linter.lint(file, contents, configLoad.results);
const folder = path.dirname(file);
if (lastFolder !== folder) {
configFile = findConfiguration(possibleConfigAbsolutePath, folder).results;
lastFolder = folder;
}
linter.lint(file, contents, configFile);
}

const lintResult = linter.getResult();
Expand Down
7 changes: 4 additions & 3 deletions test/eofLineRuleTests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,13 @@
* limitations under the License.
*/

import {TestUtils} from "./lint";
import { Rule } from "../src/rules/eoflineRule";
import { TestUtils } from "./lint";

describe("<eofline>", () => {
const EofLineRule = TestUtils.getRule("eofline");
const EofLineRule = TestUtils.getRule("eofline") as Rule | null;
const fileName = "eofline.test.ts";
const failureString = EofLineRule.FAILURE_STRING;
const failureString = Rule.FAILURE_STRING;

it("ensures a trailing newline at EOF", () => {
const actualFailures = TestUtils.applyRuleOnFile(fileName, EofLineRule);
Expand Down
21 changes: 17 additions & 4 deletions test/ruleLoaderTests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ describe("Rule Loader", () => {
"eofline": true,
"forin": false,
"no-debugger": true,
"quotemark": "single",
"quotemark": [true, "single"],
};

const rules = loadRules(validConfiguration, {}, builtRulesDir);
assert.equal(rules.length, 5);
assert.equal(rules.length, 4);
});

it("ignores invalid rules", () => {
Expand All @@ -50,17 +50,30 @@ describe("Rule Loader", () => {
assert.equal(rules.length, 1);
});

it("loads disabled rules if rule in enableDisableRuleMap", () => {
const validConfiguration: {[name: string]: any} = {
"class-name": true,
"eofline": true,
"forin": false,
"no-debugger": true,
"quotemark": [true, "single"],
};

const rules = loadRules(validConfiguration, {forin: [{isEnabled: true, position: 4}]}, builtRulesDir);
assert.equal(rules.length, 5);
});

it("works with rulesDirectory argument as an Array", () => {
const validConfiguration: {[name: string]: any} = {
"class-name": true,
"eofline": true,
"forin": false,
"no-debugger": true,
"quotemark": "single",
"quotemark": [true, "single"],
};

const rules = loadRules(validConfiguration, {}, [builtRulesDir]);
assert.equal(rules.length, 5);
assert.equal(rules.length, 4);
});

it("loads js rules", () => {
Expand Down

0 comments on commit 3554fb2

Please sign in to comment.