Skip to content

Compile SQLite with SQLITE_OMIT_SHARED_CACHE#6569

Merged
joshthoward merged 1 commit into
mainfrom
jhoward/omit-shared-cache
Apr 13, 2026
Merged

Compile SQLite with SQLITE_OMIT_SHARED_CACHE#6569
joshthoward merged 1 commit into
mainfrom
jhoward/omit-shared-cache

Conversation

@joshthoward

@joshthoward joshthoward commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

This change compiles SQLite with SQLITE_OMIT_SHARED_CACHE. From the docs:

Omitting the possibility of using shared cache allows many conditionals in performance-critical sections of the code to be eliminated. This can give a noticeable improvement in performance.

Usage of the shared cache is also incompatible with our memory metering implementation, #6380.

@joshthoward

joshthoward commented Apr 13, 2026

Copy link
Copy Markdown
Contributor Author

No idea why the latest commit in main at the time of raising this PR is showing up in my PR...

ask-bonk[bot]
ask-bonk Bot reviewed Apr 13, 2026
View reviewed changes

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm Bonk, and I've done a quick review of your PR.

This PR contains two separate changes: (1) compiling SQLite with SQLITE_OMIT_SHARED_CACHE, and (2) plumbing a ResourceLimits struct through the dynamic worker loading API. The PR title/description only describes change (1).

  1. [MEDIUM] limits parameter is silently ignored in server.c++. Both getEntrypoint() and getActorClass() accept the new kj::Maybe<ResourceLimits> limits parameter but never use it. Similarly, DynamicWorkerSource.limits is populated but never read when constructing the WorkerDef in WorkerStubImpl::start() (line ~4194). A user passing limits: { cpuMs: 100 } would get no effect and no error. If this is intentional plumbing for a future feature, it should be documented. If it's meant to work now, the implementation is missing.

  2. [LOW] Generated TypeScript interface workerdResourceLimits uses lowercase first letter. This is the only interface in the entire generated types file that starts with a lowercase letter, because ResourceLimits is registered via workerd::ResourceLimits (parent namespace) rather than api::ResourceLimits. Consider adding a JSG_STRUCT_TS_OVERRIDE or moving the struct into the api namespace.

This review was generated by an AI assistant and may contain inaccuracies.

Comment thread src/workerd/server/server.c++
Comment thread src/workerd/io/io-channels.h
@ask-bonk

ask-bonk Bot commented Apr 13, 2026

Copy link
Copy Markdown
Contributor

The review was posted successfully with both inline comments. Here's a summary of the review I posted on PR #6569:

Two findings:

  1. [MEDIUM] limits parameter silently ignored — The ResourceLimits plumbing (from the first commit) adds limits to the API surface (WorkerCode, EntrypointOptions, DynamicWorkerSource, WorkerStubChannel::getEntrypoint/getActorClass), but server.c++ never actually consumes the value. The WorkerDef construction in WorkerStubImpl::start() doesn't read source.limits. Users passing limits would get silent no-ops.

  2. [LOW] TypeScript naming convention — The generated workerdResourceLimits interface is the only lowercase-initial interface in the generated types, caused by the struct living in workerd:: namespace rather than workerd::api::.

The SQLite SQLITE_OMIT_SHARED_CACHE change (the one described in the PR title) is clean and straightforward — it's a single define addition in the build file with a clear rationale.

github run

@joshthoward joshthoward force-pushed the jhoward/omit-shared-cache branch from 8b54888 to 809bf0a Compare April 13, 2026 15:12
@joshthoward joshthoward marked this pull request as ready for review April 13, 2026 15:44
@joshthoward joshthoward requested review from a team as code owners April 13, 2026 15:44
@joshthoward joshthoward enabled auto-merge April 13, 2026 15:44
@joshthoward joshthoward merged commit cee9c5d into main Apr 13, 2026
22 checks passed
@joshthoward joshthoward deleted the jhoward/omit-shared-cache branch April 13, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ask-bonk ask-bonk[bot] ask-bonk[bot] left review comments

@a-robinson a-robinson a-robinson approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@joshthoward @a-robinson