-
Notifications
You must be signed in to change notification settings - Fork 54
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
OOMs on the Jest docusaurus site #35
Comments
I think it's more likely that the issue lays in the remark-shiki-twoslash somehow re-creating the highlighters, but I can't prove it. |
Came across this bug today. This is a severe bug since this makes twoslash completely unusable. I think a lot more people might be using docusaurus with jest and this breaks the setup entirely. The error is also scarier because I don't understand any of the logs and what's happening. |
Could this be related to this facebook/docusaurus#4978? |
Yeah, I thought we were correctly caching the highlighters with https://github.com/shikijs/twoslash/blob/main/packages/remark-shiki-twoslash/src/index.ts#L61-L62 But maybe caching the highlighter doesn't cache the syntax JSONs It's not a problem I've had on Gatsby with the TS website which has many hundreds of twoslash code samples, so there's a chance it's a docusaurus level but I think we can still find a way at this library level |
I think #43 might have fixed this, |
I just checked it. Indeed, it is fixed. Thanks! |
Probably not really the fault of twoslash. But remotion-dev/remotion@2f00e61 is still experiencing this. I'm guessing those garbage collectible weakmaps can only help a bit, probably might have to wait for facebook/docusaurus#4997. |
This is reproducible with |
Managed to reproduce the behavior with 80 (it was fine till 50) markdown files of this content on a plain docusaurus template. ```ts twoslash
type CreateMutable<Type> = {
-readonly [Property in keyof Type]: Type[Property];
};
type LockedAccount = {
readonly id: string;
readonly name: string;
};
type UnlockedAccount = CreateMutable<LockedAccount>;
\``` The content is probably not important, just put it out there for reference. |
When I increased the memory to
Maybe XML is related to tsx, but I don't have these groovy and erlang languages used in the code. Does shiki read all these files necessary or not?? Ref: https://github.com/remotion-dev/remotion/runs/2931935721 |
Hrm, still seems a tad off - I would have thought that caching the shiki highlighter would mean those files only get set up once, but if there's dupes in there - it obviously isn't |
Edit: Nope. Nevermind. It's still the same error on remotion code base with a lot more content. Hey, I've found a fix. Use the
The compilation time is also normal and way less longer than with the preset. |
I'm really confused right now. Does the remark-shiki-twoslash code contain a logic bug or something? I set up a counter server and got this (on my 80 files docusaurus setup with remark plugin). {
"getHighlighter": 182, // how many times shiki.getHighlighter is called
"setCache": 0, // how many times the data is set to weakmap
"getFromCache": 0, // how many times the highlighters are taken from weakmap
"fileHit": 1 // how many times the file is called
} |
EDIT: I think I found the fix. Just needed to make the EDIT: Tried the fix prototype with 100+ files. Working smooth like butter. Woo. I found out that the caches are set but every remark API call tries to load the highlighter and set the cache also. It seems like the remark's async handlers are being run parallel and the getHighlighter is taking quite some time so caching becomes useless as all of the functions are trying to load highlighters into cache at the same time. When I add a before 0 // <- length of highlightCache map
before 0
before 0
// ...
after 1 // <- length of highlightCache map
after 2 // <- why duplicated when there's only one setting
after 3 There's also the problem with duplicated keys in the map. The correct behavior would be, before 0
after 1 // only hit once because the function shouldn't reach to end if the cache exists
before 1
before 1
before 1 |
Maybe a leak in Shiki?
The text was updated successfully, but these errors were encountered: