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

Faster JSON parsing #583

Closed
m-muecke opened this issue Nov 14, 2024 · 7 comments
Closed

Faster JSON parsing #583

m-muecke opened this issue Nov 14, 2024 · 7 comments
Labels
feature a feature request or enhancement
Milestone

Comments

@m-muecke
Copy link
Contributor

m-muecke commented Nov 14, 2024

Since the yyjsonr package is on CRAN now, I wanted to check the openess of having another option for parsing the JSON in req_body_json() with yyjsonr, since it seems to be quite a bit faster.

Quick benchmark:

str <- yyjsonr::write_json_str(iris)
bench::mark(
  jsonlite::fromJSON(str),
  yyjsonr::read_json_str(str)
)
#> # A tibble: 2 × 6
#>   expression                       min   median `itr/sec` mem_alloc `gc/sec`
#>   <bch:expr>                  <bch:tm> <bch:tm>     <dbl> <bch:byt>    <dbl>
#> 1 jsonlite::fromJSON(str)      284.5µs  298.6µs     3298.    1.29MB    16.5 
#> 2 yyjsonr::read_json_str(str)   26.9µs   27.9µs    34519.    9.69KB     3.45

Created on 2024-11-14 with reprex v2.1.1

@hadley hadley added the feature a feature request or enhancement label Dec 19, 2024
@hadley
Copy link
Member

hadley commented Dec 19, 2024

I'm slightly worried that this might introduce subtle parsing changes, but it's probably going to be ok. I'll just need to think about how to parameterise and what should be the default.

@hadley hadley added this to the v1.1.0 milestone Dec 19, 2024
@hadley
Copy link
Member

hadley commented Dec 20, 2024

  • jsonlite is already in suggests, so adding yyjsonr there too would be fine.
  • It looks like the parsing is different enough that I don't think we can just swap (i.e. yyjsonr doesn't seem to have an equivalent of auto_unbox.)
  • If we do this all in one function, we'd probably need to add a parser argument and an options argument then deprecate auto_unbox, digits, and null and then create some helper that's the equivalent of opts_read_json.
  • Does that suggest it's better to use a suffix? resp_body_json_jsonlite() vs resp_body_json_yyjsonr(). Those are long function names.
  • Would be nice to use either read_json_raw() or read_json_file().

@m-muecke
Copy link
Contributor Author

m-muecke commented Dec 20, 2024

There is an auto_unbox equivalent in yyjsonr, in case you mean the following:

x <- list(x = 1, y = 1:10)
yyjsonr::write_json_str(x, opts = yyjsonr::opts_write_json(auto_unbox = TRUE))
#> [1] "{\"x\":1.0,\"y\":[1,2,3,4,5,6,7,8,9,10]}"

Created on 2024-12-20 with reprex v2.1.1

Integers would have to be explicitly set or set via the digits argument.

@hadley
Copy link
Member

hadley commented Dec 20, 2024

@m-muecke that's for writing, not reading though.

@hadley
Copy link
Member

hadley commented Dec 20, 2024

Oh which is what we are doing here, duh.

@hadley
Copy link
Member

hadley commented Dec 20, 2024

I was thinking about resp_body_json(). Which do also need to think about.

@hadley
Copy link
Member

hadley commented Dec 24, 2024

All in all, I think given the difficulties of making this change on global basis, compared to using yyjsonr for your specific API, I don't think it's worth changing httr2 at this time. But I appreciate you bringing this up and thanks for using httr2!

@hadley hadley closed this as completed Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants