Use an append-only buffer for the additions in the piecetable.

[johanvos]

This PR introduces an append-only buffer to be used in the piecetable. This buffer caches the lengths of units which are never going to be removed.
This fixes #421

[Copilot]

Pull request overview

This draft PR introduces an append-only buffer optimization for the PieceTable's addition buffer, replacing the standard UnitBuffer with a new AppendOnlyUnitBuffer class that uses a cumulative length array for O(log n) binary search lookups instead of O(n) linear iteration.

Key changes:

• Implements AppendOnlyUnitBuffer with a fixed 4K element capacity using binary search for improved lookup performance
• Changes visibility of UnitBuffer.unitList and UnitBuffer.dirty from private to package-private to enable subclass access
• Replaces the mutable UnitBuffer with the append-only variant in PieceTable.additionBuffer

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
AppendOnlyUnitBuffer.java	New class extending UnitBuffer with optimized binary search-based lookups using a cumulative length array
UnitBuffer.java	Relaxed visibility of unitList and dirty fields to package-private for subclass access
PieceTable.java	Changed additionBuffer type from UnitBuffer to AppendOnlyUnitBuffer and made it final

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The new AppendOnlyUnitBuffer class lacks test coverage. Given that there are comprehensive tests for PieceTable and UnitBuffer in the test directory, this new class should also have tests to verify the correctness of its append methods, getUnitWithRange binary search logic, and ensureCapacity growth behavior.

[Copilot]

The getUnitWithRange method does not use the 'end' parameter at all and only performs a lookup based on 'start'. This differs from the parent class implementation which checks if the range [start, end] fits within a unit. The current implementation will return the unit at the start position regardless of whether the entire range fits within that unit.

Suggested change

	if (start < 0 || unitList.isEmpty()) return new TextUnit("");
	int index = Arrays.binarySearch(unitLengths, 0, unitList.size(), start);
	if (index >=0) { // exact start at index
	index = index+1;
	} else { // no exact item found
	index = -(index) -1;
	}
	return unitList.get(index);
	if (start < 0 || end <= start || unitList.isEmpty()) return new TextUnit("");
	int index = Arrays.binarySearch(unitLengths, 0, unitList.size(), start);
	if (index >= 0) { // exact start at index
	index = index + 1;
	} else { // no exact item found
	index = -(index) - 1;
	}
	// Now, index is the first unit whose end offset is > start
	if (index >= unitList.size()) return new TextUnit("");
	int unitStart = (index == 0) ? 0 : unitLengths[index - 1];
	int unitEnd = unitLengths[index];
	if (start >= unitStart && end <= unitEnd) {
	return unitList.get(index);
	} else {
	return new TextUnit("");
	}

[Copilot]

Missing class-level documentation. The comment is empty and should describe the purpose of this class, particularly explaining that it uses a cumulative length array for optimized lookups and any limitations (such as the hard limit on 4K elements mentioned in the PR description).

Suggested change

	*
	* Append-only buffer used as the PieceTable Addition Buffer
	* An append-only buffer used as the PieceTable Addition Buffer.
	* <p>
	* This class maintains a cumulative length array ({@code unitLengths}) to enable
	* efficient lookup of units by character offset. The cumulative array allows
	* for optimized binary search operations when retrieving units that span a given range.
	* <p>
	* <b>Limitations:</b> This buffer has a hard limit of 4096 elements (4K units),
	* as defined by the initial size of the {@code unitLengths} array. Exceeding this
	* limit will require resizing, but performance and memory usage are optimized for
	* use cases within this bound.

[Copilot]

Trailing whitespace on this otherwise empty line. This can cause issues with some linters and version control systems.

Suggested change

[johanvos]

This PR improves FPS (locally) from 29FPS to 34FPS, and reduces average processing+rendering time from 24ms to 20ms.

[eugener]

The issues from the first glance I see in AppendOnyUnitBuffer:

• remove should throw also
• main ( probably most critical one) bottlelneck in length is still there , IMO should be something like:

@Override
  public int length() {
      return unitList.isEmpty() ? 0 : unitLengths[unitList.size() - 1];
  }

• Another good option is to not use inheritance but composition, i.e. own the list
  Composition based design could be cleaner and even more performant.

  public class AppendOnlyUnitBuffer {
  
    private final List<Unit> unitList = new ArrayList<>();
    private int[] unitLengths = new int[4096];
    private final StringBuilder internalText = new StringBuilder();
  
    public void append(Unit unit) {
        ensureCapacity(unitList.size() + 1);
        int idx = unitList.size();
        int oldLength = idx == 0 ? 0 : unitLengths[idx - 1];
        unitLengths[idx] = oldLength + unit.length();
        unitList.add(unit);
        internalText.append(unit.getInternalText());
    }
  
    public void append(List<Unit> units) {
        ensureCapacity(unitList.size() + units.size());
        for (Unit u : units) {
            append(u);
        }
    }
  
    public int length() {
        return unitList.isEmpty() ? 0 : unitLengths[unitList.size() - 1];
    }
  
    public String getText() {
        StringBuilder sb = new StringBuilder();
        unitList.forEach(unit -> sb.append(unit.getText()));
        return sb.toString();
    }
  
   // O(1) via StringBuilder - even better performance
    public String getInternalText() {
        return internalText.toString();
    }
  
    public List<Unit> getUnitList() {
        return unitList;
    }
  
    public boolean isEmpty() {
        return unitList.isEmpty();
    }
  
    public Unit getUnitWithRange(int start, int end) {
        if (start < 0 || unitList.isEmpty()) return new TextUnit("");
        int index = Arrays.binarySearch(unitLengths, 0, unitList.size(), start);
        if (index >= 0) {
            index = index + 1;
        } else {
            index = -(index) - 1;
        }
        if (index >= unitList.size()) return new TextUnit("");
        return unitList.get(index);
    }
  
    private void ensureCapacity(int required) {
        if (required > unitLengths.length) {
            int newCapacity = Math.max(unitLengths.length * 2, required);
            unitLengths = Arrays.copyOf(unitLengths, newCapacity);
        }
    }
  
    @Override
    public String toString() {
        return "AppendOnlyUnitBuffer{" + unitList + "}";
    }

}

[eugener]

I also think that it could be a good idea to replace linear search in charAt in PieceTable with Arrays.binarySearch, since it is called for every character.

[johanvos]

The issues from the first glance I see in AppendOnyUnitBuffer:

• remove should throw also

True, will do that

• main ( probably most critical one) bottlelneck in length is still there , IMO should be something like:

@Override
  public int length() {
      return unitList.isEmpty() ? 0 : unitLengths[unitList.size() - 1];
  }

It's not a bottleneck, as this is invoked O(n) while the getUnitWithRange (which is the real bottleneck) is invoked O(n^2). Nevertheless, since we do have the length handy in the appendOnlyBuffer, it is best to use that indeed.

• Another good option is to not use inheritance but composition, i.e. own the list

That is another option indeed, but I'd like to defer that to some architectural discussion. I'm not sure we really need both a UnitBuffer and an AppendOnlyUnitBuffer.