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

Refactor reflect package #2640

Merged
merged 2 commits into from Feb 17, 2023
Merged

Refactor reflect package #2640

merged 2 commits into from Feb 17, 2023

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented Feb 16, 2022

This is a big PR that changes the way runtime type information is stored in the binary. Instead of compressing it and storing it in a number of sidetables, it is stored similar to how the Go compiler toolchain stores it (but still more compactly).

TODO:

  • clean up the code
  • add tests
  • do some tests to check whether the encoding/json package can be fully supported without too much binary size increase

@ricochet
Copy link

ricochet commented Aug 1, 2022

I am very excited about this. I noticed map support like MapOf is not part of this refactor.
That's something I'm game to help implement to close #2640.

The project I'm looking to build with WASI/Wasm is fq.
With many different support formats, this will likely exercise reflect more than most libraries.

I gave this PR a try on an M1. The error below doesn't appear to be M1 specific, but many of the others likely are.

--- FAIL: TestInterfaceLowering (0.00s)
panic: expected method set [recovered]
        panic: expected method set

goroutine 8 [running]:
testing.tRunner.func1.2({0x106ed6ee0, 0x106f2ea50})
        /opt/homebrew/Cellar/go/1.18.4/libexec/src/testing/testing.go:1389 +0x1c8
testing.tRunner.func1()
        /opt/homebrew/Cellar/go/1.18.4/libexec/src/testing/testing.go:1392 +0x380
panic({0x106ed6ee0, 0x106f2ea50})
        /opt/homebrew/Cellar/go/1.18.4/libexec/src/runtime/panic.go:838 +0x20c
github.com/tinygo-org/tinygo/transform.(*lowerInterfacesPass).run(0x1400022ba48)
        /Users/bhayes/repos/tinygo/transform/interface-lowering.go:170 +0x10d8
github.com/tinygo-org/tinygo/transform.LowerInterfaces({0x14e81d3d0?}, 0x1070d49c0)
        /Users/bhayes/repos/tinygo/transform/interface-lowering.go:131 +0x270
github.com/tinygo-org/tinygo/transform_test.TestInterfaceLowering.func1({0x14000212060?})
        /Users/bhayes/repos/tinygo/transform/interface-lowering_test.go:13 +0x40
github.com/tinygo-org/tinygo/transform_test.testTransform(0x14000123a00, {0x1059f6b94, 0x12}, 0x14000053748)
        /Users/bhayes/repos/tinygo/transform/transform_test.go:48 +0x1c0
github.com/tinygo-org/tinygo/transform_test.TestInterfaceLowering(0x14000123a00)
        /Users/bhayes/repos/tinygo/transform/interface-lowering_test.go:12 +0x50
testing.tRunner(0x14000123a00, 0x106f2e0a8)
        /opt/homebrew/Cellar/go/1.18.4/libexec/src/testing/testing.go:1439 +0x110
created by testing.(*T).Run
        /opt/homebrew/Cellar/go/1.18.4/libexec/src/testing/testing.go:1486 +0x328
FAIL    github.com/tinygo-org/tinygo/transform  0.804s

fuzzyEqualIR -> given the target triple, I'm guessing this is darwin-arm64 specific:

    interp_test.go:99: output does not match expected output:
        ; ModuleID = 'testdata/interface.ll'
        source_filename = "testdata/interface.ll"
        target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
        target triple = "x86_64--linux"
        
        @main.v1 = local_unnamed_addr global i1 false
        @main.v2 = local_unnamed_addr global i1 false
        
        define void @runtime.initAll() unnamed_addr {
        entry:
          ret void
        }

Likely related to the same issue as above:

