-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Diffs Performance Improvements #5653
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
Changes from all commits
de2f38b
3e6922a
373f4ee
8d7016e
5681e90
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import type { WorkerPoolManager } from "@pierre/diffs/worker" | ||
| import { createSimpleContext } from "./helper" | ||
|
|
||
| const ctx = createSimpleContext<WorkerPoolManager | undefined, { pool: WorkerPoolManager | undefined }>({ | ||
| name: "WorkerPool", | ||
| init: (props) => props.pool, | ||
| }) | ||
|
|
||
| export const WorkerPoolProvider = ctx.provider | ||
| export const useWorkerPool = ctx.use |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,15 @@ | ||
| import { DIFFS_TAG_NAME } from "@pierre/diffs" | ||
|
|
||
| /** | ||
| * TypeScript declaration for the <diffs-container> custom element. | ||
| * This tells TypeScript that <diffs-container> is a valid JSX element in SolidJS. | ||
| * Required for using the @pierre/diffs web component in .tsx files. | ||
| */ | ||
|
|
||
| declare module "solid-js" { | ||
| namespace JSX { | ||
| interface IntrinsicElements { | ||
| "diffs-container": HTMLAttributes<HTMLElement> | ||
| [DIFFS_TAG_NAME]: HTMLAttributes<HTMLElement> | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure the types are more future compatible if we ever decide to change this |
||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,28 @@ | ||
| import { getOrCreateWorkerPoolSingleton } from "@pierre/diffs/worker" | ||
| import { getOrCreateWorkerPoolSingleton, WorkerPoolManager } from "@pierre/diffs/worker" | ||
| import ShikiWorkerUrl from "@pierre/diffs/worker/worker.js?worker&url" | ||
|
|
||
| export function workerFactory(): Worker { | ||
| return new Worker(ShikiWorkerUrl, { type: "module" }) | ||
| } | ||
|
|
||
| export const workerPool = getOrCreateWorkerPoolSingleton({ | ||
| poolOptions: { | ||
| workerFactory, | ||
| // poolSize defaults to 8. More workers = more parallelism but | ||
| // also more memory. Too many can actually slow things down. | ||
| // poolSize: 8, | ||
| }, | ||
| highlighterOptions: { | ||
| theme: "OpenCode", | ||
| // Optionally preload languages to avoid lazy-loading delays | ||
| // langs: ["typescript", "javascript", "css", "html"], | ||
| }, | ||
| }) | ||
| export const workerPool: WorkerPoolManager | undefined = (() => { | ||
| if (typeof window === "undefined") { | ||
| return undefined | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Essentially you don't want to kick off a worker pool on the server, so this is a quick way to ensure that doesn't happen |
||
| } | ||
| return getOrCreateWorkerPoolSingleton({ | ||
| poolOptions: { | ||
| workerFactory, | ||
| // poolSize defaults to 8. More workers = more parallelism but | ||
| // also more memory. Too many can actually slow things down. | ||
| // NOTE: 2 is probably better for OpenCode, as I think 8 might be | ||
| // a bit overkill, especially because Safari has a significantly slower | ||
| // boot up time for workers | ||
| poolSize: 2, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think for ya'lls usecase, having 8 workers is overkill (this is on me for making it a default) and I think in practice 2 is probably a lot better because I've found workers to be much slower to spin up on Safari in general so better to spin up a bit quicker imo. Feel free to obviously tweak this value if you find there are good usecases to get more parallelization |
||
| }, | ||
| highlighterOptions: { | ||
| theme: "OpenCode", | ||
| // Optionally preload languages to avoid lazy-loading delays | ||
| // langs: ["typescript", "javascript", "css", "html"], | ||
| }, | ||
| }) | ||
| })() | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Server rendered FileDiffs need to get the worker on instantiation