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

WIP: PoC of using a bit of generics and less reflection #172

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Zyko0
Copy link

@Zyko0 Zyko0 commented Oct 8, 2023

Not meant as a merge candidate, just a PoC

Context: ebitengine#purego (discord server)

This is a proof of concept of a generic alternative for RegisterFunc as functions ranging from RegisterFunc0_0 (0 input, 0 output) to RegisterFunc9_1 (9 inputs, 1 output).
The goal is to remove a good amount of reflection where it's not needed, but also to remove the need of reflect.MakeFunc which returns a heavy function that has plenty of overhead at runtime.

There are tests and benchmarks for a few functions of the libc in func_test.go

Difference for strlen:

Syscall9:       11762629 - 100.5 ns/op - 32 B/op  - 1 allocs/op
RegisterFunc:    2411634 - 490.4 ns/op - 120 B/op - 6 allocs/op
RegisterFunc11:  7690965 - 157.0 ns/op - 176 B/op - 2 allocs/op

Notes:

  • This approach might generate heavier go binaries due to the (abuse of) generics
  • The syscallStack types are really weird in their implementations, but can be swapped for something else implementing the same interface
  • purego.Symbol should probably be named differently or not be exposed this way
  • There are lots of type cast and type assertions (unchecked) but they should be safe because the type is always known
  • I couldn't implement some arguments or return value's types, so there's still runtime reflection in the generated function for those as a fallback
  • Not all RegisterFunc have been implemented yet
  • newSyscallStack() causes an allocation everytime in each runtime function as it escapes, couldn't find a way to remove it so far
  • /!\ Only tested on Windows
  • /!\ It needs way more tests to assess that it's okay
  • /!\ I got rid of all the runtime.KeepAlive stuff because I couldn't find a proper way to test their need
Benchmark results on my machine (Windows)
goos: windows
goarch: amd64
pkg: github.com/ebitengine/purego
cpu: AMD Ryzen 7 3800X 8-Core Processor
Benchmark_NewCallBack/RegisterFunc(original)-16         	 1304842	       918.7 ns/op	     328 B/op	      12 allocs/op
Benchmark_NewCallBack/RegisterFunc9_1(new)-16           	 3216594	       373.6 ns/op	     144 B/op	       1 allocs/op
Benchmark_qsort/RegisterFunc(original)-16               	  599888	      2022 ns/op	     264 B/op	       6 allocs/op
Benchmark_qsort/RegisterFunc1_0(new)-16                 	  666532	      1788 ns/op	     296 B/op	       4 allocs/op
Benchmark_strlen/RegisterFunc(original)-16              	 2451050	       491.5 ns/op	     120 B/op	       6 allocs/op
Benchmark_strlen/RegisterFunc1_1(new)-16                	 7641933	       158.3 ns/op	     176 B/op	       2 allocs/op
Benchmark_strlen/SyscallN-16                            	 8331891	       143.2 ns/op	     112 B/op	       2 allocs/op
Benchmark_cos/RegisterFunc(original)-16                 	 3273643	       367.5 ns/op	      64 B/op	       4 allocs/op
Benchmark_cos/RegisterFunc1_1(new)-16                   	 9373315	       126.0 ns/op	     144 B/op	       1 allocs/op
Benchmark_cos/Go-16                                     	137118896	         8.760 ns/op	       0 B/op	       0 allocs/op
Benchmark_isupper/RegisterFunc(original)-16             	 3233931	       371.6 ns/op	      56 B/op	       4 allocs/op
Benchmark_isupper/RegisterFunc1_1(new)-16               	10167706	       120.4 ns/op	     144 B/op	       1 allocs/op

@MatejMagat305
Copy link

is this plan or only example and hypotetical idea?

@TotallyGamerJet
Copy link
Collaborator

is this plan or only example and hypotetical idea?

It is just a suggestion to improve performance. There is currently no plans to integrate the ideas in this PR. However that could change in the future

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