=== CONT  TestOptimizeReflectImplements
transform_test.go:75: output does not match expected output:
    target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"
    target triple = "i686--linux"
    
    %runtime.typecodeID = type { %runtime.typecodeID*, i32, %runtime.interfaceMethodInfo*, %runtime.typecodeID*, i32 }
    %runtime.interfaceMethodInfo = type { i8*, i32 }
    
    @"reflect/types.type:named:error" = linkonce_odr constant %runtime.typecodeID { %runtime.typecodeID* @"reflect/types.type:interface:{Error:func:{}{basic:string}}", i32 0, %runtime.interfaceMethodInfo* null, %runtime.typecodeID* null, i32 ptrtoint (i1 (i32)* @"error.$typeassert" to i32) }
    @"reflect/types.type:interface:{Error:func:{}{basic:string}}" = linkonce_odr constant %runtime.typecodeID { %runtime.typecodeID* bitcast ([1 x i8*]* @"reflect/types.interface:interface{Error() string}$interface" to %runtime.typecodeID*), i32 0, %runtime.interfaceMethodInfo* null, %runtime.typecodeID* null, i32 ptrtoint (i1 (i32)* @"error.$typeassert" to i32) }
    @"reflect/methods.Error() string" = linkonce_odr constant i8 0
    @"reflect/types.interface:interface{Error() string}$interface" = linkonce_odr constant [1 x i8*] [i8* @"reflect/methods.Error() string"]
    @"reflect/methods.Align() int" = linkonce_odr constant i8 0
    @"reflect/methods.Implements(reflect.Type) bool" = linkonce_odr constant i8 0
    @"reflect.Type$interface" = linkonce_odr constant [2 x i8*] [i8* @"reflect/methods.Align() int", i8* @"reflect/methods.Implements(reflect.Type) bool"]
    @"reflect/types.type:named:reflect.rawType" = linkonce_odr constant %runtime.typecodeID { %runtime.typecodeID* @"reflect/types.type:basic:uintptr", i32 0, %runtime.interfaceMethodInfo* getelementptr inbounds ([20 x %runtime.interfaceMethodInfo], [20 x %runtime.interfaceMethodInfo]* @"reflect.rawType$methodset", i32 0, i32 0), %runtime.typecodeID* null, i32 0 }
    @"reflect.rawType$methodset" = linkonce_odr constant [20 x %runtime.interfaceMethodInfo] zeroinitializer
    @"reflect/types.type:basic:uintptr" = linkonce_odr constant %runtime.typecodeID zeroinitializer
    
    define i1 @main.isError(i32 %typ.typecode, i8* %typ.value, i8* %context) {
    entry:
      %result = call i1 @"reflect.Type.Implements$invoke"(i8* %typ.value, i32 ptrtoint (%runtime.typecodeID* @"reflect/types.type:named:reflect.rawType" to i32), i8* bitcast (%runtime.typecodeID* @"reflect/types.type:named:error" to i8*), i32 %typ.typecode, i8* undef)
      ret i1 %result
    }
    
    define i1 @main.isUnknown(i32 %typ.typecode, i8* %typ.value, i32 %itf.typecode, i8* %itf.value, i8* %context) {
    entry:
      %result = call i1 @"reflect.Type.Implements$invoke"(i8* %typ.value, i32 %itf.typecode, i8* %itf.value, i32 %typ.typecode, i8* undef)
      ret i1 %result
    }
    
    declare i1 @"reflect.Type.Implements$invoke"(i8*, i32, i8*, i32, i8*) #0
    
    declare i1 @"error.$typeassert"(i32) #1
    
    attributes #0 = { "tinygo-invoke"="reflect/methods.Implements(reflect.Type) bool" "tinygo-methods"="reflect/methods.Align() int; reflect/methods.Implements(reflect.Type) bool" }
    attributes #1 = { "tinygo-methods"="reflect/methods.Error() string" }

This is almost definitely from me running on arm (note works on dev).

=== CONT  TestTest/WASM/Fail
panic: unexpected feature flags

goroutine 234 [running]:
github.com/tinygo-org/tinygo/compiler.(*compilerContext).isThumb(0x107260501?)
        /Users/bhayes/repos/tinygo/compiler/llvm.go:299 +0x8c
github.com/tinygo-org/tinygo/compiler.(*builder).createInvokeCheckpoint(0x14004fecc80)
        /Users/bhayes/repos/tinygo/compiler/defer.go:144 +0x70
github.com/tinygo-org/tinygo/compiler.(*builder).createInvoke(0x1401263e270?, {0x1400a4af408?}, {0x1400a4af570, 0x2, 0x2}, {0x0, 0x0})
        /Users/bhayes/repos/tinygo/compiler/calls.go:81 +0x48
github.com/tinygo-org/tinygo/compiler.(*builder).createFunctionCall(0x14004fecc80, 0x14000f1fd40)
        /Users/bhayes/repos/tinygo/compiler/compiler.go:1715 +0xae8
github.com/tinygo-org/tinygo/compiler.(*builder).createExpr(0x14004fecc80, {0x1072d5ee0?, 0x14000f1fd00?})
        /Users/bhayes/repos/tinygo/compiler/compiler.go:1811 +0x1760
github.com/tinygo-org/tinygo/compiler.(*builder).createInstruction(0x14004fecc80, {0x1072d5e98?, 0x14000f1fd00?})
        /Users/bhayes/repos/tinygo/compiler/compiler.go:1323 +0x780
github.com/tinygo-org/tinygo/compiler.(*builder).createFunction(0x14004fecc80)
        /Users/bhayes/repos/tinygo/compiler/compiler.go:1222 +0x7b8
github.com/tinygo-org/tinygo/compiler.(*compilerContext).createPackage(0x1401263e270, {0x14007f581a0?}, 0x140035c8380)
        /Users/bhayes/repos/tinygo/compiler/compiler.go:795 +0x518
