perf(metadata): reference interfaces by class-name index - #436
Draft
edusperoni wants to merge 6 commits into
Draft
perf(metadata): reference interfaces by class-name index#436edusperoni wants to merge 6 commits into
edusperoni wants to merge 6 commits into
Conversation
…ding serializer BinaryTypeEncodingSerializer held its BinaryWriter by value. The writer owns the string-interning map, so the copy interned into a second map and every string reachable from both the meta path and the type-encoding path was written to the heap twice. 4519 strings were stored at two offsets; every one of them was the cross-path case.
push_binaryArray wrote a fresh count-of-zero for every empty array, and nothing deduplicated them. Empty protocol lists alone accounted for 95k of them. Empty arrays carry no payload, so they can all share one offset. Offset 0 stays the null sentinel: the heap reserves a marker byte there, so no real array can land on it.
57933 of 60589 methods carry no constructor tokens, yet every MethodMeta paid a 4-byte pointer for the field. It is the trailing field and MethodMeta has no subclass, so it can be left out entirely and gated on a flag. Also widens the member flag mask: it cleared bits 8 and up alongside the type bits, which would silently discard any member flag stored there. No member in the SDK sets bit 8 today, so this changes nothing on its own.
An interface reference in a type encoding cost 9 bytes: a tag, a name pointer and a protocols pointer. 88172 of the 92844 references in the SDK carry no protocols at all, and only 2960 distinct class names are referenced. Those references now encode as a tag plus a uint16 index into a new class-name table, 3 bytes instead of 9. References that do carry protocols keep the old form, as does any reference the generator cannot index because the table is full. The table sits between the module table and the heap, so the runtime resolves it once in setInstance rather than walking the preceding tables on every lookup. Readers must test isInterfaceReference() rather than comparing against a single tag, and read the name through interfaceName(); comparing against InterfaceDeclarationReference alone would silently drop the indexed form into whatever the default branch does. Also gates the empty-array interning behind an explicit flag. Header tables are located positionally, so interning one there would move every table after it — harmless while only one such table could be empty, but not once a second exists.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Every method and function signature wrote its own encoding list, so common shapes were stored thousands of times over. Lists are now serialized to scratch and interned by their exact bytes; nothing in them depends on where the list lands, so identical lists are interchangeable. This is worth far more after the class-name index than before it: encoding the common interface reference as a bare index removed the per-reference pointers that used to make otherwise-identical signatures differ. Sharing lists means two declarations can now point at one encoding, which the ffi_cif cache was not prepared for -- it keyed on the encoding pointer alone while callers pass different initialParameterIndex/argsCount, so aliasing would hand back a cif describing the wrong stack. It now keys on all three.
Enums were serialized as JS source, __tsEnum({"A":1,...}), compiled and run on
first access. Almost all of that text was redundant: every member name is
already interned in the heap because the same enum's constants are emitted as
standalone globals, so the source string held a second copy of each name plus
JSON punctuation.
They now serialize as a count followed by (name offset, int64 value) pairs and
the runtime builds the object directly. Entries stay in stored order because
__tsEnum's reverse mapping is last-write-wins, so reordering would change which
name a duplicated value maps back to.
Also fixes push_number, which masked with `255 << pad`. That shifts an int, so
it overflowed past the fourth byte and silently zeroed the high half of every
8-byte value. Caught by an enum whose members use bits 62 and 63.
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
1 participant
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.
Description
Draft, stacked on #434 — review only the last commit. #434 holds the small self-contained wins; this branch is for structural format changes, the ones that reshape a record rather than eliding a field.
main)This commit accounts for −516,948 B.
Current behavior
An interface reference inside a type encoding costs 9 bytes: a 1-byte tag, a 4-byte name pointer, and a 4-byte pointer to its protocol list.
Across the SDK there are 92,844 such references, of which 88,172 carry no protocols at all — and they name only 2,960 distinct classes. So the overwhelmingly common case pays 8 bytes of pointers to say "NSString, no protocols", over and over.
New behavior
A new
InterfaceIndexReferencetag encodes the class as auint16index into a new class-name table: 3 bytes instead of 9. References that genuinely carry protocols keep the existing form, so nothing is lost.The table is written between the module table and the heap. It costs 11,844 bytes (2,960 entries + count), which is already netted out of the figure above.
There is a fallback:
internClassNamereturns false once the table would exceedUINT16_MAX, and the generator emits the old pointer form instead. So a hypothetically enormous SDK degrades in size rather than corrupting.Reader changes
TypeEncodinggains three helpers, and all 16 call sites acrossInterop.mm,ArgConverter.mm,ClassBuilder.mmandFFICall.cppnow go through them:isInterfaceReference()— true for both spellings. This is the important one: a site that keeps comparing againstInterfaceDeclarationReferencealone would silently drop the indexed form into whatever its default branch does, mis-marshalling an argument with no crash to point at it.interfaceName()— resolves the name for either spelling.interfaceProtocols()— returnsnullptrfor the indexed form, which by construction has none.MetaFile::classNames()is resolved once insetInstancerather than per lookup, because locating the table means walking every preceding table and two of the converted sites are on hot marshalling paths.Also here: hardening the empty-array interning from #434
#434 made
push_binaryArrayreuse a single empty array. That is safe in the heap, where arrays are reached by offset, but wrong for the header tables, which the runtime locates positionally by walkingsizeInBytes(). Interning one there would shift every table after it, including the heap pointer.It could not actually trigger in #434 — bucket arrays are never empty (the hashtable floors at 100 buckets) and only one other header array existed. Adding a fifth table makes two-empties reachable, so
push_binaryArraynow takes an explicitshouldIntern, and all five header writes passfalse.Verification
Both files rendered to a canonical, offset-independent form and diffed:
The renderer auto-detects the extra root table (the heap always opens with a 0 marker byte, so a nonzero word in that position is the class-name count) and renders an indexed reference identically to a protocol-less pointer reference — so if the two ever disagreed on the resolved class name, it would show up as a diff rather than being masked.
I also checked the emitted table directly: 2,960 entries, all offsets distinct, first names resolving to
NSString,NSUUID,ARAnchor, …, and the heap marker byte still at offset 0.Build status
The
NativeScripttarget compiles cleanly with these changes — every source file, including all 16 converted call sites. The link step fails in my worktree on-lzip, a prebuilt artifact the worktree does not have; that is environmental and unrelated. The device suite has not been run.Still to come on this branch
__tsEnum(...)→ binary name/value tableswiftNameFields[i]is a literal suffix offullNameFields[i](MetaFactory.cpp:412-418) and both are written. Must preserve insertion order —__tsEnumbuilds a bidirectional map whose reverse entries are last-write-winscount == 1 + colons(selector), 0 mismatches across all 60,589 methods_introduced→ flag bitsIsIosAppExtensionAvailable, which the runtime never reads)ParametrizedCall::Get(FFICall.cpp:279-306) keys itsffi_cifcache on theTypeEncodingpointer alone while callers pass differentinitialParameterIndex/argsCount. Latent only because no two declarations share a blob today; deduping aliases them and silently builds the wrong cif. Re-key the cache firstDeriving method
jsName(~1.42 MB, the single biggest lever) stays deferred pending a decision onMeta::jsName()'s signature — see #434 for the detail. Performance is not the blocker there; the API shape is.Does your pull request have unit tests?
Not yet — draft. This changes reader behavior, so the device suite must run before it leaves draft.