runtime: Simplify slice growing/appending code

[lpereira]

Refactor the slice appending function to rely on the slice growing function, and remove branches/loops to use a branchfree variant.

CC @dgryski

[dgryski]

Some quick testing shows this makes the multiplication factor 1.125. However the expression doesn't seem to work for newCap <= 8 so I think we need to make sure the edge cases are handled properly.

[dgryski]

I'm not sure this actually saves us anything. I threw together a quick simulation and it seems that the new algorithm requires more calls to "grow" (since we're not growing as much, we need to do it more frequently, requiring more CPU) and ends up generating more garbage, because the additional calls to grow means we have to throw more stuff away.

You can play with my sample code here: https://go.dev/play/p/QcHiMqnGlym

[lpereira]

Yup, that seems to be the case from some of the testing I did last week to understand why some tests were failing. I'm going to send a new version that does pretty much the same thing as before but keeps the cleanup.

…

On Mon, Jul 22, 2024, at 11:53 AM, Damian Gryski wrote: I'm not sure this actually saves us anything. I threw together a quick simulation and it seems that the new algorithm requires more calls to "grow" (since we're not growing as much, we need to do it more frequently, requiring more CPU) *and* ends up generating more garbage, because the additional calls to grow means we have to throw more stuff away. You can play with my sample code here: https://go.dev/play/p/QcHiMqnGlym — Reply to this email directly, view it on GitHub <#4287 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAADVGKW2UBXOAKSZRH5APLZNVIKBAVCNFSM6AAAAABI7FV6YCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBTGYYDSOJZHE>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[lpereira]

Alright, @dgryski, pushed a new version, rebased on the current dev branch, that goes back to the "align to the next power of two" code, but using a branchfree version instead.

[lpereira]

The failure inside the reflect test is very weird.

[cee-dub]

The Index field of "want" for the reflect field F looks incorrect, it is field 1, but want is 0,0.

[ldemailly]

lgtm. nice to share the code in grow. just some (non merge blocking) questions

[ldemailly]

should this have some overflow check?

[lpereira]

Yes, it should. Even calculating the new newCap can overflow. However, handling overflow conditions doesn't seem to be something that's done pervasively in TinyGo, so I didn't do it here so this is closer to the rest of the code.

[aykevl]

I think it is very unlikely to overflow in practice, but yes it would be a good idea to have some sort of check for this.
For 32-bit and 64-bit systems it would be possible to use math/bits for this: https://pkg.go.dev/math/bits#Add32
But maybe there are universal expressions for detecting such an overflow condition efficiently.

[lpereira]

The Index field of "want" for the reflect field F looks incorrect, it is field 1, but want is 0,0.

Right, and I have no idea why! I've spent some time in the reflect code, and @dgryski too, but we don't yet know what's happening here. Our best guesses is that there's either memory corruption somewhere, or that, although the code seems correct, its behavior is different from before, and the reflect code is probably depending on that somehow.

[cee-dub]

Hmm, I was suggesting/agreeing with @dgryski that the test may be faulty and the old implementation matched. I think the expectation looks incorrect, though I have very little experience or deep knowledge of this code.

[lpereira]

Hmm, I was suggesting/agreeing with @dgryski that the test may be faulty and the old implementation matched. I think the expectation looks incorrect, though I have very little experience or deep knowledge of this code.

If I understood the test correctly, it seems correct. The index is a slice of indices, starting with the current type, and following any nested type. So an index of []int{0} means the field is the first in that struct; and an index of []int{0, 1} means it's the second of the first of that struct.

The test defines two structs: Rec1 and Rec2; Rec1 has a pointer to Rec2, and Rec2 has a F string and a pointer to Rec1. To find F from Rec1, you need to go through the first element, and then find the first element ([]int{0, 0}), which seems to agree with the test.

I might be mistaken about how this works, though.

[cee-dub]

Reading more of #tinygo-dev, feel free to ignore me. I didn't look closely enough at the Index type in the test.

[lpereira]

So I copied the failing test to its own file, and reflect.VisibleFields() gives the right output, agreeing with Big Go. So the problem seems to be in the actual test driver somewhere.

[lpereira]

Fixed the reflect test; let's see if CI likes it.

[lpereira]

Alright, archive/zip failing is probably the reflect.DeepEqual() calls inside the tests, most likely some unbounded recursion or something of the sort.

[dgryski]

@lpereira Ok, so it does seem like the bug is somewhere in reflect then. I'm going to try to hunt down the bug looking at that function.

[dgryski]

This fixes the reflect test:

diff --git a/src/reflect/type.go b/src/reflect/type.go
index 1356f67c..32bf68d7 100644
--- a/src/reflect/type.go
+++ b/src/reflect/type.go
@@ -774,7 +774,7 @@ func (t *rawType) rawFieldByNameFunc(match func(string) bool) (rawStructField, [
                                if match(name) {
                                        found = append(found, result{
                                                rawStructFieldFromPointer(descriptor, field.fieldType, data, flagsByte, name, offset),
-                                               append(ll.index, int(i)),
+                                               append(ll.index[:len(ll.index):len(ll.index)], int(i)),
                                        })
                                }

@@ -787,7 +787,7 @@ func (t *rawType) rawFieldByNameFunc(match func(string) bool) (rawStructField, [

                                        nextlevel = append(nextlevel, fieldWalker{
                                                t:     embedded,
-                                               index: append(ll.index, int(i)),
+                                               index: append(ll.index[:len(ll.index):len(ll.index)], int(i)),
                                        })
                                }

I'll get this in as another PR and you can rebase against that with only the slice allocation changes.

[dgryski]

#4367

[lpereira]

I rebased this on top of #4387 (please merge it first!), and removed the commit changing reflect. CI is still failing with some error I was unable to track down.

[dgryski]

I'll look at the archive/zip failure this evening.

[dgryski]

The archive/zip failure seems to be related to the stepped doubling logic function.

[dgryski]

Nope; failure seems intermittent.

[dgryski]

This is due to memory fragmentation which (annoyingly) has different behaviour with the new growth algorithm. A smarter allocator that has free lists for small allocation sizes to reduce fragmentation would help here, but that's outside the scope of this PR.

[lpereira]

A smarter allocator that has free lists for small allocation sizes to reduce fragmentation would help here, but that's outside the scope of this PR.

Agreed. I've rebased on the current dev branch and changed the code to have only one overallocation strategy until we have a better allocator. This won't reduce the amount of memory used by slices, but it simplifies the code quite a bit, which is already a gain IMHO.

[ldemailly]

fragmentation because of not taking headers/meta into account was my point in #4287 (comment)

[lpereira]

sizediff failure is unrelated.

[dgryski]

LGTM

[dgryski]

Merging. Will close my PR that has the reflect fix.

[aykevl]

This will force a new allocation on every append, is that intentional?

[dgryski]

Yes. This fixes a test failure in reflect that was triggered by the new slice allocation code which made this sometimes be a copy and sometimes not. (The old growth code made this always a copy in the test, but the new growth code expanded it more and so sometimes reused the old slice, which then had entries in the slice overwritten by later loop iterations, causing the faiure.)