Skip to content

Optimize source mapping into external source map sources #40055

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

Merged

Conversation

JoostK
Copy link
Contributor

@JoostK JoostK commented Aug 14, 2020

Fixes #40054

Timings

fn Before After Improvement
emit phase 13.4 8.8 -34%
emitSourcePos 4.8s 0.6s -87%
setSourceMapSource 4.3s 0.1s -98%

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Aug 14, 2020
@@ -51,6 +52,10 @@ namespace ts {

function addSource(fileName: string) {
enter();
const sourceIndexByFileName = fileNameToSourceIndexMap.get(fileName);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation note: this could alternatively be done without a full cache, using just a single MRU fileName/sourceIndex. That does however require that the second call to setSourceMapSource in emitSourcePos has to take an additional parameter to not even call addSource (which is safe as reverting to the prior source map source is known to have been added) as otherwise 2 MRU items are needed:

function emitSourcePos(source: SourceMapSource, pos: number) {
if (source !== sourceMapSource) {
const savedSourceMapSource = sourceMapSource;
setSourceMapSource(source);
emitPos(pos);
setSourceMapSource(savedSourceMapSource);
}
else {
emitPos(pos);
}
}

This alternative would also mitigate the issue (I verified that) as it's typical for a single external source map source to be applied on nodes without interference from nodes that have a different external source map source, so a single MRU cache entry is sufficient.

@JoostK JoostK marked this pull request as ready for review August 14, 2020 21:10
@JoostK JoostK force-pushed the optimize-external-source-map-source branch from afb77aa to 4b4a156 Compare August 15, 2020 17:14
@DanielRosenwasser DanielRosenwasser merged commit 1e84ffc into microsoft:master Aug 19, 2020
@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Aug 19, 2020
@DanielRosenwasser
Copy link
Member

Shucks, we really should have run a perf test on this.

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 19, 2020

Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 4b4a156. You can monitor the build here.

@DanielRosenwasser
Copy link
Member

well duh, that'll do nothing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance bottleneck with external source map sources
4 participants