Skip to content
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

Release 0.66.0 performance fixes #834

Merged
merged 20 commits into from
Jul 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
d48fede
Performance fixes
TwitchBronBron Jul 10, 2023
7932e22
Lift `isUsedAsType` to cached expressionInfo obj
TwitchBronBron Jul 10, 2023
139d0b2
Slightly more aggressive symbol type caching
TwitchBronBron Jul 10, 2023
6fb0f93
Merge branch 'release-0.66.0' of https://github.com/rokucommunity/bri…
TwitchBronBron Jul 11, 2023
7ad0db7
Cache speed boost
TwitchBronBron Jul 11, 2023
ac68722
Potentially solved Scope Linking slow down (#840)
markwpearce Jul 11, 2023
2da10d2
Make CacheVerifier more stable
TwitchBronBron Jul 11, 2023
c45058f
Merge branch 'release-0.66.0-performance-fixes' of https://github.com…
TwitchBronBron Jul 11, 2023
b402d78
fix cross-platform benchmark bug
TwitchBronBron Jul 11, 2023
11e59c6
Disable execSync logging
TwitchBronBron Jul 11, 2023
5b2a8b0
Add specific configuration options for benchmarks at the command line…
markwpearce Jul 11, 2023
79895b1
Added package.json back
markwpearce Jul 11, 2023
1e867aa
Small optimization during symbol table linking
TwitchBronBron Jul 12, 2023
9d9de26
Better symbol table type retrieval - uses parent and sibling caches (…
markwpearce Jul 12, 2023
62f9502
Merge branch 'release-0.66.0-performance-fixes' of https://github.com…
TwitchBronBron Jul 12, 2023
8b60410
Prevent leaking sibling tables
TwitchBronBron Jul 12, 2023
30e5a2a
Add more benchmark padding (just cuz)
TwitchBronBron Jul 12, 2023
8d1e679
Revert `getSymbolTypeRecursive`
TwitchBronBron Jul 12, 2023
0542722
Better return type for `getScopeByName`
TwitchBronBron Jul 13, 2023
6bc6b5b
Optimize buildNamespaceLookup and some string stuff
TwitchBronBron Jul 14, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ module.exports = {
'@typescript-eslint/dot-notation': 'off',
'@typescript-eslint/no-unnecessary-type-assertion': 'off',
'@typescript-eslint/no-non-null-assertion': 'off',
'no-var': 'off',
'camelcase': 'off'
}
}]
Expand Down
9 changes: 8 additions & 1 deletion benchmarks/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ interface RunnerOptions {
quick: boolean;
profile: boolean;
tar: boolean;
config: string;
}
class Runner {
constructor(
Expand Down Expand Up @@ -144,8 +145,9 @@ class Runner {
cwd: cwd
});

execSync(`npx ts-node target-runner.ts "${version}" "${maxVersionLength}" "${target}" "${maxTargetLength}" "${alias}" "${this.options.project}" "${this.options.quick}" "${this.options.profile}"`, {
execSync(`npx ts-node target-runner.ts "${version}" "${maxVersionLength}" "${target}" "${maxTargetLength}" "${alias}" "${this.options.project}" "${this.options.quick}" "${this.options.profile}" "${(this.options.config ?? '{}').replaceAll('\"', '\\"')}"`, {
env: {
...process.env,
'NODE_OPTIONS': `--max-old-space-size=${MAX_OLD_SPACE}`
}
});
Expand Down Expand Up @@ -205,6 +207,11 @@ let options = yargs
description: 'use a npm-packed tarball for local files instead of using the files directly',
default: true
})
.option('config', {
type: 'string',
description: 'add additional BsConfig settings as JSON - eg. \'{"enableTypeValidation":true}\'',
default: '{}'
})
.strict()
.check(argv => {
const idx = argv.versions.indexOf('latest');
Expand Down
2,592 changes: 2,583 additions & 9 deletions benchmarks/package-lock.json

Large diffs are not rendered by default.

8 changes: 6 additions & 2 deletions benchmarks/target-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as chalk from 'chalk';
const v8Profiler = require('v8-profiler-next');

let idx = 2;
const extraNamePadding = 20;

const version = process.argv[idx++];
const maxVersionLength = parseInt(process.argv[idx++]);
Expand All @@ -16,6 +17,7 @@ const bscAlias = process.argv[idx++];
const projectPath = process.argv[idx++];
const quick = JSON.parse(process.argv[idx++]);
const profile = JSON.parse(process.argv[idx++]);
const config = JSON.parse(process.argv[idx++]);

const profileTitle = `${target}@${version}`;

Expand Down Expand Up @@ -63,7 +65,7 @@ const addTargetTestFunction = require(path.join(__dirname, 'targets', target));
const formattedHz = formatNumber(result.hz.toFixed(3));
let [name] = result.name.split('@');
console.log(
`${name.padStart(maxTargetLength + 8, ' ')}@${version.padEnd(maxVersionLength, ' ')}`,
`${name.padStart(maxTargetLength + extraNamePadding, ' ')}@${version.padEnd(maxVersionLength, ' ')}`,
'-'.repeat(' ###,###,###.###'.length - formattedHz.length),
chalk.yellow(formattedHz), 'ops/sec'
);
Expand All @@ -88,7 +90,8 @@ const addTargetTestFunction = require(path.join(__dirname, 'targets', target));
projectPath: projectPath,
suiteOptions: {
// minTime: quick ? undefined : 3.5
}
},
additionalConfig: config
})
);

