Let json:decode/3 keep whitespaces #8809
Conversation
CT Test Results 2 files 95 suites 57m 32s ⏱️ Results for commit 5b6e52e. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
dgud
force-pushed
the
dgud/stdlib/json-decode-keep-ws/OTP-19227
branch
from
c142d08
to
ceb3c11
Compare
|
@michalmuskala Can you take a look.. |
lib/stdlib/src/json.erl
Outdated
| @@ -1386,16 +1386,18 @@ object_key(_, Original, Skip, Acc, Stack, Decode) -> | |||
|
|
|||
| continue(<<Rest/bits>>, Original, Skip, Acc, Stack0, Decode, Value) -> | |||
| case Stack0 of | |||
| [] -> terminate(Rest, Original, Skip, Acc, Value); | |||
| [] -> terminate(Rest, Rest, Acc, Value); | |||
There was a problem hiding this comment.
This breaks the binary pattern optimisation since the tail is now used in two paces - I'm fairly sure it will significantly affect performance
There was a problem hiding this comment.
From what we could see, looking at the beam asm,
the only difference was that it creates a sub-binary in the case when Stack = [],
which should be ok since that is only for the last "term" in the json string.
But I'm not fluent in beam-asm.
There was a problem hiding this comment.
It looks fine, the optimization is disabled for this particular call but not the others. Should this become a performance problem, it's not a huge deal for the compiler to allow the reused Rest context to be passed alongside a new tail binary representing Rest.
There was a problem hiding this comment.
I pushed up a new variant that postpones the creation and only does when necessary.
rickard-green
added
team:VM
`json:decode/3` always stripped leading whitespaces in the `Rest` binary,
which could be problematic if user expected them.
E.g `json:decode(<<"foo\n bar">>, ok, #{})` returned:
`{<<"foo">>, ok, <<"bar">>}` instead of
`{<<"foo">>, ok, <<"\n bar">>}`.
If `Rest` only contains whitespaces they are removed, so that the user
can match on empty binary to know if they should continue the decoding loop.
E.g `json:decode(<<"foo\n ">>, ok, #{})` still returns:
`{<<"foo">>, ok, <<>>}`
dgud
force-pushed
the
dgud/stdlib/json-decode-keep-ws/OTP-19227
branch
from
ea95878
to
5b6e52e
Compare
json:decode/3always stripped leading whitespaces in theRestbinary, which could be problematic if user expected them.E.g
json:decode(<<"foo\n bar">>, ok, #{})returned:{<<"foo">>, ok, <<"bar">>}instead of{<<"foo">>, ok, <<"\n bar">>}.If
Restonly contains whitespaces they are removed, so that the user can match on empty binary to know if they should continue the decoding loop.E.g
json:decode(<<"foo\n ">>, ok, #{})still returns:{<<"foo">>, ok, <<>>}