Skip to content

Commit 5cbd14a

Browse files
kalaspuffarclaude
andcommitted
perf: share RowGroup instances across ScenarioData snapshots
RowGroup is effectively immutable — all fourteen fields are declared final and no caller mutates the lists returned by its getters (getRows() even wraps its internal list in Collections.unmodifiableList()). The deep-copy loop in RowGroupSequence that called new RowGroup(rg) for every element was therefore pure waste: for a scenario-heavy sequence it could produce thousands of unnecessary RowGroup objects and four times as many ArrayList allocations per layout pass. Replace the per-element copy loop with new ArrayList<>(template.group), which copies the list container (so independent add() calls on one snapshot do not affect another) while sharing the RowGroup element objects. Add a null guard to match the behaviour of the non-deepMode path. Remove the now-unreachable RowGroup(RowGroup template) copy constructor to prevent future confusion. Note: the blocks list is intentionally still shallow-copied in deepMode. ScenarioData.getBlocks().add() mutates it after snapshots are taken, so sharing it would corrupt saved states (confirmed by three integration test failures during development). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 9983d7b commit 5cbd14a

2 files changed

Lines changed: 4 additions & 28 deletions

File tree

src/org/daisy/dotify/formatter/impl/page/RowGroup.java

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -171,30 +171,6 @@ private RowGroup(Builder builder) {
171171
this.lineProps = builder.lineProps;
172172
}
173173

174-
/**
175-
* Creates a deep copy of the supplied instance.
176-
*
177-
* @param template the instance to copy
178-
*/
179-
RowGroup(RowGroup template) {
180-
this.rows = new ArrayList<>(template.rows);
181-
this.markers = new ArrayList<>(template.markers);
182-
this.identifiers = new ArrayList<>(template.identifiers);
183-
this.breakable = template.breakable;
184-
this.skippable = template.skippable;
185-
this.collapsible = template.collapsible;
186-
this.unitSize = template.unitSize;
187-
this.lastUnitSize = template.lastUnitSize;
188-
this.ids = new ArrayList<>(template.ids);
189-
this.lazyCollapse = template.lazyCollapse;
190-
this.keepWithNextSheets = template.keepWithNextSheets;
191-
this.keepWithPreviousSheets = template.keepWithPreviousSheets;
192-
this.avoidVolumeBreakAfterPriority = template.avoidVolumeBreakAfterPriority;
193-
this.lastInBlock = template.lastInBlock;
194-
this.mergeable = template.mergeable;
195-
this.lineProps = template.lineProps;
196-
}
197-
198174
private static float getRowSpacing(float rowDefault, RowImpl r) {
199175
return (r.getRowSpacing() != null ? r.getRowSpacing() : rowDefault);
200176
}

src/org/daisy/dotify/formatter/impl/page/RowGroupSequence.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,10 @@ public RowGroupSequence(
4949
private RowGroupSequence(RowGroupSequence template, VerticalSpacing vs, int offset, boolean deepMode) {
5050
this.blocks = deepMode ? new ArrayList<>(template.blocks) : template.blocks;
5151
if (deepMode) {
52-
this.group = new ArrayList<>();
53-
for (RowGroup rg : template.group) {
54-
group.add(new RowGroup(rg));
55-
}
52+
// RowGroup is effectively immutable (all fields are final and no caller mutates the
53+
// lists returned by its getters), so the element objects can be shared. A new list
54+
// container is required so that add() calls on one snapshot do not affect another.
55+
this.group = template.group == null ? null : new ArrayList<>(template.group);
5656
} else {
5757
if (template.group == null) {
5858
this.group = null;

0 commit comments

Comments
 (0)