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

Simplify identity source map generator #265

Merged
merged 2 commits into from Jun 27, 2018

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Jun 18, 2018

This should be a lot faster than using the source-map library, since all we're doing is generating an identity line map.

@alangpierce
Copy link
Owner

Thank you! And sorry for the delay, work stuff has been a bit crazy recently.

I worry a bit about this code being really mysterious to readers or fragile if it gets any more complex. Could you bring back source-map as a devDependency and add a test that creates the mappings using that library and asserts that the results are the same? (You'll still need the RawSourceMap explicitly written out like you have, since it's part of the public API.)

An alternative to consider is something like ";AACA".repeat(numLines), which theoretically could be faster since it knows up-front how big the string is going to be. But I just tested it, and it seemed either the same or slightly slower, so what you have looks good to me!

@MaxGraey
Copy link

MaxGraey commented Jun 21, 2018

Also I recommend use this:

const NEW_LINE_CODE = "\n".charCodeAt(0);
for (...) {
  if (code.charCodeAt(i) === NEW_LINE_CODE) {
    ...
  }
}

instead:

for (...) {
  if (code[i] === "\n") {
    ...
  }
}

This could be also fast:

const NEW_LINE_CODE = "\n".charCodeAt(0);
let numLines = 0;
for (...) {
   numLines += ((code.charCodeAt(i) === NEW_LINE_CODE) | 0);
}
mappings = ";AACA".repeat(numLines);

But need tests

@alangpierce
Copy link
Owner

@MaxGraey thanks! I was actually skeptical that it would really matter (V8 seems to generally be smart about small code snippets), but from my testing it looks like that is measurably faster. So @aleclarson would be great if you included that as well.

Some informal measurements from running the benchmark about 5 times each:
460ms baseline (no source maps)
505ms with === "\n"
485ms with === charCodes.lineFeed

There's already a charCodes const enum, so that would be the way to do it in this case:

import {charCodes} from "./parser/util/charcodes";
...
if (code.charCodeAt(i) === charCodes.lineFeed) {

Copy link
Owner

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

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

Moving to "requested changes" state per comments above. @aleclarson, would be nice to get this merged and released reasonably soon, since I think it does improve perf for my main use case. I'd be happy to take over if you don't have bandwidth.

Copy link
Owner

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

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

Thanks!

Still would be nice to have a test that source-map and this code produce the same result, but it's fine for that to happen in a follow-up. In general feels like there's still a little more to do before this source map stuff is settled.

@alangpierce alangpierce merged commit eb1d9b8 into alangpierce:master Jun 27, 2018
@aleclarson aleclarson deleted the sourcemaps branch June 28, 2018 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants