Stream: git-wasmtime

Topic: wasmtime / issue #4766 cranelift: Remove TestName as an o...

Wasmtime GitHub notifications bot (Aug 24 2022 at 09:15):

afonso360 edited issue #4766:

:wave: Hey,

Feature

This is a follow up to https://github.com/bytecodealliance/wasmtime/pull/4667#issuecomment-1218328143, where we discussed removing TestName as an option for Function names.

Benefit

The biggest benefit is cleaning up our naming interface. TestNames arguably shouldn't belong to the core cranelift infrastructure. Instead the test suite should have an external table as any other embedder would do.

The other benefit was allowing larger names in test functions, but @jameysharp has fixed that in https://github.com/bytecodealliance/wasmtime/pull/4764 (Thanks!)

@jameysharp tested this and while it does have some performance benefits, they are minimal.

This also removes some edges such as cranelift-jit not supporting test names which makes for a better experience using it.

Implementation

I started working on this a few days ago, but I'm putting it on hold to fix some other more prioritary issues. (in progress branch) If anyone wants to pick this up in the mean time feel free!

The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.

Following from that we need to switch the testsuite to use this new side table.

And after that we should be able to remove the TestName from cranelift.

This sounds simple, but I think it will end up being quite a big change.

Alternatives

Do nothing. This has been the state of cranelift for a long time. I think this would also break the interface for pretty much every downstream user since we would probably remove UserFuncName and replace it with UserExernalName directly.

Wasmtime GitHub notifications bot (Aug 24 2022 at 09:15):

afonso360 edited issue #4766:

:wave: Hey,

Feature

This is a follow up to https://github.com/bytecodealliance/wasmtime/pull/4667#issuecomment-1218328143, where we discussed removing TestName as an option for Function names.

Benefit

The biggest benefit is cleaning up our naming interface. TestNames arguably shouldn't belong to the core cranelift infrastructure. Instead the test suite should have an external table as any other embedder would do.

The other benefit was allowing larger names in test functions, but @jameysharp has fixed that in https://github.com/bytecodealliance/wasmtime/pull/4764 (Thanks!)

@jameysharp tested this and while it does have some performance benefits, they are minimal.

This also removes some edges such as cranelift-jit not supporting test names which makes for a better experience using it.

Implementation

I started working on this a few days ago, but I'm putting it on hold to fix some other more prioritary issues. (in progress branch) If anyone wants to pick this up in the mean time feel free!

The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.

Following from that we need to switch the testsuite to use this new side table.

And after that we should be able to remove the TestName from cranelift.

This sounds simple, but I think it will end up being quite a big change.

Alternatives

Do nothing. This has been the state of cranelift for a long time.

I think this would also break the interface for pretty much every downstream user since we would probably remove UserFuncName and replace it with UserExernalName directly.

Wasmtime GitHub notifications bot (Aug 24 2022 at 10:08):

bjorn3 commented on issue #4766:

The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.

Would it be possible to provide this side table while printing functions?

Wasmtime GitHub notifications bot (Aug 24 2022 at 10:14):

afonso360 edited issue #4766:

:wave: Hey,

Feature

This is a follow up to https://github.com/bytecodealliance/wasmtime/pull/4667#issuecomment-1218328143, where we discussed removing TestName as an option for Function names.

Benefit

The biggest benefit is cleaning up our naming interface. TestNames arguably shouldn't belong to the core cranelift infrastructure. Instead the test suite should have an external table as any other embedder would do.

The other benefit was allowing larger names in test functions, but @jameysharp has fixed that in https://github.com/bytecodealliance/wasmtime/pull/4764 (Thanks!)

@jameysharp tested this and while it does have some performance benefits, they are minimal.

This also removes some edges such as cranelift-jit not supporting test names which makes for a better experience using it.

Implementation

I started working on this a few days ago, but I'm putting it on hold to fix some other more prioritary issues. (in progress branch) If anyone wants to pick this up in the mean time feel free!

The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.

Following from that we need to switch the testsuite to use this new side table.

And after that we should be able to remove the TestName from cranelift.

This sounds simple, but I think it will end up being quite a big change.

Alternatives

Do nothing. This has been the state of cranelift for a long time.

I think this would also break the interface for pretty much every downstream user since we would probably remove UserFuncName and replace it with UserExernalName directly.

Edit: Another alternative would be to expand this, and start supporting string function names, which would also be an interesting option.

Wasmtime GitHub notifications bot (Aug 24 2022 at 10:17):

afonso360 commented on issue #4766:

I think it should be possible, inside the test suite we use TestCase 's so when printing those we can just query the name table.

clif-util run i think also shows functions to the user when compilation fails, but I think it also uses the TestCase (not quite sure).

I don't know exactly where else we show these names to the user.

Wasmtime GitHub notifications bot (Aug 24 2022 at 10:17):

afonso360 edited a comment on issue #4766:

I think it should be possible, inside the test suite we use TestCase 's so when printing those we can just query the name table.

clif-util run/compile I think also shows functions to the user when compilation fails, but I think it also uses the TestCase (not quite sure).

I don't know exactly where else we show these names to the user.

Wasmtime GitHub notifications bot (Aug 24 2022 at 10:24):

bjorn3 commented on issue #4766:

We show them as part of fn0 = %foo() -> i32, f32 function declarations inside the function body.

Wasmtime GitHub notifications bot (Sep 02 2022 at 15:50):

akirilov-arm labeled issue #4766:

:wave: Hey,

Feature

This is a follow up to https://github.com/bytecodealliance/wasmtime/pull/4667#issuecomment-1218328143, where we discussed removing TestName as an option for Function names.

Benefit

The biggest benefit is cleaning up our naming interface. TestNames arguably shouldn't belong to the core cranelift infrastructure. Instead the test suite should have an external table as any other embedder would do.

The other benefit was allowing larger names in test functions, but @jameysharp has fixed that in https://github.com/bytecodealliance/wasmtime/pull/4764 (Thanks!)

@jameysharp tested this and while it does have some performance benefits, they are minimal.

This also removes some edges such as cranelift-jit not supporting test names which makes for a better experience using it.

Implementation

I started working on this a few days ago, but I'm putting it on hold to fix some other more prioritary issues. (in progress branch) If anyone wants to pick this up in the mean time feel free!

The branch above starts by changing the parser to produce a side table of test names while assigning UserExernalNames to each function.

Following from that we need to switch the testsuite to use this new side table.

And after that we should be able to remove the TestName from cranelift.

This sounds simple, but I think it will end up being quite a big change.

Alternatives

Do nothing. This has been the state of cranelift for a long time.

I think this would also break the interface for pretty much every downstream user since we would probably remove UserFuncName and replace it with UserExernalName directly.

Edit: Another alternative would be to expand this, and start supporting string function names, which would also be an interesting option.

Last updated: Nov 22 2024 at 16:03 UTC