Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add gc layout for some basic types #4495

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Conversation

dgryski
Copy link
Member

@dgryski dgryski commented Oct 3, 2024

No description provided.

@dgryski
Copy link
Member Author

dgryski commented Oct 3, 2024

For #4479

@@ -47,7 +47,13 @@ func sliceGrow(oldBuf unsafe.Pointer, oldLen, oldCap, newCap, elemSize uintptr)
// memory allocators, this causes some difficult to debug issues.
newCap = 1 << bits.Len(uint(newCap))

buf := alloc(newCap*elemSize, nil)
var layout unsafe.Pointer
if elemSize == 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this check < size of pointer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was just being ultra-conservative for this initial POC, and also my test case was focussed on strings. This could absolutely be smarter.

@dgryski
Copy link
Member Author

dgryski commented Oct 23, 2024

Here's the result of https://github.com/dgryski/trifles/tree/master/tinyjsonbench with the layout patches:

   3.1  +
        |                   |
        |         +---------*---------+
        |         +-------------------+
        |                   |
   3.0  +
        |
        |
        |
   2.9  +                                        *
        |
        |
        |                                        |
   2.8  +                              +-------------------+
        |                              +---------*---------+
        |                              +-------------------+
        |                                        |
   2.7  +--------------------------------------------------------------
                        old.times            new.times

File       N   Mean  Stddev
old.times  20  3.06  0.02    (control)
new.times  20  2.79  0.04    (2.79 < 3.06 ± 0.02, p = .000)

and on wasi

        |
  10.0  +
        |
        |
   9.8  +                   |
        |                   |
        |                   |
   9.6  +                   |
        |         +-------------------+
        |         |         *         |
        |         +-------------------+          |
   9.4  +                   |                    |
        |                   |          +---------*---------+
        |                              +-------------------+
   9.2  +                                        |
        |
        |
   9.0  +--------------------------------------------------------------
                        old.times            new.times

File       N   Mean  Stddev
old.times  20  9.49  0.11    (control)
new.times  20  9.31  0.07    (9.31 < 9.49 ± 0.06, p = .000)

@dgryski dgryski changed the title WIP: add layout for strings and slices add layout for some basic types Oct 23, 2024
@dgryski dgryski marked this pull request as ready for review October 23, 2024 17:12
@dgryski
Copy link
Member Author

dgryski commented Oct 23, 2024

Ideally I think the layout should be computed by the compiler and stored with the rest of the type info data, but I'm leaving that for a later PR. I just did the easy ones for now.

@dgryski dgryski force-pushed the dgryski/gc-layout branch 2 times, most recently from badf1dd to 07d9d53 Compare October 23, 2024 17:32
@dgryski dgryski requested a review from aykevl October 23, 2024 17:39
@dgryski dgryski changed the title add layout for some basic types add gc layout for some basic types Oct 23, 2024
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! It's one of those "I should do that some day" but never actually did.

src/reflect/gclayout_precise.go Outdated Show resolved Hide resolved
src/reflect/gclayout_precise.go Outdated Show resolved Hide resolved
src/reflect/gclayout_precise.go Outdated Show resolved Hide resolved
src/reflect/gclayout_none.go Outdated Show resolved Hide resolved
Comment on lines 51 to 55
// less type info here; can only go off element size
if elemSize < unsafe.Sizeof(uintptr(0)) {
layout = gcLayoutNoPtrs
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably best solved by a small compiler change that will pass the layout to runtime.sliceAppend (like it does today with runtime.alloc).

src/runtime/string.go Outdated Show resolved Hide resolved
@dgryski
Copy link
Member Author

dgryski commented Oct 23, 2024

PTAL. I think I've addressed everything except the compiler change to pass more type info to the runtime slice functions.

@ydnar
Copy link
Contributor

ydnar commented Oct 24, 2024

Is a GC layout in this PR the same thing that big Go refers to as a GC shape?

@dgryski
Copy link
Member Author

dgryski commented Oct 24, 2024

@ydnar Yes, basically. (For those playing along at home: https://github.com/golang/proposal/blob/master/design/generics-implementation-dictionaries-go1.18.md )

For our implementation, see https://github.com/tinygo-org/tinygo/blob/dev/src/runtime/gc_precise.go .

@aykevl
Copy link
Member

aykevl commented Oct 24, 2024

Is a GC layout in this PR the same thing that big Go refers to as a GC shape?

More or less (source):

The GC shape of a type means how that type appears to the allocator / garbage collector. It is determined by its size, its required alignment, and which parts of the type contain a pointer.

The TinyGo GC layout value doesn't contain alignment and it barely contains size (it doesn't repeat pointer locations, so for a slice or array it would only use the element layout and expect the GC to repeat that). But most importantly, it contains information on which parts (may) contain a pointer - or rather, which parts most definitely don't contain a pointer.

Also, am I glad now that I wrote some rather extensive documentation for this feature!

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Assuming it passes the test corpus, I think this is fine to merge?

@dgryski
Copy link
Member Author

dgryski commented Oct 24, 2024

Yes, it passes the test corpus. If you're happy with the code in gclayout.go that constructs the layout bits, then yes we can merge this.

@aykevl
Copy link
Member

aykevl commented Oct 24, 2024

Interestingly there are some binary size and RAM usage changes:

flash                          ram
before   after   diff          before   after   diff
[...]
244264  245468   1204   0.49%    9516   10488    972  10.21% tinygo build -size short -o ./build/test.hex -target=nano-rp2040 -stack-size 8kb ./examples/net/websocket/dial/
285816  287104   1288   0.45%   17240   18212    972   5.64% tinygo build -size short -o ./build/test.hex -target=wioterminal -stack-size 8kb ./examples/net/mqttclient/paho/
337000  338372   1372   0.41%   15748   15396   -352  -2.24% tinygo build -size short -o ./build/test.hex -target=matrixportal-m4 -stack-size 8kb ./examples/net/webstatic/
284680  286076   1396   0.49%   18172   17812   -360  -1.98% tinygo build -size short -o ./build/test.hex -target=wioterminal -stack-size 8kb ./examples/net/webserver/
295148  296612   1464   0.50%   12760   13788   1028   8.06% tinygo build -size short -o ./build/test.hex -target=pyportal -stack-size 8kb ./examples/net/http-get/

These don't use the precise GC yet, and the changes are too large to reasonably be caused by the added code in this PR, so I think these are actually due to interp changes. Which I think is a good thing?

@aykevl aykevl merged commit 6e6507b into tinygo-org:dev Oct 24, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants