-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Json2 (count): pre define buffer capacity to avoid memory copy and allocation #20920
Conversation
This reverts commit 96336a2.
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.
The new Count
struct and its related API, does not need to be public.
The tests for it are very repetitive. The test file can also be an internal one (starting with module json2
), thus avoiding exposing the helper Count struct.
I thought this might be useful in cases like mut count := json.Count{0}
count.count_chars(val)
mut request_buffer := 'POST / HTTP/1.1\r\nContent-Length: ${count.get_total()}\r\n\r\n'.bytes()
request_buffer.ensure_cap( request_buffer.len + count.get_total())
encoder.encode_value(val, mut request_buffer)! What do you think? |
I think, that we already have io reader/writer interfaces. I also think that making something public, if there is a need for it in the future, justified by actual code and actual measurements in concrete situations, is easy. In contrast, turning a public API to a private one, is much harder, costing time for deprecation, to minimize the breaking change. I also think, that the cost for both maintainers and users, of a non essential, inconvenient API is high. |
You rock! Thanks for the review. |
Gains around 20% for now in
vlib/v/tests/bench/bench_json_vs_json2.v
;benchmark to verify impact when using push_many
TODO
chars_in_struct
that should be working (This is breaking the CI)