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

feat(javascript): Refactor & Compress Long #1313

Merged
merged 6 commits into from
Jan 6, 2024

Conversation

theweipeng
Copy link
Member

Refactor the code generator

  1. Currently, JavaScript can only register a Object by Type.object, because the generator is too simple
  2. Primitive generator of types like string、number can't be inlined in the holder collection, which cause necessary polymorphic that affect the performance

Enhancement performance

  1. Currently, Buffer.latin1Slice would transfer from native code generated by JIT to v8 runtime, This has a significant impact on performance, so we use String.fromCharCode to create little string when the string length is less than 15. It is a magic number, but I have tested it and it works fine.

Improve

When useSliceString is disabled, the performance of deserialization improves by 100%. It is 3 times faster than native JSON and twice as fast as protobuf.

When useSliceString is enabled, the performance of deserialization improves by 30%. It is 5 times faster than native JSON and 3 times faster than protobuf.

Before:

Disable useSliceString

image

Enable useSliceString

image

After:

Disable useSliceString

image

Enable useSliceString

image

@theweipeng theweipeng requested a review from chaokunyang January 6, 2024 08:16
@theweipeng theweipeng force-pushed the compress_u64 branch 2 times, most recently from 4c3d57a to 8284cd5 Compare January 6, 2024 08:23
@theweipeng
Copy link
Member Author

@bytemain Hi, could you please take a review, It remove the limit which mentioned in #1304

@bytemain
Copy link
Contributor

bytemain commented Jan 6, 2024

it seem that this is not a problem, all the call will happen in this closure


image

I checked the snapshot and donot sure is this a problem?

@bytemain
Copy link
Contributor

bytemain commented Jan 6, 2024

fantasitic work, this is the thing I wanted of a serialization framework

@theweipeng
Copy link
Member Author

it seem that this is not a problem, all the call will happen in this closure

image I checked the snapshot and donot sure is this a problem?

This is not a problem, but declare the readInner before the read is better. I will change it

@bytemain
Copy link
Contributor

bytemain commented Jan 6, 2024

@bytemain Hi, could you please take a review, It remove the limit which mentioned in #1304

I think you can copy some test case in the #1304:

@bytemain
Copy link
Contributor

bytemain commented Jan 6, 2024

Looks good to me, but has some typo.

And we can improve the style of generated code, this can be done in further

javascript/packages/fury/lib/classResolver.ts Outdated Show resolved Hide resolved
javascript/packages/fury/lib/gen/index.ts Outdated Show resolved Hide resolved
javascript/packages/fury/lib/type.ts Show resolved Hide resolved
@theweipeng
Copy link
Member Author

Looks good to me, but has some typo.

And we can improve the style of generated code, this can be done in further

I fixed it and added an afterCodeGenerated hook to allow users to format the code. Formatting is not a default action, as I believe we should keep things simple and avoid including too many dependencies.

@PragmaTwice PragmaTwice changed the title [JavaScript] Refactor & Compress Long feat(JavaScript): Refactor & Compress Long Jan 6, 2024
@PragmaTwice PragmaTwice changed the title feat(JavaScript): Refactor & Compress Long feat(javascript): Refactor & Compress Long Jan 6, 2024
Copy link
Collaborator

@chaokunyang chaokunyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@chaokunyang chaokunyang merged commit 44cfbb7 into apache:main Jan 6, 2024
18 checks passed
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