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

Question about _fetchJSONAssets #439

Closed
banga opened this issue Mar 5, 2023 · 4 comments · Fixed by #443
Closed

Question about _fetchJSONAssets #439

banga opened this issue Mar 5, 2023 · 4 comments · Fixed by #443

Comments

@banga
Copy link
Contributor

banga commented Mar 5, 2023

Hello again,

I'm wondering why _fetchJSONAssets uses jsonc-parser instead of JSON.parse. It looks like all of the bundled themes and languages are valid JSON:

find ./packages/shiki/ -name "*.json" | \
  grep -e "\(languages\|themes\)" | \
  xargs -n1 -I{} node -e "JSON.parse(fs.readFileSync('{}','utf-8'))"

_fetchJSONAssets takes around 300ms right now, but switching it to JSON.parse brings it down to 30ms. I ask because this is the source of a noticeable delay in start up time for https://github.com/banga/git-split-diffs.

@orta
Copy link
Contributor

orta commented Mar 5, 2023

It was before my time, but I'd guess because the vscode parser uses jsonc then folks probably add comments to the JSON files - there could maybe be a processing step in this repo to allow them to be converted to something JSON.parse could handle during a deploy though

@banga
Copy link
Contributor Author

banga commented Mar 6, 2023

I see, yeah that would be great (unless people supply their own language/theme files at runtime). Another option is that we try JSON.parse and then fall back to jsonc-parser if it throws. I'd be happy to put up a PR for that, if that sounds good to you.

@orta
Copy link
Contributor

orta commented Mar 7, 2023

I think trying JSON parse then falling back is a good answer 👍🏻

@DanielRosenwasser
Copy link

DanielRosenwasser commented Mar 7, 2023

That 270ms difference is surprising - I wonder if VS Code itself tries to side-step that.

banga added a commit to banga/shiki that referenced this issue Mar 7, 2023
This improves the parsing time for themes and grammars by first using
`JSON.parse` and then falling back on `jsonc-parser`. Since all of the
themes and grammars currently checked in are valid JSON, this speeds up
startup time considerably:

```
import shiki from './packages/shiki/dist/index.js'
import { hrtime } from 'process';

const start = hrtime()
const highlighter = await shiki.getHighlighter({ theme: 'nord'});
console.log(hrtime(start)[1] / 1e6)
```

This goes from ~420ms to ~185ms on my machine.

Fixes shikijs#439
@orta orta closed this as completed in #443 Mar 16, 2023
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 a pull request may close this issue.

3 participants