-
Notifications
You must be signed in to change notification settings - Fork 40
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
RFC: prettify the default output #85
Conversation
I just updated the PR based on our discussion:
Now two questions for you:
Thanks! |
I've copied the branch to my repo. I intend to work on this once my schedule frees up a bit. It's not high priority but nice to have. To answer your question: lets keep the default as it currently is (no prettification). There are many people using jsonlite in production and I don't want to them to experience sudden performance regressions because things get prettified by default after updating. I'm not sure your colleagues will agree with "we will almost surely use a wrapper function in shiny/htmlwidgets and we can change the default there". I think in the context of shiny performance is more important than prettyness, but that's up to you guys. |
Prettiness does not make much sense in the context of shiny, but it does makes some sense in htmlwidgets, e.g. I often read the git diff's of the HTML pages generated from htmlwidgets to make sure what exactly I changed. I understand this may not be of high priority. Please take your time. Thanks! |
@jeroenooms I have completed this PR by rewriting the two R functions in C. I have tested them as thoroughly as I could. You can also see that the PR passes Please let me know what else you want. Thanks! |
Thanks! Can you run some performance benchmarks of your branch vs the current version? Doesn't have to overly fine grained, something like this: library(microbenchmark)
library(jsonlite)
data(flights, package="nycflights13")
data(diamonds, package="ggplot2")
microbenchmark (times = 10,
toJSON(diamonds, dataframe = "rows"),
toJSON(diamonds, dataframe = "columns"),
toJSON(diamonds, dataframe = "values"),
toJSON(diamonds, dataframe = "rows", pretty = TRUE),
toJSON(diamonds, dataframe = "columns", pretty = TRUE),
toJSON(diamonds, dataframe = "values", pretty = TRUE),
toJSON(flights, dataframe = "rows"),
toJSON(flights, dataframe = "columns"),
toJSON(flights, dataframe = "values"),
toJSON(flights, dataframe = "rows", pretty = TRUE),
toJSON(flights, dataframe = "columns", pretty = TRUE),
toJSON(flights, dataframe = "values", pretty = TRUE)
) |
Running some quick benchmarks myself, it looks like your version is faster for |
Okay, let me try. |
It does not seem to help much to have separate C functions Current CRAN version:
Current PR:
|
I just optimized it a bit, and the speed is closer. Not sure if you are satisfied. |
I don't understand where the performance overhead is coming from for the case of |
I don't understand it either. There isn't much difference in the performance for So what do you expect me to do next? :) |
SEXP out = PROTECT(allocVector(STRSXP, 1)); | ||
SET_STRING_ELT(out, 0, mkCharCE(olds, CE_UTF8)); | ||
UNPROTECT(1); | ||
free(olds); |
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.
I think you also need to free sp
and sp2
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.
I cannot free them since they were not malloc'd. It will abort R if I do free them.
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.
I'm really confused by c_spaces
- you malloc()
but you don't free()
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.
I see. You are probably right.
… of pretty = TRUE, which now means prettifying in R instead of yajl; if one prefers the latter, use pretty = 'yajl' or pretty = A_NUMBER
although it is not necessary for collapse_array to create sp and sp2 when inner = TRUE
Perfect. Thanks Jeroen! |
As mentioned in #84, neither the pretty nor unpretty output looks good to me. I feel it is a much better job to be done on the R side instead of handing it over to yajl, since yajl does not know the data structure information in R.
What I did was pretty straightforward:
indent
argument toasJSON()
, andindent = indent + 2
very timeasJSON()
is called on arrays, lists, and data frames;inner
argument tocollapse()
, so we know when to add line breaks (only add line breaks in the outer loops of collapsing);Since my C-fu is weak, I'm using the R versions of
collapse()
andcollapse_object()
. If you prefer doing collapsing in C, please feel free to do so. I imagine it is not going to be terribly hard. (Or ispaste()
really that slow?)Some examples:
library(jsonlite)