Optimize MetaDataProtoEditor.renameRecordTypes() - #4429
Draft
robert-brunel wants to merge 1 commit into
Draft
Conversation
* Replace the naive O(N²) implementation of `renameRecordTypes()`, which renames the record types one by one via `renameRecordType()`, with an efficient batched version that applies all renames in one go. * Cleanly split the per-rename validation from the actual mutation throughout, so that the whole batch is validated before anything is mutated. * Add a `RecordTypeRenames` data structures that indexes renames by fully qualified name to avoid having to scan all renames per item. This brings what would otherwise still be an O(N²) algorithm down to O(N). * Leave `renameRecordType()` itself untouched for the time being. It will be simplified in a subsequent change by reusing the utility methods introduced here. The batched implementation performs the costly compilation of the `FileDescriptor` exactly _once_ (via `Descriptors.FileDescriptor.buildFrom()`, from the original proto) and applies the name mapping in a single walk of the builder. The trivial implemetation, by contrast, performs a recompilation of the entire records `FileDescriptor` plus a full `records.toBuilder()` deep-copy _in each iteration_ of the loop over the record types. In the common use case where _all_ top-level record types are to be renamed, that means O(N) descriptor compilations for N record types; a factor which the batched implementation brings down to O(1). Applying multiple renames in the batched matter is equivalent applying them one-by-one, _provided that_ the name mapping defined by the given `renamer` is collision-free. The batched implementation validates this mapping upfront. This allows it to provide a stronger exception safety guarantee. If the mapping is invalid, an exception will be thrown before _any_ of the renames is performed. The one-by-one implementation, by contrast, may raise an exception halfway through; and that may depend on the order in which the renames are applied. * Cross-check `renameRecordType()` and `renameRecordTypes()` against each other throughout the existing test suite. * Add dedicated tests for every `MetaDataException` reachable from `renameRecordTypes()`, plus a couple of previously-unexercised edge cases. * Fix an unrelated, pre-existing bug in the `Joined.json` test fixture (a join referenced a nonexistent constituent), uncovered along the way. * Add a performance test that renames a large number of record types. According to a local ad-hoc experiment with 50 record types, the batched implementation measured about 17x faster than an equivalent one-by-one sequence of `renameRecordType()` calls (0.1 ms versus 1.5 ms), which is consistent with the O(N) vs. O(N²) difference between the two.
robert-brunel
force-pushed
the
renameRecordTypes-2
branch
from
August 13, 2026 17:48
65ff0cc to
2267e2d
Compare
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
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.
renameRecordTypes(), which renames the record types one by one viarenameRecordType(), with an efficient batched version that applies all renames in one go.RecordTypeRenamesdata structures that indexes renames by fully qualified name to avoid having to scan all renames per item. This brings what would otherwise still be an O(N²) algorithm down to O(N).renameRecordType()itself untouched for the time being. It will be simplified in a subsequent change by reusing the utility methods introduced here.The batched implementation performs the costly compilation of the
FileDescriptorexactly once (viaDescriptors.FileDescriptor.buildFrom(), from the original proto) and applies the name mapping in a single walk of the builder. The trivial implemetation, by contrast, performs a recompilation of the entire recordsFileDescriptorplus a fullrecords.toBuilder()deep-copy in each iteration of the loop over the record types. In the common use case where all top-level record types are to be renamed, that means O(N) descriptor compilations for N record types; a factor which the batched implementation brings down to O(1).Applying multiple renames in the batched matter is equivalent applying them one-by-one, provided that the name mapping defined by the given
renameris collision-free. The batched implementation validates this mapping upfront. This allows it to provide a stronger exception safety guarantee. If the mapping is invalid, an exception will be thrown before any of the renames is performed. The one-by-one implementation, by contrast, may raise an exception halfway through; and that may depend on the order in which the renames are applied.Testing
renameRecordType()andrenameRecordTypes()against each other throughout the existing test suite.MetaDataExceptionreachable fromrenameRecordTypes(), plus a couple of previously-unexercised edge cases.Joined.jsontest fixture (a join referenced a nonexistent constituent), uncovered along the way.According to a local ad-hoc experiment with 50 record types, the batched implementation measured about 17x faster than an equivalent one-by-one sequence of
renameRecordType()calls (0.1 ms versus 1.5 ms), which is consistent with the O(N) vs. O(N²) difference between the two.