Skip to content

Commit

Permalink
Don't store terminal snapshot as a ByteBuf
Browse files Browse the repository at this point in the history 
There's too many issues here around object pooling and reference
counting, that it's just not practical to correctly handle. We now just
use a plain old byte[] rather than a ByteBuf.

This does mean the array now lives on the Java heap rather than the
direct heap, but I *think* that's fine. It's something that is hard to
measure.

Fixes #2059
  • Loading branch information
@SquidDev
SquidDev committed Jan 12, 2025
1 parent 3c46b8a commit 22e6c06
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 79 deletions.
2 changes: 1 addition & 1 deletion projects/common/src/main/java/dan200/computercraft/shared/computer/core/ServerComputer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ protected void onTerminalChanged() {
}

public TerminalState getTerminalState() {
return new TerminalState(terminal);
return TerminalState.create(terminal);
}

public void keepAlive() {
Expand Down
49 changes: 25 additions & 24 deletions ...common/src/main/java/dan200/computercraft/shared/computer/terminal/NetworkedTerminal.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
import dan200.computercraft.core.terminal.Terminal;
import dan200.computercraft.core.util.Colour;
import net.minecraft.nbt.CompoundTag;
import net.minecraft.network.FriendlyByteBuf;

public class NetworkedTerminal extends Terminal {
public NetworkedTerminal(int width, int height, boolean colour) {
Expand All @@ -19,59 +18,61 @@ public NetworkedTerminal(int width, int height, boolean colour, Runnable changed
super(width, height, colour, changedCallback);
}

public synchronized void write(FriendlyByteBuf buffer) {
buffer.writeInt(cursorX);
buffer.writeInt(cursorY);
buffer.writeBoolean(cursorBlink);
buffer.writeByte(cursorBackgroundColour << 4 | cursorColour);
synchronized TerminalState write() {
var contents = new byte[width * height * 2 + Palette.PALETTE_SIZE * 3];
var idx = 0;

for (var y = 0; y < height; y++) {
var text = this.text[y];
var textColour = this.textColour[y];
var backColour = backgroundColour[y];

for (var x = 0; x < width; x++) buffer.writeByte(text.charAt(x) & 0xFF);
for (var x = 0; x < width; x++) contents[idx++] = (byte) (text.charAt(x) & 0xFF);
for (var x = 0; x < width; x++) {
buffer.writeByte(getColour(
backColour.charAt(x), Colour.BLACK) << 4 |
getColour(textColour.charAt(x), Colour.WHITE)
);
contents[idx++] = (byte) (getColour(backColour.charAt(x), Colour.BLACK) << 4 | getColour(textColour.charAt(x), Colour.WHITE));
}
}

for (var i = 0; i < Palette.PALETTE_SIZE; i++) {
for (var channel : palette.getColour(i)) buffer.writeByte((int) (channel * 0xFF) & 0xFF);
for (var channel : palette.getColour(i)) contents[idx++] = (byte) ((int) (channel * 0xFF) & 0xFF);
}

assert idx == contents.length;
return new TerminalState(colour, width, height, cursorX, cursorY, cursorBlink, cursorColour, cursorBackgroundColour, contents);
}

public synchronized void read(FriendlyByteBuf buffer) {
cursorX = buffer.readInt();
cursorY = buffer.readInt();
cursorBlink = buffer.readBoolean();
synchronized void read(TerminalState state) {
resize(state.width, state.height);
cursorX = state.cursorX;
cursorY = state.cursorY;
cursorBlink = state.cursorBlink;

var cursorColour = buffer.readByte();
cursorBackgroundColour = (cursorColour >> 4) & 0xF;
this.cursorColour = cursorColour & 0xF;
cursorBackgroundColour = state.cursorBgColour;
this.cursorColour = state.cursorFgColour;

var contents = state.contents;
var idx = 0;
for (var y = 0; y < height; y++) {
var text = this.text[y];
var textColour = this.textColour[y];
var backColour = backgroundColour[y];

for (var x = 0; x < width; x++) text.setChar(x, (char) (buffer.readByte() & 0xFF));
for (var x = 0; x < width; x++) text.setChar(x, (char) (contents[idx++] & 0xFF));
for (var x = 0; x < width; x++) {
var colour = buffer.readByte();
var colour = contents[idx++];
backColour.setChar(x, BASE_16.charAt((colour >> 4) & 0xF));
textColour.setChar(x, BASE_16.charAt(colour & 0xF));
}
}

for (var i = 0; i < Palette.PALETTE_SIZE; i++) {
var r = (buffer.readByte() & 0xFF) / 255.0;
var g = (buffer.readByte() & 0xFF) / 255.0;
var b = (buffer.readByte() & 0xFF) / 255.0;
var r = (contents[idx++] & 0xFF) / 255.0;
var g = (contents[idx++] & 0xFF) / 255.0;
var b = (contents[idx++] & 0xFF) / 255.0;
palette.setColour(i, r, g, b);
}

assert idx == contents.length;
setChanged();
}

Expand Down
59 changes: 38 additions & 21 deletions ...cts/common/src/main/java/dan200/computercraft/shared/computer/terminal/TerminalState.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

package dan200.computercraft.shared.computer.terminal;

import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import net.minecraft.network.FriendlyByteBuf;
import org.jetbrains.annotations.Contract;

Expand All @@ -20,53 +18,72 @@
*/
public class TerminalState {
private final boolean colour;
private final int width;
private final int height;
private final ByteBuf buffer;
final int width;
final int height;
final int cursorX;
final int cursorY;
final boolean cursorBlink;
final int cursorBgColour;
final int cursorFgColour;
final byte[] contents;

public TerminalState(NetworkedTerminal terminal) {
colour = terminal.isColour();
width = terminal.getWidth();
height = terminal.getHeight();

var buf = buffer = Unpooled.buffer();
terminal.write(new FriendlyByteBuf(buf));
TerminalState(
boolean colour, int width, int height, int cursorX, int cursorY, boolean cursorBlink, int cursorFgColour, int cursorBgColour, byte[] contents
) {
this.colour = colour;
this.width = width;
this.height = height;
this.cursorX = cursorX;
this.cursorY = cursorY;
this.cursorBlink = cursorBlink;
this.cursorFgColour = cursorFgColour;
this.cursorBgColour = cursorBgColour;
this.contents = contents;
}

@Contract("null -> null; !null -> !null")
public static @Nullable TerminalState create(@Nullable NetworkedTerminal terminal) {
return terminal == null ? null : new TerminalState(terminal);
return terminal == null ? null : terminal.write();
}

public TerminalState(FriendlyByteBuf buf) {
colour = buf.readBoolean();
width = buf.readVarInt();
height = buf.readVarInt();
cursorX = buf.readVarInt();
cursorY = buf.readVarInt();
cursorBlink = buf.readBoolean();

var cursorColour = buf.readByte();
this.cursorBgColour = (cursorColour >> 4) & 0xF;
this.cursorFgColour = cursorColour & 0xF;

var length = buf.readVarInt();
buffer = buf.readBytes(length);
contents = buf.readByteArray();
}

public void write(FriendlyByteBuf buf) {
buf.writeBoolean(colour);
buf.writeVarInt(width);
buf.writeVarInt(height);
buf.writeVarInt(buffer.readableBytes());
buf.writeBytes(buffer, buffer.readerIndex(), buffer.readableBytes());
buf.writeVarInt(cursorX);
buf.writeVarInt(cursorY);
buf.writeBoolean(cursorBlink);
buf.writeByte(cursorBgColour << 4 | cursorFgColour);

buf.writeByteArray(contents);
}

public int size() {
return buffer.readableBytes();
return contents.length;
}

public void apply(NetworkedTerminal terminal) {
terminal.resize(width, height);
terminal.read(new FriendlyByteBuf(buffer));
terminal.read(this);
}

public NetworkedTerminal create() {
var terminal = new NetworkedTerminal(width, height, colour);
terminal.read(new FriendlyByteBuf(buffer));
terminal.read(this);
return terminal;
}
}
32 changes: 0 additions & 32 deletions ...on/src/test/java/dan200/computercraft/shared/computer/terminal/NetworkedTerminalTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
import dan200.computercraft.api.lua.LuaValues;
import dan200.computercraft.core.terminal.Terminal;
import dan200.computercraft.test.core.CallCounter;
import io.netty.buffer.Unpooled;
import net.minecraft.nbt.CompoundTag;
import net.minecraft.network.FriendlyByteBuf;
import org.junit.jupiter.api.Test;

import static dan200.computercraft.test.core.terminal.TerminalMatchers.*;
Expand All @@ -18,36 +16,6 @@
import static org.junit.jupiter.api.Assertions.assertEquals;

class NetworkedTerminalTest {
@Test
void testPacketBufferRoundtrip() {
var writeTerminal = new NetworkedTerminal(2, 1, true);

blit(writeTerminal, "hi", "11", "ee");
writeTerminal.setCursorPos(2, 5);
writeTerminal.setTextColour(3);
writeTerminal.setBackgroundColour(5);

var packetBuffer = new FriendlyByteBuf(Unpooled.buffer());
writeTerminal.write(packetBuffer);

var callCounter = new CallCounter();
var readTerminal = new NetworkedTerminal(2, 1, true, callCounter);
packetBuffer.writeBytes(packetBuffer);
readTerminal.read(packetBuffer);

assertThat(readTerminal, allOf(
textMatches(new String[]{ "hi", }),
textColourMatches(new String[]{ "11", }),
backgroundColourMatches(new String[]{ "ee", })
));

assertEquals(2, readTerminal.getCursorX());
assertEquals(5, readTerminal.getCursorY());
assertEquals(3, readTerminal.getTextColour());
assertEquals(5, readTerminal.getBackgroundColour());
callCounter.assertCalledTimes(1);
}

@Test
void testNbtRoundtrip() {
var writeTerminal = new NetworkedTerminal(10, 5, true);
Expand Down
10 changes: 9 additions & 1 deletion ...common/src/test/java/dan200/computercraft/shared/computer/terminal/TerminalStateTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public void testRoundTrip() {
var terminal = randomTerminal();

var buffer = new FriendlyByteBuf(Unpooled.directBuffer());
new TerminalState(terminal).write(buffer);
TerminalState.create(terminal).write(buffer);

checkEqual(terminal, read(buffer));
assertEquals(0, buffer.readableBytes());
Expand All @@ -37,6 +37,10 @@ private static NetworkedTerminal randomTerminal() {
for (var x = 0; x < buffer.length(); x++) buffer.setChar(x, (char) (random.nextInt(26) + 65));
}

terminal.setTextColour(random.nextInt(16));
terminal.setBackgroundColour(random.nextInt(16));
terminal.setCursorPos(random.nextInt(terminal.getWidth()), random.nextInt(terminal.getHeight()));

return terminal;
}

Expand All @@ -46,6 +50,10 @@ private static void checkEqual(Terminal expected, Terminal actual) {
assertEquals(expected.isColour(), actual.isColour(), "isColour must match");
assertEquals(expected.getHeight(), actual.getHeight(), "Heights must match");
assertEquals(expected.getWidth(), actual.getWidth(), "Widths must match");
assertEquals(expected.getTextColour(), actual.getTextColour(), "Text colours must match");
assertEquals(expected.getBackgroundColour(), actual.getBackgroundColour(), "Background colours must match");
assertEquals(expected.getCursorX(), actual.getCursorX(), "Cursor x must match");
assertEquals(expected.getCursorY(), actual.getCursorY(), "Cursor y must match");

for (var y = 0; y < expected.getHeight(); y++) {
assertEquals(expected.getLine(y).toString(), actual.getLine(y).toString());
Expand Down

0 comments on commit 22e6c06

Please sign in to comment.