perf(buffer)!: filled moves the cell to be filled

[EdJoPaTo]

This removes a clone from inside the method allowing the user to do it explicitly when needing.

BREAKING CHANGE: Buffer::filled moves the cell instead of taking a reference.

-Buffer::filled(area, &Cell::new("X"));
+Buffer::filled(area, Cell::new("X"));

---

https://github.com/ratatui-org/ratatui/pull/1143#issuecomment-2131247181

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.2%. Comparing base (8b447ec) to head (e38bd57). Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1148 +/- ## ===================================== Coverage 94.2% 94.2% ===================================== Files 60 60 Lines 14511 14511 ===================================== Hits 13672 13672 Misses 839 839 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[orhun]

Q: Isn't taking a reference better in terms of performance? (no copy & clone?)

[EdJoPaTo]

Depends on the case. Moving is always the simplest solution with the best performance. (= moving the ownership, the data stays exactly the same, nothing needs to be done at runtime with the data, no (64-bit) reference pointer needs to be created). When the data is only read it depends on the size of the data (u16 for example is smaller than a pointer on a 64-bit system → its more efficient to copy then move the u16 instead of using a 64 bit reference. Which is why it's often a performance improvement to #[derive(Copy)] enum and moving them rather than taking a reference of them). When the data is used (written to or whatever) and also required after the method then it needs to be cloned at some point. So either take a reference and clone it inside the method or clone it outside the method. Cloning it outside the method allows the user to make that choice depending on their use case. Cloning it inside the method forces the user with an additional clone / drop even when it might not be required like in this case.

Buffer::filled(area, &Cell::new()) results basically in this code:

let cell = Cell::new();
Buffer::filled(area, &cell)
drop(cell); // Rust silently does this for everything not moved away at the end of the context

In this case we can save the drop and one clone as we can move in the cell directly.

This is not very significant on a Cell as a cell is rather small. With stuff like String or Vec this can get rather expensive.

Basically… when the method takes a reference and clones it directly, then it could just take the value directly and allow the caller to decide whether cloning or moving is the better choice.

Fun fact: Into can hide situations like this resulting in additional clones.

fn something<S: Into<String>>(input: S) { … }

let huge_string: String = …;
something(&huge_string); // into silently clones in this case
drop(huge_string); // Rust silently does this at the end of the context

Changing the reference to a move can be a performance improvement (and did for me in the past) which is why I avoid Into which is not a pure into and only a to. Especially as .into() might change to a different implementation silently on a refactoring, and it's quite annoying to find the actually used implementation making sure it doesn't do something performance critical.

[EdJoPaTo]

Probably off-topic to this PR, any idea what the effect of calling Buffer::filled with a double width string should be / actually is?

Likely gibberish, but that's hard to detect as cells with width > 1 are possible and allowed (emojis, …). The ones after the width > 1 cells are skipped I think?

It might be an idea to Buffer::filled(area, symbol) directly and set Cell private. The only use case of filled is probably testing. No need to expose internals for that when not needed. And prevents changes to this more internal stuff as it ends up being breaking fast.

[joshka]

Yeah, I think it might be ok, AAAA AA -> A A A . Every second cell is hidden, but will display correctly regardless I think.