-
Notifications
You must be signed in to change notification settings - Fork 76
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
use StringBuilder instead of Buffer in builtin and json package #550
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
d8ff2d2
to
a9099f6
Compare
a9099f6
to
fdb1350
Compare
fdb1350
to
e3e92cc
Compare
Pull Request Test Coverage Report for Build 3372Details
💛 - Coveralls |
e3e92cc
to
ddb5956
Compare
ddb5956
to
56ee2ec
Compare
"array_nonjs_test.mbt": ["not", "js"] | ||
"array_nonjs_test.mbt": ["not", "js"], | ||
"stringbuilder_buffer.mbt": ["not", ["js"]], | ||
"stringbuilder_concat.mbt": ["js"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark todo: add wasm-gc
here once we have support for jsstring proposal on wasm-gc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have some concrete data points to justify the change?
I was supposed the StringBuilder is a faster (in js backend) and safer abstraction than Buffer. The Byte/Buffer is more suitable to writing binary stuff, programmers use it at their own risk: the But in this PR I wonder why we still want to introduce a new abstraction. What functionalities should StringBuilder and Buffer provide separately? |
As you said, Buffer is more suitable to writing binary stuff, and StringBuilder only cares string concatenation. Buffer is not always the best approach to concat strings on each targets. It's too slow compared to concat based approach on js platform even wasm-gc platform if we support JsStringBuiltins proposal. I'll present some data result later to show that. |
The test result use this code: https://gist.github.com/hackwaly/24c2c16e25ef660772f3b06625ddb7a4 On my machine. Under js target: buffer based cost 5.7s. concat based cost 1.25s. @bobzhang This PR also reduces simple |
I was wrong because the invalid UTF16 string (contains unpaired surrogate) can still produced by |
Probably not. It will align with the javascript's behavior. I think validation should be opt-in for users. It's best placed in unicode package? |
1fc0514
to
a52df69
Compare
From the provided
Overall, these observations highlight the importance of ensuring consistency, optimizing performance, and thoroughly testing new functionalities or changes in the codebase. By addressing these issues, the code can become more robust, efficient, and less prone to bugs. |
By replace the internal StringBuilder previously used in json package. The performance |
a52df69
to
06c281d
Compare
Per discussions offline, we agree to make a reasonably fast StringBuilder package on all platform.
|
Thus StringBuilder doesn't depend on Buffer
06c281d
to
ce042d8
Compare
Now |
This PR introduce StringBuilder in builtin package. Thus avoid expensive Buffer operations when use
Show
trait in js/wasm-gc target.