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

feat: format with generator #82

Closed
wants to merge 1 commit into from

Conversation

H4ad
Copy link
Contributor

@H4ad H4ad commented Jan 13, 2024

This PR adds a new method to be able to consume the format edits using generators, which can help reduce the amount of memory needed to process a heavier json.

With the current implementation, using the benchmark of #81, we had a slowdown while using sync result:

End: 1264.72ms
RSS: 693.6484375MB

But, if we consider how this API can be used, for example to replace the current implementation of format on vscode-json-language-service, we have good results:

Before:

End of Sync: 1832.79ms
RSS: 937.5625MB
text-format-with-map.js
const heavyJson = require('fs').readFileSync('./rrdom-benchmark-1.json', 'utf8');

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

class Range {
  construtor(start, end) {
    this.start = start;
    this.end = end;
  }
}

class TextEdit {
  construtor(range, content) {
    this.range = range;
    this.content = content;
  }
}

const startTime = performance.now();
const edits = lib.format(heavyJson, undefined, {
  tabSize: 2,
  insertFinalNewline: true,
  insertSpaces: true,
});
const mappedEdits = edits.map(e => {
  return new TextEdit(
    new Range(e.offset, e.offset + e.length),
    e.content,
  );
});
console.log(`End of Sync: ${(performance.now() - startTime).toFixed(2)}ms`);
console.log(`RSS: ${process.memoryUsage.rss() / 1024 / 1024}MB`);

After:

End of Generator: 1661.84ms
RSS: 471.7890625MB
test-format-with-map-generator.js
const heavyJson = require('fs').readFileSync('./rrdom-benchmark-1.json', 'utf8');

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

class Range {
  construtor(start, end) {
    this.start = start;
    this.end = end;
  }
}

class TextEdit {
  construtor(range, content) {
    this.range = range;
    this.content = content;
  }
}

const startTime = performance.now();
const mappedEdits = [];

for (const e of lib.formatGenerator(heavyJson, undefined, {
  tabSize: 2,
  insertFinalNewline: true,
  insertSpaces: true,
})) {
  mappedEdits.push(new TextEdit(
    new Range(e.offset, e.offset + e.length),
    e.content,
  ));
}

console.log(`End of Generator: ${(performance.now() - startTime).toFixed(2)}ms`);
console.log(`RSS: ${process.memoryUsage.rss() / 1024 / 1024}MB`);

This new approach will be faster and also will consume less memory 🚀

@H4ad H4ad force-pushed the perf/format-with-generator branch from d37ab29 to 34162ac Compare January 22, 2024 20:33
@H4ad H4ad marked this pull request as ready for review January 22, 2024 20:35
@H4ad
Copy link
Contributor Author

H4ad commented Jan 29, 2024

Hey @aeschli, did you have time to take a look on this one?

@H4ad
Copy link
Contributor Author

H4ad commented Mar 20, 2024

@aeschli Sorry for pinging again, but can you take a look on this one?

@aeschli
Copy link
Contributor

aeschli commented Mar 20, 2024

Using the generator is an interesting idea. It's not something we in VS Code could use right away given the way the rest of our APIs are designed.
So I'm a bit hesitant about adding a new API (and maintaining it) unless there's considerable demand for it.

@H4ad
Copy link
Contributor Author

H4ad commented Mar 20, 2024

I think this feature can help reduce memory usage but it also introduces generators by default even when we don't want them, which leads to slower formatting since generators are slower.

I hope in the future vscode will support a way to send partial updates to the UI, if it happens, then we can use a feature like this with the full power.

@aeschli
Copy link
Contributor

aeschli commented Mar 21, 2024

At this moment there's just too much missing to make the generator API useful.

  • LSP doesn't support streaming for format
  • VS Code API expecting a TextEdit array, not a stream
  • Applying a TextEdit array to a document is not trivial. In a TextEdit[], all TextEdit ranges refer to the original document, so once you have applied an edit you have to transform the range of the next TextEdit.

I don't see us getting there any time soon. I'd rather not maintain an API that we don't use. If you depend on it, I think it is fait to copy the code and maintain it on your side
Closing the PR

@aeschli aeschli closed this Mar 21, 2024
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.

2 participants