[build] JSG: key struct field wrappers on the field type, not on the field - #7248
Open
fhanau wants to merge 1 commit into
Open
[build] JSG: key struct field wrappers on the field type, not on the field#7248fhanau wants to merge 1 commit into
fhanau wants to merge 1 commit into
Conversation
…field FieldWrapper was parameterized on the declaring struct, a pointer to the member, and a pointer to the field's exported name, so every field of a JSG_STRUCT got its own instantiation of the class. Nothing in FieldWrapper actually depends on which field it serves, only on the field's type. This results in many instances of FieldWrapper, who can have large mangled names based on the template arguments. Pass those three things in at runtime instead. StructWrapper already knows the struct, so it does the member access itself and hands the field to FieldWrapper by reference. The exported name is stored in the wrapper and typeid(Struct) is passed to unwrap() for error messages. FieldWrapper is now instantiated once per field type, shared by every field of that type. In the downstream api translation unit that reduces the number of field wrappers by over 80%. To make the names available at runtime, JSG_STRUCT adds a jsgFieldNames table and the field list becomes a pack of pointers to members. The table also replaces jsgGetTemplate() and jsgAddToStructNames(): building the DictionaryTemplate is now a loop in StructWrapper over that table rather than a per-field function template.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| template <typename TypeWrapper, typename Self> \ | ||
| using JsgFieldWrappers = \ | ||
| ::workerd::jsg::TypeTuple<JSG_FOR_EACH(JSG_STRUCT_FIELD, , __VA_ARGS__)>; \ | ||
| static constexpr ::kj::StringPtr jsgFieldNames[] KJ_UNUSED = { \ |
Contributor
There was a problem hiding this comment.
JSG_STRUCT is a public macro and these two ordinary member names were not previously reserved. A struct that declares jsgFieldNames or JsgFields now fails to compile when it is annotated, which is a source-compatibility regression for embedders. Use the existing explicit internal-name convention for the generated members and update the corresponding consumers.
Suggested change
| static constexpr ::kj::StringPtr jsgFieldNames[] KJ_UNUSED = { \ | |
| static constexpr ::kj::StringPtr _JSG_STRUCT_FIELD_NAMES_DO_NOT_USE_DIRECTLY[] KJ_UNUSED = { \ |
Collaborator
There was a problem hiding this comment.
While it's not an actual problem that breaks any actual code, I think the suggestion is probably a good idea. The DO_NOT_USE_DIRECTLY is a worthwhile hint. I wouldn't consider these suggestions to be blocking in any way tho.
| return v8::DictionaryTemplate::New( \ | ||
| isolate, std::span<const std::string_view>(namesPtr.begin(), namesPtr.size())); \ | ||
| } \ | ||
| using JsgFields = ::workerd::jsg::StructFields<JSG_FOR_EACH(JSG_STRUCT_FIELD, , __VA_ARGS__)>; \ |
Contributor
There was a problem hiding this comment.
Suggested change
| using JsgFields = ::workerd::jsg::StructFields<JSG_FOR_EACH(JSG_STRUCT_FIELD, , __VA_ARGS__)>; \ | |
| using _JSG_STRUCT_FIELDS_DO_NOT_USE_DIRECTLY = ::workerd::jsg::StructFields<JSG_FOR_EACH(JSG_STRUCT_FIELD, , __VA_ARGS__)>; \ |
Comment on lines
+311
to
+312
| return lazyFields.emplace( | ||
| kj::tuple(FieldWrapper<Self, FieldType<fields>>(isolate, T::jsgFieldNames[indices])...)); |
Contributor
There was a problem hiding this comment.
Suggested change
| return lazyFields.emplace( | |
| kj::tuple(FieldWrapper<Self, FieldType<fields>>(isolate, T::jsgFieldNames[indices])...)); | |
| kj::tuple(FieldWrapper<Self, FieldType<fields>>( | |
| isolate, T::_JSG_STRUCT_FIELD_NAMES_DO_NOT_USE_DIRECTLY[indices])...)); |
| static v8::Local<v8::DictionaryTemplate> makeTemplate(v8::Isolate* isolate) { | ||
| static constexpr bool isUsable[] = {isUsableStructField<FieldType<fields>>...}; | ||
| kj::Vector<std::string_view> names(kCountOfUsableFields); | ||
| for (auto i: kj::indices(T::jsgFieldNames)) { |
Contributor
There was a problem hiding this comment.
Suggested change
| for (auto i: kj::indices(T::jsgFieldNames)) { | |
| for (auto i: kj::indices(T::_JSG_STRUCT_FIELD_NAMES_DO_NOT_USE_DIRECTLY)) { |
| template <typename Self, typename T> | ||
| class TypeWrapperBase<Self, T, JsgKind::STRUCT> | ||
| : public StructWrapper<Self, T, typename T::template JsgFieldWrappers<Self, T>> { | ||
| : public StructWrapper<Self, T, typename T::template JsgFields<T>> { |
Contributor
There was a problem hiding this comment.
Suggested change
| : public StructWrapper<Self, T, typename T::template JsgFields<T>> { | |
| : public StructWrapper<Self, T, typename T::template _JSG_STRUCT_FIELDS_DO_NOT_USE_DIRECTLY<T>> { |
Contributor
|
I'm Bonk, and I've done a quick review of your PR. Refactors JSG struct field wrappers to reduce template instantiations and binary size.
|
jasnell
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FieldWrapper was parameterized on the declaring struct, a pointer to the member, and a pointer to the field's exported name, so every field of a JSG_STRUCT got its own instantiation of the class. Nothing in FieldWrapper actually depends on which field it serves, only on the field's type. This results in many instances of FieldWrapper, who can have large mangled names based on the template arguments.
Pass those three things in at runtime instead. StructWrapper already knows the struct, so it does the member access itself and hands the field to FieldWrapper by reference. The exported name is stored in the wrapper and typeid(Struct) is passed to unwrap() for error messages. FieldWrapper is now instantiated once per field type, shared by every field of that type. In the downstream api translation unit that reduces the number of field wrappers by over 80%.
To make the names available at runtime, JSG_STRUCT adds a jsgFieldNames table and the field list becomes a pack of pointers to members. The table also replaces jsgGetTemplate() and jsgAddToStructNames(): building the DictionaryTemplate is now a loop in StructWrapper over that table rather than a per-field function template.
==============
This massively reduces code size by reducing excessive template instantiations. Compile time (which I think people care about more) also improves. Part of a 3-part series to debloat workerd-api. Code changes made using an LLM, fully reviewed and edited by me.
No optimization, limited debug info, before change:
No optimization, limited debug info, after:
When compiling server directory with -O3, before:
When compiling server directory with -O3, after: