dynamicgo icon indicating copy to clipboard operation
dynamicgo copied to clipboard

WIP: Thrift Compact protocol support

Open ii64 opened this issue 2 years ago • 11 comments

What type of PR is this?

feat: Thrift Compact protocol support

What this PR does / why we need it (en: English/zh: Chinese):

en: support Thrift Compact protocol conversion zh: 支持 Thrift Compact 转化

Which issue(s) this PR fixes:

ii64 avatar Apr 04 '23 10:04 ii64

I'll add more test, there's a basic test and it's passing. I would like to know if the current state of native C codes are able to be converted to Go (via. clang macos ASM syntax and asm2asm).

ii64 avatar Apr 04 '23 12:04 ii64

Added some tests, Thrift encoder part is passing. There's an issue with some fallback mechanism especially for HTTP mapping.

ii64 avatar Apr 10 '23 07:04 ii64

Thanks for contributing! It will take some time to review it.

AsterDY avatar Apr 12 '23 02:04 AsterDY

It seems you use CGO for implementing j2t, which I concern about the performance. Can you give me some benchmark results?

AsterDY avatar Apr 12 '23 02:04 AsterDY

It seems you use CGO for implementing j2t, which I concern about the performance. Can you give me some benchmark results?

We are not going to use CGo for normal usage, only for development purpose, by adding "sharedlib" to Go build tag, it's kind of temporary replacement for asm2asm as I can't manage to make it work on Linux, it can also give you nice DX when debugging the native C code (which is dynamically linked with Go), compile the native using -ggdb3 can also give us source code line info

ii64 avatar Apr 12 '23 03:04 ii64

If so, I am afraid that its performance is even worse than using pure Golang... BTW, we have a plan to implement j2t in pure Golang for better compatibility. Maybe we can work together

AsterDY avatar Apr 13 '23 10:04 AsterDY

If so, I am afraid that its performance is even worse than using pure Golang...

To make it clear, we are still going to use asm2asm. I understand that there's an overhead of using CGo. The CGo dispatch is only an alternative for Linux user and used for development purpose. We are not going to use it to replace asm2asm generated code so that the end user are still get the performance they wanted. I haven't compare much with pure Golang code but given that LLVM can do more optimization toward the C code can help us get a better performance, isn't it?

As an example, on my observation, both j2t2_fsm_tb_exec (for TBinary) and j2t2_fsm_tc_exec (for TCompact) are calling j2t2_fsm_exec with providing a vTable containing specific Thrift protocol encoder methods, and since it's using __always_inline modifier, it's getting inlined, the vTable methods are also getting inlined so there's no indirect branching.

j2t2_fsm_tb_exec j2t2_fsm_tc_exec
image image

BTW, we have a plan to implement j2t in pure Golang for better compatibility. Maybe we can work together

Sure, no problem! )

ii64 avatar Apr 13 '23 16:04 ii64

LGTM, when can it be ready for review

AsterDY avatar Apr 27 '23 08:04 AsterDY

There are still some failing test for HTTP mapping path use-case, https://github.com/cloudwego/dynamicgo/blob/089a6d41bb09e503e03815c8e2d1d4a3666d2eec/conv/j2t/compact_test/failing_test.go I am not really sure on how it works with J2T FSM

ii64 avatar Apr 30 '23 08:04 ii64

I feel like this has been stale for a while. I am going to introduce smaller changes PR, this one look kinda massive. I'll start from making the native Thrift serde as vtables, then the next one is j2t Thrift Compact FSM or maybe HTTP conv on Go side

ii64 avatar Dec 20 '23 04:12 ii64

I feel like this has been stale for a while. I am going to introduce smaller changes PR, this one look kinda massive. I'll start from making the native Thrift serde as vtables, then the next one is j2t Thrift Compact FSM or maybe HTTP conv on Go side

Sure. You can try use sonic/ast.Visitor to implement j2t as well

AsterDY avatar Dec 21 '23 07:12 AsterDY