Skip to content

Add an allocator for external strings#6307

Merged
danlapid merged 14 commits intomainfrom
dcarney/external-strings
Mar 12, 2026
Merged

Add an allocator for external strings#6307
danlapid merged 14 commits intomainfrom
dcarney/external-strings

Conversation

@dcarney-cf
Copy link
Contributor

No description provided.

@dcarney-cf dcarney-cf requested review from a team as code owners March 12, 2026 07:51
Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

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 adds an ExternalStringAllocator abstraction for V8 external string resources, replacing direct new/delete with a configurable allocator threaded through IsolateBase.

Issues found (highest severity first):

  1. [HIGH] Compile breakage — The primary Isolate constructor (with IsolateGroup) now requires an externalStringAllocator parameter, but existing callers in workerd-api.c++, setup-test.c++, modules-new-test.c++, and multiple-typewrappers-test.c++ were not updated.
  2. [MEDIUM] Silent failure from allocate returning nullptrcreateExtern returns an empty MaybeLocal when allocation fails, but the callers (newExternalOneByteString, newExternalTwoByteString) pass the result through check() which throws JsExceptionThrown without an actual JS exception pending.
  3. [LOW] Allocator interface lacks alignment contractallocate(size_t) doesn't specify alignment requirements. ExternString contains a vtable pointer and kj::ArrayPtr, requiring at least pointer alignment. Custom allocators could violate this.

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

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 12, 2026

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

This PR adds an ExternalStringAllocator abstraction for V8 external string resources, replacing direct new/delete with a configurable allocator threaded through IsolateBase.

I posted a batch review on PR #6307 with 3 inline comments covering:

  1. [HIGH] Compile breakage — The primary Isolate constructor now requires externalStringAllocator but existing callers in workerd-api.c++, setup-test.c++, modules-new-test.c++, and multiple-typewrappers-test.c++ were not updated. Suggested defaulting the parameter to kj::heap<DefaultExternalStringAllocator>().

  2. [MEDIUM] Silent failure path — When allocate() returns nullptr, createExtern returns an empty MaybeLocal, but callers pass it through check() which throws JsExceptionThrown without an actual JS exception pending.

  3. [LOW] Missing alignment contract — The allocate(size_t) interface doesn't document or enforce alignment requirements needed by ExternString's vtable and pointer members.

github run

@dcarney-cf dcarney-cf requested a review from erikcorry March 12, 2026 07:55
@github-actions
Copy link

github-actions bot commented Mar 12, 2026

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

Copy link
Contributor

@erikcorry erikcorry left a comment

Choose a reason for hiding this comment

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

Probably needs a test with an allocator that says no, to ensure the exception throwing works.

@dcarney-cf dcarney-cf force-pushed the dcarney/external-strings branch from 7a9405e to 79679cc Compare March 12, 2026 11:53
@dcarney-cf dcarney-cf enabled auto-merge March 12, 2026 17:09
@dcarney-cf dcarney-cf disabled auto-merge March 12, 2026 19:47
@dcarney-cf dcarney-cf enabled auto-merge March 12, 2026 19:47
@danlapid danlapid disabled auto-merge March 12, 2026 22:19
@danlapid danlapid merged commit 329f6d4 into main Mar 12, 2026
16 of 17 checks passed
@danlapid danlapid deleted the dcarney/external-strings branch March 12, 2026 22:19
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.

5 participants