Thickness should not be a reference type

[dodexahedron]

Thickness shouldn't be a class/reference type.

TL;DR at the bottom. The majority of everything between here and that is technical backing of that assertion, in my characteristically extremely terse style. (Do I need to provide a /s?)

First off, it's still good stuff and I support its existence and the fact that it should be a familiar concept for devs coming to Terminal.Gui from WPF.

The TL;WDYTWTRT¹ Version:

Expand if you're bored

But it has unfortunate and disproportionate performance impact just from being a class and not a struct.

Note that System.Windows.Thickness is a struct, even though drawing in a Windows desktop app involves even hotter access to this kind of data than a Terminal.Gui app typically would, by a pretty wide margin, so that alone should probably be enough to justify it without what follows:

For one thing, nearly everything that class does is a value semantic, including the additions made to it in #3295.

So, it almost definitely should be a struct or (more preferably) a record struct, unless there's a really good reason I'm unaware of for why it isn't.

As a minor design comment, the whole type essentially wraps a Rectangle-like structure, but with the slight behavioral differences necessary to work its magic as intended. Thus, it could easily be implemented exactly like that - having a private Rectangle for its underlying value, but exposing things as it currently does (and not exposing that Rectangle at all).

But the main problem is that it lives on the heap.

Every operation involving it means we can't do that operation entirely on the stack and have run to the heap (which means methods involving it miss out on some optimization possibilities), and any time a new one is created, even ephemerally, it's another heap allocation, at minimum. In addition to all that, it also means this class is responsible for generating a lot of heap garbage for the GC (which also means updating references on containing objects, on top of anything else).

Any time it exists anywhere in code, access to it automatically requires indirection to the heap, extremely likely (always, for practical purposes), meaning you lose a supremely important thing: cache locality. And then, that compounds with the fact that not a single View is going to be sequentially allocated on the heap, either, meaning truly random access.

If an operation can stay entirely on the stack, you significantly increase cache locality and might not even leave L1 cache. And that's major. Even "resorting" to L2 is typically at least 4x slower than L1, and is typically over two orders of magnitude faster than a single trip to main memory, which use of heap objects greatly increases the chance of, along with a whole bunch of other environment-/platform- dependent realities. And use of a value type, where prudent, at least mitigates most of them.

Even a certain amount of unnecessary copy activity on the stack from sub-optimal use of a value type can pretty easily outperform heap access to an otherwise identical reference type, due to this and various other language, compiler, runtime, and hardware realities.

A real bear of it all that also compounds with all that cache stuff is that reference types are not collocated with their enclosing types. An object containing a Thickness field (either explicit or implicit via an auto-property) is already a pointer to a heap object, and then any reference members of that heap object are themselves pointers to other heap objects. Value types are stored in-line with their enclosing type.

If you want to access a Thickness property of a View, you have the dereference of the View, at one point on the heap, and then the dereference of the Thickness, elsewhere on the heap, before you can even interact with it. And the GC can and will move things around, when it feels like it, and when the heap has become fragmented (which this type contributes to).

And mutability of it is inconsistent. With some operations, you might not even be getting that object back, but a newly-constructed (and therefore heap-allocated) object (an immutable behavior). That new object has to have its values copied in, at some point.

Even if you passed an equivalent struct by value, that simple fact alone already means one copy is paid for at the CIL level, and likely 20 or more as well at the machine level, on top of that (meaning multiplicative cost), and that's just the literal copying of the values, assuming it all happens atomically (which isn't even remotely guaranteed). Consider a simple example for conceptual proof: Why is boxing a value type so expensive that analyzers warn you to help you avoid it and why is it one of the things always mentioned in dotnet performance tuning articles? The main reason is all the above, implicitly started from the first HeapAlloc (or equivalent platform calls on non-Windows platforms) and just getting worse from there.

AND, as a concurrency concern, that object (which, again, is expected to have reference, not value, semantics) will have field values (which are currently public instance fields, which is also a no-no for reference types) not equal to the values of the object referenced by the containing object, but without static analysis of that situation being possible. Classic race conditions are the basic exposed issue there, with of course undefined effects possible.

And more, but I realized this is probably documented and I can just dump a link.... And yep! At least the c# and dotnet reasons are, anyway (the cache stuff is just something to be aware of because it is important - especially in hot code.

So, with the disclaimer that is hasn't been kept fully up-to-date with the language and the runtime, see https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/choosing-between-class-and-struct .

For how this type is designed, how it behaves, and how it is used and expected to be used, even that doc - which is almost old enough to drive in the US - very strongly supports this being a value type. Plus, some minor tweaks to it and a small number of places it is used (such as passing by reference when relevant) make the case even stronger. But it still likely would perform better even without those changes. And that's even if it stays 4 integers, meaning it's still half the size of the suggested max struct size, which is 4 times native pointer size (so 32 bytes on a 64-bit platform or 16 bytes on a 32-bit platform - Thickness is 16 bytes). The size recommendation is actually the main "outdated" part of that document... But the underlying values are the most minor part of the whole thing (aside from the fields being public).