github.com/tinygo-org/tinygo/compiler.CompilePackage({0x14012d4da40?, 0x140081f69c0?}, 0x14013189680, 0x140035c8380, {0x1400005dc18?}, 0x105c7d86d?, 0x0?)
        /Users/bhayes/repos/tinygo/compiler/compiler.go:294 +0x3c8
github.com/tinygo-org/tinygo/builder.Build.func3(0x14000db8780)
        /Users/bhayes/repos/tinygo/builder/build.go:354 +0x1a8
github.com/tinygo-org/tinygo/builder.runJob(0x14000db8780, 0x0?)
        /Users/bhayes/repos/tinygo/builder/jobs.go:222 +0x4c
created by github.com/tinygo-org/tinygo/builder.runJobs
        /Users/bhayes/repos/tinygo/builder/jobs.go:123 +0x44c
FAIL    github.com/tinygo-org/tinygo    1.929s
FAIL
make: *** [test] Error 1
# test workflow
git checkout dev
git pull --rebase origin dev
make test
# all tests passed
gh pr checkout 2640

make test
# about 4 test failures

# passing spot checks
make wasmtest
make tinygo-test-wasi-fast

make smoketest # failed
# /opt/homebrew/bin/tinygo build -size short -o test.hex -target=xiao-rp2040         examples/blinky1
# error: open /opt/homebrew/Cellar/tinygo/0.24.0/targets/xiao-rp2040.json: no such file or directory
# make: *** [smoketest] Error 1

@aykevl
Copy link
Member Author

aykevl commented Aug 31, 2022

I am very excited about this. I noticed map support like MapOf is not part of this refactor.
That's something I'm game to help implement to close #2640.

Correct. This is purely a refactor, it doesn't add new features.

@aykevl
Copy link
Member Author

aykevl commented Oct 9, 2022

Current state: it works, but I'd like to do some more testing before marking this ready for review.

@dgryski
Copy link
Member

dgryski commented Oct 10, 2022

This seems like the perfect time to spin up the test corpus..

@aykevl
Copy link
Member Author

aykevl commented Oct 10, 2022

I had to patch the corpus a bit to get it to work (this is also an issue in the dev branch):

   - pkg: curve25519
   - pkg: ed25519
   - pkg: hkdf
-  - pkg: internal/subtle
+  #- pkg: internal/subtle
   - pkg: md4
   - pkg: nacl/auth
   - pkg: nacl/box

but with that, all tests in the corpus pass.

I also did a quick code size check, and it currently stands at a 3-4% increase in code size. Which still feels like too much to me, but we do need the features this PR unlocks. Maybe I can still improve it somehow.

@dgryski
Copy link
Member

dgryski commented Oct 10, 2022

Arguably we shouldn't test internal/* packages. Probably safe to remove that from the corpus yaml.

@aykevl
Copy link
Member Author

aykevl commented Oct 11, 2022

Update: I managed to shave off ~0.5% of code size (out of 3-4%) with some optimizations, but I'm hitting a weird memory corruption that I don't understand.

@dgryski
Copy link
Member

dgryski commented Oct 17, 2022

Do you have an example of the memory corruption I could take a look at?

@johnkeates
Copy link

Any chance this makes it into 0.27?

@dgryski
Copy link
Member

dgryski commented Feb 4, 2023

As this is a large change, we specifically didn't want to land it into a release right at the end of the cycle. The plan has always been to merge it right at the start so that people running the development branch have more time to hit and hunt down any issues before releasing.

@johnkeates
Copy link

Makes sense, thanks for the reply 👍

This is a big commit that changes the way runtime type information is stored in
the binary. Instead of compressing it and storing it in a number of sidetables,
it is stored similar to how the Go compiler toolchain stores it (but still more
compactly).

This has a number of advantages:

  * It is much easier to add new features to reflect support. They can simply
    be added to these structs without requiring massive changes (especially in
    the reflect lowering pass).
  * It removes the reflect lowering pass, which was a large amount of hard to
    understand and debug code.
  * The reflect lowering pass also required merging all LLVM IR into one
    module, which is terrible for performance especially when compiling large
    amounts of code. See issue 2870 for details.
  * It is (probably!) easier to reason about for the compiler.

The downside is that it increases code size a bit, especially when reflect is
involved. I hope to fix some of that in later patches.
@aykevl
Copy link
Member Author

aykevl commented Feb 15, 2023

Rebased to fix merge conflicts.

@dgryski
Copy link
Member

dgryski commented Feb 15, 2023

I'm happy to get this in. It passes the test corpus and my work on getting JSON and friends working will shake out any further issues.

@aykevl
Copy link
Member Author

aykevl commented Feb 15, 2023

@deadprogram any comment before merging?

@deadprogram
Copy link
Member

deadprogram commented Feb 15, 2023

Waiting until the Windows CI builds all pass...

Which means yet another full LLVM build. (sigh)

@deadprogram
Copy link
Member

