Repository navigation
perf(parser): defer segment field parsing until first access - #40
shadowhand wants to merge 2 commits into
Conversation
Keep each segment's raw line until a field is written so unread messages stay small and a read-only round trip is byte-exact.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The Robin Hood of code review
Heads up: this is a point-in-time review. Push fixes freely, then comment
/robinwhenever you want another pass.
Summary
The change adds lazy segment hydration and preserves untouched wire lines, with useful coverage for read-only round trips and mutation tracking. Two edge cases remain: recovery no longer preserves custom Segment implementations that do not extend AbstractSegment, and a failed lazy hydration can discard the original wire line.
A caught parse error must leave the bound line in place and the segment unhydrated so a later read can try again.
|
There was a problem hiding this comment.
The Robin Hood of code review
Heads up: this is a point-in-time review. Push fixes freely, then comment
/robinwhenever you want another pass.
Summary
The change adds lazy segment hydration and preserves untouched wire lines, with focused tests covering round trips, reads, and writes. Two compatibility gaps remain: mutable custom Type implementations can leave the raw line stale, and out-of-order recovery no longer preserves custom segments that do not extend AbstractSegment.
|
There was a problem hiding this comment.
The Robin Hood of code review
Heads up: this is a point-in-time review. Push fixes freely, then comment
/robinwhenever you want another pass.
Summary
The change adds lazy segment hydration and mutation tracking to preserve untouched wire lines, with focused tests for round trips, delimiter changes, and writes. Two compatibility regressions remain: out-of-order recovery no longer preserves custom Segment implementations, and parsing now bypasses existing AbstractSegment subclasses that override the public parse method.
| // A group lead out of order is unsafe to re-consume, so only segments recover typed. | ||
| if ($structure instanceof Segment) { | ||
| $structure->parse($encoding, $element->raw); | ||
| if ($structure instanceof AbstractSegment) { |
There was a problem hiding this comment.
Robin —
Out-of-order recovery previously retained any structure implementing Segment as its declared type. Restricting this to AbstractSegment sends custom Segment implementations through the GenericSegment fallback, losing their typed behavior and schema when a declared segment appears out of order.
Recommendation: Preserve the Segment interface behavior during recovery; use the segment's parse method for implementations that cannot bind a raw line, or provide a compatible lazy-binding contract.
| ); | ||
|
|
||
| $this->parse($encoding, $element->raw); | ||
| $this->bind($encoding, $element->raw); |
There was a problem hiding this comment.
Robin —
parseSegments previously called the virtual parse() method. Binding instead means that lazy hydration calls populate(), so existing AbstractSegment subclasses that override the public parse() method are silently bypassed when parsed through a message or group.
Recommendation: Provide a migration-compatible hydration hook for custom parsing behavior, or otherwise ensure subclasses overriding parse() continue to participate in the parseSegments path.
|
Superseded: SV-77 is being redesigned around field-level lazy parsing with comparison-based change detection instead of a dirty flag. |
1 participant
Problem
MessageFactory::parse()eagerly parses every field of every segment, whether or not the caller reads it. After SV-76 the remaining cost is field hydration: on a 7.9 KBADT_A01with 100NK1,parse()with nothing read was about 5 ms and retained about 3.3 MB for 7.9 KB of input. Serializing an unread message then re-encoded that whole graph.Change
Segments bind the raw line and the parse-time delimiters instead of parsing fields up front.
getField(),getFieldRepetition(),getLength(), andserialize()hydrate on first use. Schema queries (getNames(),getFieldCount(),isRequired(),maxRepetitions()) still read only the definition table.serialize()emits the bound line verbatim when no field has been written and the target delimiters match. The line ending is not part of a segment line, so changing it only changes the join. A write (setValue(),clear(), compositeparse(), orVaries::setData()) drops that segment's line; only the touched segment is re-encoded, and trailing empty fields are still trimmed. Hand-built segments have no raw line and serialize as before.debug()reads one field before counting, so an undeclared segment still shows its wire fields. The routingMSHinMessageFactoryis still parsed immediately, because the message type is needed to choose the message class.Verification
vendor/bin/phpunit— 852 tests, 2663 assertions, coverage 100%.mago format --check,mago lint, andmago analyzereport no issues.Refs SV-77