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

fix: validate on demand at first for GetByPath and NewRaw #389

Merged
merged 15 commits into from
Mar 22, 2023
Merged

fix: validate on demand at first for GetByPath and NewRaw #389

merged 15 commits into from
Mar 22, 2023

Conversation

liuq19
Copy link
Collaborator

@liuq19 liuq19 commented Mar 20, 2023

To make the ast API safer, compatible with sonic early versions(< v1.7.0), and remain the optimization by SIMD skip.
We make sure that the raw jsons in ast.Node are always well-formed and valid as possible.

Main Changes:

  1. GetByPath: always validate the ondemand fields at first.
  2. GetByPath: check path, panic when the path is not string or integer(>=0)
  3. NewRaw: validate the input json, make sure all nodes' json is well-formed and valid.
goos: linux
goarch: amd64
pkg: github.com/bytedance/sonic/ast
cpu: Intel(R) Xeon(R) Platinum 8260 CPU @ 2.40GHz
                │ /tmp/tmplsy6pra1.source.txt │      /tmp/tmpnyhtp7f1.target.txt       │
                │           sec/op            │    sec/op     vs base                  │
GetFull_Sonic-8                   1.506µ ± 7%   24.492µ ± 3%  +1526.26% (p=0.000 n=10)
GetOne_Sonic-8                    2.522µ ± 8%    2.506µ ± 5%          ~ (p=0.579 n=10)
geomean                           1.949µ         7.834µ        +301.95%

                │ /tmp/tmplsy6pra1.source.txt │     /tmp/tmpnyhtp7f1.target.txt      │
                │             B/s             │     B/s       vs base                │
GetFull_Sonic-8                 8245.1Mi ± 6%   507.1Mi ± 3%  -93.85% (p=0.000 n=10)
GetOne_Sonic-8                   4.808Gi ± 7%   4.840Gi ± 5%        ~ (p=0.579 n=10)
geomean                          6.222Gi        1.548Gi       -75.12%

                │ /tmp/tmplsy6pra1.source.txt │     /tmp/tmpnyhtp7f1.target.txt     │
                │            B/op             │    B/op      vs base                │
GetFull_Sonic-8                   0.00 ± 0%     19.50 ± 18%        ? (p=0.000 n=10)
GetOne_Sonic-8                   24.00 ± 0%     46.50 ±  5%  +93.75% (p=0.000 n=10)
geomean                                     ¹   30.11        ?
¹ summaries must be >0 to compute geomean

                │ /tmp/tmplsy6pra1.source.txt │     /tmp/tmpnyhtp7f1.target.txt     │
                │          allocs/op          │ allocs/op   vs base                 │
GetFull_Sonic-8                  0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
GetOne_Sonic-8                   1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                     ²               +0.00%   

@liuq19 liuq19 changed the title fix: validate the on demand field at first fix: validate the on demand field at first for GetByPath and NewRaw Mar 22, 2023
@liuq19 liuq19 changed the title fix: validate the on demand field at first for GetByPath and NewRaw fix: validate on demand at first for GetByPath and NewRaw Mar 22, 2023
@AsterDY AsterDY merged commit 8639e93 into bytedance:main Mar 22, 2023
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.

2 participants