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: Thrift Compact protocol support #19

Open
wants to merge 44 commits into
base: main
Choose a base branch
from

Conversation

ii64
Copy link
Member

@ii64 ii64 commented Apr 4, 2023

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 added 30 commits March 28, 2023 00:19
- WARNING: cgocheck always disabled
- Rename conflict symbol names with autogenerated CGo C-header
  (prologue)
    - GoSlice  -> _GoSlice
    - GoString -> _GoString
    - GoEface  -> _GoEface
    - GoIface  -> _GoIface
- Added `sharedlib` build tag
- Added vscode workspace sample
    - tasks, launch list
    - build tag
@ii64
Copy link
Member Author

ii64 commented Apr 4, 2023

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
Copy link
Member Author

ii64 commented Apr 10, 2023

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

@AsterDY
Copy link
Collaborator

AsterDY commented Apr 12, 2023

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

@AsterDY
Copy link
Collaborator

AsterDY commented Apr 12, 2023

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

@ii64
Copy link
Member Author

ii64 commented Apr 12, 2023

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

@AsterDY
Copy link
Collaborator

AsterDY commented Apr 13, 2023

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

@ii64
Copy link
Member Author

ii64 commented Apr 13, 2023

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! )

@AsterDY
Copy link
Collaborator

AsterDY commented Apr 27, 2023

LGTM, when can it be ready for review

@ii64
Copy link
Member Author

ii64 commented Apr 30, 2023

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 ii64 marked this pull request as ready for review April 30, 2023 08:19
@ii64
Copy link
Member Author

ii64 commented Dec 20, 2023

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

@AsterDY
Copy link
Collaborator

AsterDY commented Dec 21, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants