Optimize Font calculations by avoiding unnecessary copy-on-write.

[Ivorforce]

• Production edit: Related to Optimize text rendering by caching UBreakIterator instances. #102129.

I profiled scrolling across 4000 Node entries in the scene tree.
I was able to improve its performance by 20% by avoiding unnecessary copy-on-write calls.

Explanation

Array[i] has a const and a non-const implementation.
When a mutable instance is available, the non-const implementation is always chosen. This version implies a fork of the CowData (copy-on-write).
Font has a mutable TypedArray<RID> rids - because it is mutable, it is always non-const. For this reason, it will always cause forks on operator[] access.
The .get function does not have a non-const implementation. As such, it does not cause forks.

Profiler

I profiled the scrolling across 4000 items in the scene tree.
I used Apple Instruments' CPU profiler and inverted the call tree.

Before (after this PR, this entry is completely gone).

1.50 Gc  22,8 %	  CowData<Variant>::_copy_on_write()
1.48 Gc  22,4 %	   CowData<Variant>::ptrw()
1.48 Gc  22,4 %	    VectorWriteProxy<Variant>::operator[](long long)
1.48 Gc  22,4 %	     Array::operator[](int)
1.48 Gc  22,4 %	      Font::get_height(int) const
1.48 Gc  22,4 %	       Tree::compute_item_height(TreeItem*) const
917.26 Mc  13,9 %	        Tree::get_item_height(TreeItem*) const
917.26 Mc  13,9 %	         Tree::get_item_height(TreeItem*) const
917.26 Mc  13,9 %	          Tree::get_internal_min_size() const
917.26 Mc  13,9 %	           Tree::update_scrollbars()

Caveats

It would be better to eliminate the non-const operator[] entirely, and replace it with a Vector.write-like construct, such that CoW must be intentional. This change would avoid this kind of problem in the future.
However, that will require quite a lot more work.

[kiroxas]

Nice catch !

[fire]

I believe that CoW forced on and the edge cases on a case-by-case issue were historical improvements. https://godotengine.org/article/why-we-broke-your-pr/ So I don't suggest flipping it back.

[Ivorforce]

My suggestion would be to double down and do the same to Array as was done to Vector, for the same reason outlined by your linked article :) In other words, the opposite of flipping it back!
As mentioned however that would be a bigger change requiring touching more code.

[bruvzg]

This change should be completely safe, so might be worth moving it to 4.4 milestone.

[Calinou]

Tested locally, it works as expected.

Scrolling the Scene tree dock with 16385 nodes in it feels noticeably smoother, especially near the bottom (tested on an Linux optimized editor build with LTO).

[akien-mga]

Would we gain anything from extracting rids.get(i) to a const variable, or will the compiler optimize it all the same?

[Ivorforce]

Good question!
Array.get has the overhead of a function call, an if statement that is likely to be predicted correctly (read_only), and another if statement that is likely to be predicted correctly (bad index). If the array is large, there will also be an unnecessary cache contention (through repeated access of size() and dereferencing ArrayPrivate).
All in all, it could be optimized, but it's unlikely to make an observable difference, since all these factors should add up to only ~20 wasted clock cycles¹ for each get call in most cases - and draw_char isn't particularly problematic in the profiler right now anyway.

If I were to make another rebase, I'd probably cache the reference to be safe, but through this estimation I don't think it's particularly important.

Footnotes

1. Napkin math via Agner Fog's heuristics. ↩

[kiroxas]

60 cycles gained for adding a single line seems like a good trade off to me :)

[Ivorforce]

I actually implemented this and was close to pushing, but then found this bug when checking related code to make sure it's safe.
It doesn't appear that Font.rids is read-only, but I didn't want to risk ruining a simple PR by introducing a potential regression, so I would like to keep it as-is for now. I plan to make a PR switching Font.rids to Vector for 4.5 anyway, and I'll just add the cache there I think.

[Repiteo]

Thanks!