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

perf(format): cache breaklines and spaces as much as possible #81

Merged
merged 1 commit into from
Jan 22, 2024

Conversation

H4ad
Copy link
Contributor

@H4ad H4ad commented Jan 13, 2024

This PR basically tries to cache as much as possible the strings and spaces created since they are basically the same, there's no need to allocate new memory for those strings.

This optimization affects eol:

  • \r
  • \n
  • \r\n

This optimization will only cache strings up to 200 characters (it was chosen just because it speedups tabSize=4 for this huge json).

In the happy path, this algorithm will only allocate memory for the array and the objects, but not for the content.

For smaller files, this change didn't make the code run faster, this is only valid for larger json.

Before:

End: 2431.62ms
RSS: 2290.38671875MB
Amount of edits: 7876047

After:

End: 989.64ms
RSS: 709.5234375MB
Amount of edits: 7876047

Benchmark:

const heavyJson = require('fs').readFileSync('./rrdom-benchmark-1.json', 'utf8');

const lib = require('./lib/umd/main');

const startTime = performance.now();
const edits = lib.format(heavyJson, undefined, {
  tabSize: 2,
  insertFinalNewline: true,
  insertSpaces: true,
});
console.log(`End: ${(performance.now() - startTime).toFixed(2)}ms`);
console.log(`RSS: ${process.memoryUsage.rss() / 1024 / 1024}MB`);
console.log(`Amount of edits: ${edits.length}`);

Large Json: rrdom-benchmark-1.json.zip

@H4ad
Copy link
Contributor Author

H4ad commented Jan 13, 2024

@microsoft-github-policy-service agree

@H4ad H4ad changed the title perf(format): internalize breaklines and spaces as much as possible perf(format): cache breaklines and spaces as much as possible Jan 13, 2024
@vscodenpa vscodenpa added this to the December / January 2024 milestone Jan 22, 2024
@aeschli aeschli merged commit c6d4008 into microsoft:main Jan 22, 2024
2 checks passed
@H4ad H4ad deleted the perf/intern-strings branch January 22, 2024 19:55

let numberLineBreaks = 0;

let indentLevel = 0;
let indentValue: string;
if (options.insertSpaces) {
indentValue = repeat(' ', options.tabSize || 4);
indentValue = cachedSpaces[options.tabSize || 4] ?? repeat(cachedSpaces[1], options.tabSize || 4);
Copy link

Choose a reason for hiding this comment

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

the ?? operator is a breaking change for node 12. since there's no engines declaration, everything that worked in 3.0.0 is part of the public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The minimum version of this package is 14.x, so I think this is expected, it worked before because of luck not because was the intention.

An PR can be created to fix this but probably this can happen more times in the future.

Copy link

Choose a reason for hiding this comment

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

That's only the case if it's declared in engines.node - intention doesn't matter for semver.

The way to avoid it happening in the future, after fixing it and releasing a patch, is to either add node 12 into the CI matrix, or, to immediately add engines.node and do a major bump.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the minimum is now 16.x.

Copy link

Choose a reason for hiding this comment

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

Then this should have been a v4.0.0 that included an explicit engines.node declaration.

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.

5 participants