Repository navigation
Adopt EncDotNet 0.8.0 in the S-101 reader, update applicator and S-57 translator - #892
Merged
Merged
Conversation
… translator Part of #890. - Move EncDotNet.Iso8211 and EncDotNet.S57 to 0.8.0. S57FeaturePointer.Name is now an S57LongName, so the translator keys its long-name indexes by S57LongName on both sides. Keeping the (agency, fid, sub) tuple would have converted a record's FIDN from int but a pointer's from uint, so a FIDN above 2^31 would no longer have matched. A record without FOID keys as the all-zero long name, as before. - S101DocumentReader reads records one at a time with Iso8211DocumentReader.EnumerateRecords instead of building the whole ISO 8211 document, and reads repeating fields by ordinal instead of creating a group object per repetition. Repetitions are counted as GetSubfieldGroups counted them, including a final one cut short by the end of the field, and missing subfields default or throw as before; new tests pin both and pass on the previous reader too. - S101UpdateApplicator no longer searches the feature order list on every delete. A deleted RCID stays in the order and is skipped; a feature inserted again after a delete is output only at its new, final position. A randomized test compares the result with the list-based merge. - Test helpers: SyntheticS57Cell edits field data through a writable view of the reader's private copy, and the translator tests build feature pointers as S57LongName. Reading US4MD81M translated to S-101 (3.4 MB) takes 105 ms and allocates 36 MB, down from 238 ms and 156 MB. Deleting a third of its 7,378 features in an update takes 2 ms instead of 13 ms. The 19 S-101 test datasets, and 9 NOAA cells translated, written and read back, give the same model fingerprints as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YagvELyJKBANk61DyFGUNJ
The perf gate overlays the PR's Directory.Packages.props onto the base tree so the overlaid PerfRunner can restore packages the PR adds. That also upgraded packages the base already uses, so the base library code was built against dependency versions it wasn't written for: this PR's move to EncDotNet 0.8.0 made the base build fail, and a compatible upgrade would have been measured on both sides, hiding its effect. Keep the base's version for every package the base declares; only packages new in the PR take the PR's version. Checked by running the step on main with this branch as head: the base PerfRunner builds against EncDotNet 0.7.2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YagvELyJKBANk61DyFGUNJ
The base step now writes the base's package versions into Directory.Packages.props as a local change, and a plain checkout of the PR head carried that change over, so the candidate was built against the base's EncDotNet 0.7.2. Check out the PR head with --force so the candidate builds from the PR tree as committed. Simulated locally: after the overlay and the forced checkout, the tree is clean and on 0.8.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YagvELyJKBANk61DyFGUNJ
Contributor
Performance Gate✅ PASSED — no regressions. Threshold: 10.0%, MAD multiplier (k): 3.0, retry-zone mult: 2.0× Scenario summary
exchange-set-openIteration statistics
Spans (sum of all iterations)
Metrics
s101-pick-warmIteration statistics
s101-portray-coldIteration statistics
Spans (sum of all iterations)
Metrics
s101-portray-warmIteration statistics
Spans (sum of all iterations)
Metrics
s101-real-coldIteration statistics
s101-real-warmIteration statistics
s101-render-repeatIteration statistics
Spans (sum of all iterations)
Metrics
s101-render-warmIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverageIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-openIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-render-largeIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-render-repeatIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-viewport-fitIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-viewport-zoomed-inIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-viewport-zoomed-outIteration statistics
Spans (sum of all iterations)
Metrics
s102-real-warmIteration statistics
s111-arrow-repeatIteration statistics
Spans (sum of all iterations)
Metrics
s111-real-warmIteration statistics
s124-vectorIteration statistics
Spans (sum of all iterations)
Metrics
s201-vectorIteration statistics
Spans (sum of all iterations)
Metrics
Generated by EncDotNet.S100.PerfReport gate command |
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.
Summary
This covers items 1–4 of #890. Item 5, allocation budgets for S100, is left for a follow-up.
1. Move to EncDotNet 0.8.0
EncDotNet.Iso8211andEncDotNet.S57go from 0.7.2 to 0.8.0.S57FeaturePointer.Nameis now anS57LongName, soS57ToS101Translatorkeys its long-name indexes byS57LongNamefor both records and pointers. TheLnam(fp.Name)calls now usefp.Namedirectly.(agency, fid, sub)tuple: the record side converts a signedintFIDN tolong, while the pointer side would convert an unsigneduint. For any FIDN above 2³¹ the two keys would differ, so the pointer would stop matching its target.2–3.
S101DocumentReader: stream records and read by ordinalIso8211DocumentReader.EnumerateRecords, instead of building the whole ISO 8211 document first.GetSubfieldGroups()counted them. That includes a final repetition cut short by the end of the field, whichGroupCountwould leave out.KeyNotFoundException, and PAIX, NARC, ATVL and the update instructions default as the oldtry/catchblocks did.S101DocumentReaderTruncatedFieldTestspin this. They pass on the previous reader, and fail if the loop usesGroupCount.4.
S101UpdateApplicator: no list search on deleteorder.Remove. Its RCID stays in the order and is skipped when the result is built.S101UpdateApplicatorFeatureOrderTestscompares 2,000 random update sequences against the list-based merge. It passes on the previous code, and fails if the reinsert handling is broken.Test helpers
SyntheticS57Celledits a field's bytes through a writable view of the reader's private copy of the file.Iso8211Field.Datais nowReadOnlyMemory<byte>.S57LongName.Perf gate (
.github/workflows/perf.yml)main, the gate buildsmain's library code with this PR's perf tooling overlaid, including the PR's wholeDirectory.Packages.props.mainalready uses. With EncDotNet 0.8.0,main's translator no longer compiled, so the gate failed before measuring anything.main's version for every packagemaindeclares. Only packages new in the PR take the PR's version, which was the overlay's original purpose.mainwith this branch as head: the base PerfRunner builds against EncDotNet 0.7.2.Results
Measured locally in Release, as medians of 9 runs. The S-101 files are NOAA cells (with their updates applied) translated by
S57ToS101Translatorand written withS101DocumentWriter.The output is unchanged. I fingerprinted every property of the resulting
S101Documentbefore any change and again after each step, and all fingerprints match. The inputs were:tests/datasets/S101, opened from a file and from a stream;Spec alignment
s100-framework)s101-enc)s102-bathymetry)s104-water-level)s111-surface-currents)s124-nav-warnings)s129-ukc)Spec section references cited in code/docs:
None new. ISO 8211 reads still go through
EncDotNet.Iso8211(s101-encchecklist item 1).Tests
tests/:S101DocumentReaderTruncatedFieldTestsandS101UpdateApplicatorFeatureOrderTests.Assert.SkipUnless/Assert.SkipWhen. The new tests need no real data.dotnet test --configuration Releasepasses locally: everything passes except the 5 tests below.EncDotNet.S100.Cli.Tests(CliVersionAndUpdateTests×4 andSkillOutputTests.RootHelpDocumentsSkillOption) fail in my sandbox. They fail identically onmain, so this branch doesn't cause them.S100MapSessionTests.AddDatasetAsyncFailsWhenRemoveCancelsRenderBeforeOwnershipIsReleasedfailed once during the full run, while another build was competing for the machine. It passes when re-run, both on this branch and onmain.Documentation
src/<project>/README.md. Not needed: no public API or behaviour change.docs/. Not needed, for the same reason.Dependencies
Directory.Packages.props. Two existing package versions are bumped there, and nothing is added.gh-advisory-databasesecurity check run for any new dependency. Not applicable: no new dependencies.Breaking changes
None in EncDotNet.S100's public API. EncDotNet 0.8.0's own breaking changes, listed in philliphoff/EncDotNet#34 and #35, are absorbed here.
Closes #890 except item 5.
🤖 Generated with Claude Code
https://claude.ai/code/session_01YagvELyJKBANk61DyFGUNJ