Fix insertLines and deleteLines
#2167
Conversation
Doing this makes it possible to appropriately handle a failed cloneFrom operation by adjusting the correct part of the page capacity accordingly
Handle `clonePartialRowFrom` errors in `insertLines` and `deleteLines` by adjusting page capacity. To do this, I've rewritten both functions with a new way of iterating rows by moving a tracked pin up/down. Benchmarks seem to indicate that this has no effect on performance.
| @@ -433,7 +433,7 @@ pub fn clonePool( | |||
| /// Adjust the capacity of a page within the pagelist of this screen. | |||
| /// This handles some accounting if the page being modified is the | |||
| /// cursor page. | |||
| fn adjustCapacity( | |||
| pub fn adjustCapacity( | |||
There was a problem hiding this comment.
This is a code smell you already noted. This is a good hint that we should be moving the insertLines/deleteLines logic into Screen. We can do that later, just agreeing with you on the future. :)
|
|
||
| // Clone the link. | ||
| const dst_link = other_link.dupe(other, self) catch |e| { | ||
| comptime assert(@TypeOf(e) == error{OutOfMemory}); |
There was a problem hiding this comment.
Clever, I created ziglang/zig#21267 because of this.
src/terminal/Terminal.zig
Outdated
| self.screen.cursor.page_pin.down(rem - 1).?, | ||
| ) catch |err| { | ||
| std.log.err("TODO: insertLines handle trackPin error err={}", .{err}); | ||
| @panic("TODO: insertLines handle trackPin error"); |
There was a problem hiding this comment.
I think we can crash here. This means that our general purpose allocator is OOM which means our system is OOM and we're in a really bad spot because of it. I'd love to handle OOM situations more gracefully (probably change the screen to an error state) but for now crashing is fine. I'm going to change the message since its not a big TODO item.
src/terminal/Terminal.zig
Outdated
| }, | ||
| ) catch |e| { | ||
| std.log.err("TODO: insertLines handle adjustCapacity error err={}", .{e}); | ||
| @panic("TODO: insertLines handle adjustCapacity error"); |
There was a problem hiding this comment.
I traced down and added explicit error sets all the way up to this point. This has led me to question... the current error set for Screen.adjustCapacity is effectively Allocator.Error || Page.CloneFromError. The latter error set has all the GraphemeMapOOM etc etc errors.
My question is: is it reasonable or even possible at this point because all we're doing is adjusting capacity upwards? My thinking atm is to make that a panic since there's probably no performance benefit to making it unreachable but from a comment perspective I'm going to say it should be impossible. Just want to check my assumption.
The Allocator.Error is fully possible and that's really not recoverable since that is from the GPA.
|
See some of my updated error handling. It doesn't change any behavior so if I get to the point of merge I'll do it without a response, but I want to make sure my understanding and comments are correct. |
|
Since this doesn't fail any previous tests and should improve stability significantly I'm going to merge this. Its very hard to write tests w/o verifying they failed prior to the change in cases like this so I think I'll have to wait for a bug. |
|
❤️ |
Changes
insertLinesanddeleteLinesto handle adjusting page capacity when it's exceeded while moving rows. This can happen when scrolling lines with managed memory in to a different page which is already full of the given type of memory (most crashes that hit this seem to occur with hyperlinks, but it's theoretically possible with graphemes as well, and possibly even styles).Benchmarks seem to show no performance degradation from the rewrite.
insertLinesanddeleteLinesshare like 99% of their functionality, and really should be abstracted to a shared function, probably onScreennotTerminal. I've left aTODOfor this.In order to do this I had to discriminate the source of the error from
clonePartialRowFrom, and while doing that I did also fix a couple small bugs which were based on the assumption that it would only ever be used when cloning to a new, empty page with sufficient capacity.I didn't make any unit tests to cover the improved functionality, but here's a list of tests that could/should be added:
clonePartialRowFromerrors should be tested, and it should be made sure they leave the page in a valid state.insertLinesanddeleteLineswhich move managed memory of all types across page boundaries in to a page whose managed memory for that type is full, requiring a capacity adjustment.Future work:
insertLinesanddeleteLinesfunctionality in single method onScreen.movePartialRowFromfunction where the src row is allowed to be modified, which allows for fast paths that can reassigned managed memory within the same page; and use this instead ofclonePartialRowFromin theinsertLines/deleteLineslogic.Screen.clonethat doesn't usecloneFrom, since there are assumptions that can be made (new pages with exact capacity) and optimizations (straightmemcpying rows and managed memory sections) which should make it much faster.