This keeps failing just because windows, but since it has to rebuild all of LLVM it cannot seem to get past it.

Just created #3453 to try to improve the situation.

@deadprogram
Copy link
Member

At last, merge! Thank you very much @aykevl for all your work on this and to @dgryski for the extra effort to help get it in.

@deadprogram deadprogram merged commit 4e84531 into dev Feb 17, 2023
@deadprogram deadprogram deleted the reflect-refactor-3 branch February 17, 2023 21:54
@jamesmulcahy
Copy link

I've been watching and waiting for this PR for some time, and I just wanted to say thanks to @aykevl @dgryski and everyone else for their efforts here. I'm excited for the improvements (notably json support) that can build on this refactor.

👏 👏👏👏

aykevl added a commit that referenced this pull request Feb 25, 2023
Add the timers test because they now work correctly on AVR, probably as
a result of the reflect refactor: #2640

I've also updated a few of the other tests to indicate the new status
and why they don't work. It's no longer because of compiler errors, but
because of linker or runtime errors (which is at least some progress).
For example, I found that testdata/reflect.go works if you disable
`testAppendSlice` and increase the stack size.
@aykevl aykevl mentioned this pull request Feb 25, 2023
aykevl added a commit that referenced this pull request Feb 25, 2023
Add the timers test because they now work correctly on AVR, probably as
a result of the reflect refactor: #2640

I've also updated a few of the other tests to indicate the new status
and why they don't work. It's no longer because of compiler errors, but
because of linker or runtime errors (which is at least some progress).
For example, I found that testdata/reflect.go works if you disable
`testAppendSlice` and increase the stack size.
deadprogram pushed a commit that referenced this pull request Feb 26, 2023
Add the timers test because they now work correctly on AVR, probably as
a result of the reflect refactor: #2640

I've also updated a few of the other tests to indicate the new status
and why they don't work. It's no longer because of compiler errors, but
because of linker or runtime errors (which is at least some progress).
For example, I found that testdata/reflect.go works if you disable
`testAppendSlice` and increase the stack size.
@aykevl aykevl mentioned this pull request Mar 2, 2023
aykevl added a commit that referenced this pull request Mar 2, 2023
This non-determinism was introduced in
#2640. Non-determinism in the
compiler is a bug because it makes it harder to find whether a compiler
change actually affected the binary.

Fixes #3504
deadprogram pushed a commit that referenced this pull request Mar 2, 2023
This non-determinism was introduced in
#2640. Non-determinism in the
compiler is a bug because it makes it harder to find whether a compiler
change actually affected the binary.

Fixes #3504
conejoninja pushed a commit to conejoninja/tinygo that referenced this pull request Mar 22, 2023
Add the timers test because they now work correctly on AVR, probably as
a result of the reflect refactor: tinygo-org#2640

I've also updated a few of the other tests to indicate the new status
and why they don't work. It's no longer because of compiler errors, but
because of linker or runtime errors (which is at least some progress).
For example, I found that testdata/reflect.go works if you disable
`testAppendSlice` and increase the stack size.
conejoninja pushed a commit to conejoninja/tinygo that referenced this pull request Mar 22, 2023
This non-determinism was introduced in
tinygo-org#2640. Non-determinism in the
compiler is a bug because it makes it harder to find whether a compiler
change actually affected the binary.

Fixes tinygo-org#3504
deadprogram pushed a commit that referenced this pull request Mar 28, 2023
Add the timers test because they now work correctly on AVR, probably as
a result of the reflect refactor: #2640

I've also updated a few of the other tests to indicate the new status
and why they don't work. It's no longer because of compiler errors, but
because of linker or runtime errors (which is at least some progress).
For example, I found that testdata/reflect.go works if you disable
`testAppendSlice` and increase the stack size.
deadprogram pushed a commit that referenced this pull request Mar 28, 2023
This non-determinism was introduced in
#2640. Non-determinism in the
compiler is a bug because it makes it harder to find whether a compiler
change actually affected the binary.

Fixes #3504
LiuJiazheng pushed a commit to LiuJiazheng/tinygo that referenced this pull request Aug 20, 2023
Add the timers test because they now work correctly on AVR, probably as
a result of the reflect refactor: tinygo-org#2640

I've also updated a few of the other tests to indicate the new status
and why they don't work. It's no longer because of compiler errors, but
because of linker or runtime errors (which is at least some progress).
For example, I found that testdata/reflect.go works if you disable
`testAppendSlice` and increase the stack size.
LiuJiazheng pushed a commit to LiuJiazheng/tinygo that referenced this pull request Aug 20, 2023
This non-determinism was introduced in
tinygo-org#2640. Non-determinism in the
compiler is a bug because it makes it harder to find whether a compiler
change actually affected the binary.

Fixes tinygo-org#3504
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.

None yet

6 participants