I think I even mentioned on at least one other tangentially-related issue someone else was working on, a month or two ago, how I had performed the conversion to record struct with very little other work needed, didn't I? Or maybe it was commentary on one of my own? 😅 I do know I've at least started it before stopping myself more than once in the past 3 or 4 weeks (including the one I'm actively working on right now), mostly due to both high conflict probability and being way out of scope for existing issues (except maybe the general cleanup issues, but this is too important/has too broad an impact to go there).

TL;ANGTFT² Points, justified in the above ramblings:

• Thickness should be a record struct or a readonly record struct if immutability is wanted.
• It may benefit from its underlying value being a Rectangle, depending on how ref-y you want to get.
  • If not that, storing its values in a Vector4 or a more advanced type like Vector128<int> could possibly be beneficial, especially for certain operations (like growing all 4 underlying values).
  • Or all sorts of options, none of which matter as much as Thickness itself being a struct.
• Even a surprising amount of wasteful stack copies due to sub-optimal passing of structs is STILL faster than even one copy of an equivalent heap object.
• All the other compiler-generated goodies you get with a record struct.
• That was the TL;DR? Dude...

Footnotes

1. Too Long; Who Do You Think Wants To Read That ↩

2. Too Long; Ain't Nobody Got Time For That ↩

[dodexahedron]

That write-up was probably more work than just doing it in isolation and submitting a PR. But I did really want to make the general points about the heap, caches, and just how impactful the choice of value or reference type can be, in cases like this type.

But, for reasons touched on in the long version, I would rather wait til things have stabilized a bit more, unless consensus is to just get it over with now. 🤷‍♂️

[tig]

LOL. I didn't even know System.Windows.Thickness existed when I came up with ours.

All good points. I actually read the TL;WDYTWTRT this time too!

I'm happy to take this on at some point. I tend to write stuff from "what's the cleanest API, let's think about performance, concurrency, and all that other crap later" POV. I have no qualms about revisiting these things later. I know if I were smarter, I could do it all upfront, but hey (you're making me smarter and I appreciate that).

All that said, I'm am becoming concerned that we'll never finish v2 if we keep revisiting things. At some point good is good enough and "shipping is a feature too."

[dodexahedron]

I both feel and agree with that final concern, as that is a very important consideration (and one of the many reasons I keep bringing up the use of Projects), and I just want to reiterate that, unless I make it clear something is pretty urgent, I don't necessarily intend for it to be treated like it is, and just want to get things written down so they're on the radar. I also try to remember to put a low priority tag on them if they're not critical for V2 release as far as I'm concerned. I also am often (including with this one) plenty willing to do it myself, after whatever associated feature/issue is completed by whoever is working on it, so it doesn't disrupt their work.

That said, this specific suggestion is actually a pretty minimal effort one to carry out, and IMO the work to do it belongs in one of my miscellaneous work branches.

The one I'm most concerned about in line with your concern - because it's a major part of the public API surface and thus would not be a great idea to change significantly after release (even for the next major version) - is the event handling refactor, which I'm getting closer to being able to dive into. In addition to the other things that were obvious conflicts with it before, I am/was also waiting for new built-in View types and their desired behaviors to stabilize a bit more before I go forward with it, mostly to make things easier on you guys (and then, as a result, on myself), for change conflict avoidance. That's why I've been making fairly detailed suggestions on implementation on various issues when events come up - to help everyone else help me by doing things in a more standard way, so the final refactor results in less-drastic changes. And I'm quite appreciative that work of that nature since those suggestions were made has been much more ideal, in general, so THANKS for that!

So, some thoughts and proposals:

One thing that would be wise, just to help light fires under our butts and also to help us control both feature creep and scope creep is to have a V2 project that we put a target due date on and at least a rudimentary road map as a spec for us to target.

And milestones help show and give a rough roadmap for whatever granularity they are designated at (I'm a fan of using them for release stages, such as alpha, beta, RC, and stable, for example).

If we then choose due dates for those milestones that we at least try to stick to (but not treat them as hard deadlines, for lots of reasons), it will help both us and consumers eagerly awaiting V2 to visualize where we're at, what's going on, and manage expectations.

At minimum, we should choose a few dates, regardless of if they're in projects, milestones, or whatever:

• Feature Freeze - Date after which no new features are to be added for initial V2 release. That's also a good point to demarcate the Alpha/Beta transition. API should remain at least mostly stable after that, insofar as breaking changes for the public API surface go.
• API Freeze - Date after which breaking changes to the public API surface are to cease. Non-critical additions and bug fixes only beyond that point. Can still be a beta phase, but is reasonable to be release candidate toward the end of that phase.
• Change Freeze - Date after which no non-fix changes are to be made before release of V2. This is solidly release candidate stage.

And again all of those dates can be as soft or hard as we want, especially since this is all a volunteer labor of love.

If stuff comes up that would violate one of those dates after it passes and we've agreed we're in the next stage, we can triage at that time for whether it's important enough to push a date back or just slate it for the next minor or major version.

I think we should institute a feature freeze very soon and, once current new feature work is done, shift focus to refining, consolidating, standardizing, etc whatever we have at that point. That means things like the event refactor and other refactors that involve changes to the public API surface, going over tests to fix, expand, formalize, trim, and otherwise make them better and more solid, and turn a focused eye toward performance and resiliency. So, basically the same sort of work that's been that majority of what I've been contributing, but for all work from that point on.

All this is aimed at the very point of concern you raised about what is a VERY common problem for open-source/volunteer projects: Lack of at least loose or rudimentary structure and management of expectations for both dev and users. It's an un-fun but typical reality of any major project, but the benefits are real and at minimum give everyone something to point at to make less-arbitrary triage decisions of whether to directly address an issue now or later.