Expand All @@ -104,4 +107,5 @@ export interface TargetOptions {
brighterscript: typeof import('../src');
projectPath: string;
suiteOptions: Benchmark.Options;
additionalConfig: Record<string, unknown>;
}
3 changes: 2 additions & 1 deletion benchmarks/targets/lex-parse-validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ module.exports = (options: TargetOptions) => {
copyToStaging: false,
//disable diagnostic reporting (they still get collected)
diagnosticFilters: ['**/*'],
logLevel: 'error'
logLevel: 'error',
...options.additionalConfig
}).then(() => {
if (Object.keys(builder.program.files).length === 0) {
throw new Error('No files found in program');
Expand Down
3 changes: 2 additions & 1 deletion benchmarks/targets/lex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ module.exports = async (options: TargetOptions) => {
copyToStaging: false,
//disable diagnostic reporting (they still get collected)
diagnosticFilters: ['**/*'],
logLevel: 'error'
logLevel: 'error',
...options.additionalConfig
});
//collect all the brighterscript files
const brsFiles = Object.values(builder.program.files).filter(x => x.extension === '.brs' || x.extension === '.bs');
Expand Down
5 changes: 3 additions & 2 deletions benchmarks/targets/parse-brs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ module.exports = async (options: TargetOptions) => {
copyToStaging: false,
//disable diagnostic reporting (they still get collected)
diagnosticFilters: ['**/*'],
logLevel: 'error'
logLevel: 'error',
...options.additionalConfig
});
//collect all the brs file contents
const files = Object.values(builder.program.files).filter(x => ['.brs', '.bs', '.d.bs'].includes(x.extension)).map(x => ({
Expand All @@ -27,7 +28,7 @@ module.exports = async (options: TargetOptions) => {
const setFileFuncName = builder.program['setFile'] ? 'setFile' : 'addOrReplaceFile';

suite.add(fullName, (deferred) => {
const promises = [];
const promises: unknown[] = [];
for (const file of files) {
promises.push(
builder.program[setFileFuncName](file.pkgPath, file.fileContents)
Expand Down
5 changes: 3 additions & 2 deletions benchmarks/targets/parse-xml.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,12 @@ module.exports = async (options: TargetOptions) => {
copyToStaging: false,
//disable diagnostic reporting (they still get collected)
diagnosticFilters: ['**/*'],
logLevel: 'error'
logLevel: 'error',
...options.additionalConfig
});
//collect all the XML file contents
const xmlFiles = Object.values(builder.program.files).filter(x => x.extension === '.xml').map(x => ({
srcPath: x.srcPath ?? x.pathAbsolute,
srcPath: x.srcPath ?? (x as any).pathAbsolute,
pkgPath: x.pkgPath,
fileContents: x.fileContents
}));
Expand Down
3 changes: 2 additions & 1 deletion benchmarks/targets/parse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ module.exports = async (options: TargetOptions) => {
copyToStaging: false,
//disable diagnostic reporting (they still get collected)
diagnosticFilters: ['**/*'],
logLevel: 'error'
logLevel: 'error',
...options.additionalConfig
});
//collect all the brighterscript files
const brsFiles = Object.values(builder.program.files).filter(x => x.extension === '.brs' || x.extension === '.bs') as Array<BrsFile>;
Expand Down
3 changes: 2 additions & 1 deletion benchmarks/targets/transpile-brs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ module.exports = async (options: TargetOptions) => {
copyToStaging: false,
//disable diagnostic reporting (they still get collected)
diagnosticFilters: ['**/*'],
logLevel: 'error'
logLevel: 'error',
...options.additionalConfig
});
//collect all the brs files
const files = Object.values(builder.program.files).filter(x => ['.brs', '.bs'].includes(x.extension));
Expand Down
3 changes: 2 additions & 1 deletion benchmarks/targets/transpile-xml.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ module.exports = async (options: TargetOptions) => {
copyToStaging: false,
//disable diagnostic reporting (they still get collected)
diagnosticFilters: ['**/*'],
logLevel: 'error'
logLevel: 'error',
...options.additionalConfig
});
//collect all the XML files
const files = Object.values(builder.program.files).filter(x => x.extension === '.xml');
Expand Down
3 changes: 2 additions & 1 deletion benchmarks/targets/transpile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ module.exports = async (options: TargetOptions) => {
copyToStaging: false,
//disable diagnostic reporting (they still get collected)
diagnosticFilters: ['**/*'],
logLevel: 'error'
logLevel: 'error',
...options.additionalConfig
});
if (Object.keys(builder.program.files).length === 0) {
throw new Error('No files found in program');
Expand Down
3 changes: 2 additions & 1 deletion benchmarks/targets/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ module.exports = async (options: TargetOptions) => {
enableTypeValidation: false,
//disable diagnostic reporting (they still get collected)
diagnosticFilters: ['**/*'],
logLevel: 'error'
logLevel: 'error',
...options.additionalConfig
} as any);
if (Object.keys(builder.program.files).length === 0) {
throw new Error('No files found in program');
Expand Down
6 changes: 3 additions & 3 deletions src/Cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ export class Cache<TKey = any, TValue = any> extends Map<TKey, TValue> {
* otherwise call the factory function to create the value, add it to the cache, and return it.
*/
public getOrAdd<R extends TValue = TValue>(key: TKey, factory: (key: TKey) => R): R {
if (!this.has(key)) {
if (!super.has(key)) {
const value = factory(key);
this.set(key, value);
super.set(key, value);
return value;
} else {
return this.get(key);
return super.get(key) as R;
}
}

Expand Down
18 changes: 4 additions & 14 deletions src/Program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import { SignatureHelpUtil } from './bscPlugin/SignatureHelpUtil';
import { DiagnosticSeverityAdjuster } from './DiagnosticSeverityAdjuster';
import { IntegerType } from './types/IntegerType';
import { StringType } from './types/StringType';
import { SymbolTable, SymbolTypeFlag } from './SymbolTable';
import { SymbolTypeFlag } from './SymbolTable';
import { BooleanType } from './types/BooleanType';
import { DoubleType } from './types/DoubleType';
import { DynamicType } from './types/DynamicType';
Expand All @@ -40,9 +40,6 @@ import { FunctionType } from './types/FunctionType';
import { LongIntegerType } from './types/LongIntegerType';
import { ObjectType } from './types/ObjectType';
import { VoidType } from './types/VoidType';
import { CacheVerifier } from './CacheVerifier';
import { referenceTypeFactory } from './types/ReferenceType';
import { unionTypeFactory } from './types/UnionType';

const startOfSourcePkgPath = `source${path.sep}`;
const bslibNonAliasedRokuModulesPkgPath = s`source/roku_modules/rokucommunity_bslib/bslib.brs`;
Expand Down Expand Up @@ -89,8 +86,6 @@ export class Program {

public logger: Logger;

public typeCacheVerifier = new CacheVerifier();

private createGlobalScope() {
//create the 'global' scope
this.globalScope = new Scope('global', this, 'scope:global');
Expand All @@ -106,11 +101,6 @@ export class Program {
this.globalScope.getDiagnostics = () => [];
//TODO we might need to fix this because the isValidated clears stuff now
(this.globalScope as any).isValidated = true;

// Adds a factory to SymbolTable so it can create ReferenceTypes
SymbolTable.ReferenceTypeFactory = referenceTypeFactory;
SymbolTable.cacheVerifier = this.typeCacheVerifier;
SymbolTable.UnionTypeFactory = unionTypeFactory;
}

/**
Expand Down Expand Up @@ -368,14 +358,14 @@ export class Program {
/**
* roku filesystem is case INsensitive, so find the scope by key case insensitive
*/
public getScopeByName(scopeName: string) {
public getScopeByName(scopeName: string): Scope {
if (!scopeName) {
return undefined;
}
//most scopes are xml file pkg paths. however, the ones that are not are single names like "global" and "scope",
//so it's safe to run the standardizePkgPath method
scopeName = s`${scopeName}`;
let key = Object.keys(this.scopes).find(x => x.toLowerCase() === scopeName.toLowerCase());
scopeName = s`${scopeName.toLowerCase()}`;
let key = Object.keys(this.scopes).find(x => x.toLowerCase() === scopeName);
return this.scopes[key];
}

Expand Down
39 changes: 39 additions & 0 deletions src/Scope.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import { DynamicType } from './types/DynamicType';
import { ObjectType } from './types/ObjectType';
import { FloatType } from './types/FloatType';
import { NamespaceType } from './types/NamespaceType';
import { isFunctionStatement, isNamespaceStatement } from './astUtils/reflection';

describe('Scope', () => {
let sinon = sinonImport.createSandbox();
Expand Down Expand Up @@ -2600,4 +2601,42 @@ describe('Scope', () => {
expectZeroDiagnostics(program);
});
});

describe('unlinkSymbolTable', () => {
it('properly removes sibling symbol tables', () => {
const file = program.setFile<BrsFile>('source/lib.bs', `
namespace alpha.beta.charlie
function delta()
end function
end namespace

namespace alpha.beta.charlie
function echo()
end function
end namespace
`);
const scope = program.getScopeByName('source');

scope.linkSymbolTable();

const opts = { flags: 3 as SymbolTypeFlag };
const symbolTables = [
file.ast.findChild(x => isFunctionStatement(x) && x.name.text === 'delta').findAncestor(x => isNamespaceStatement(x)).getSymbolTable(),
scope.symbolTable.getSymbolType('alpha', opts).memberTable,
scope.symbolTable.getSymbolType('alpha', opts).getMemberType('beta', opts).memberTable,
scope.symbolTable.getSymbolType('alpha', opts).getMemberType('beta', opts).getMemberType('charlie', opts).memberTable
];

symbolTables.forEach(x => expect(x['siblings'].size).to.eql(1, `${x.name} has wrong number of siblings`));
scope.unlinkSymbolTable();
symbolTables.forEach(x => expect(x['siblings'].size).to.eql(0, `${x.name} has wrong number of siblings`));

//do it again, make sure we don't end up with additional siblings

scope.linkSymbolTable();
symbolTables.forEach(x => expect(x['siblings'].size).to.eql(1, `${x.name} has wrong number of siblings`));
scope.unlinkSymbolTable();
symbolTables.forEach(x => expect(x['siblings'].size).to.eql(0, `${x.name} has wrong number of siblings`));
});
});
});
Loading