Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC] jsond »

[RFC] jsond

php.internals

Jakub Zelenka

11 years ago
Hi, This rfc about replacing json with jsond is under discussion. https://wiki.php.net/rfc/jsond I have finally done some benchmarks. You can find a link in the RFC. It's been extensively tested for the last few months and there is just one BC that I'm aware of. The new parser accepts only float number that conforms to RFC 7159 . This is also not consistent between decoding float number that are and are not in the array/object ( there are two conversion rules). The following formats are rejected: 1. parsed at the top level: 07, 0xff, .1, -.1 2. parser by JSON_Parser: [1.], [1.e1] I feel that this was a bug in the new parser and we must conform to RFC 7159 ! Cheers Jakub

Leigh

11 years ago
On 11 January 2015 at 17:27, Jakub Zelenka <bukka@php.net> wrote:
> > I feel that this was a bug in the new parser and we must conform to RFC > 7159 ! >
I agree. +1

Jakub Zelenka

11 years ago
On Sun, Jan 11, 2015 at 5:27 PM, Jakub Zelenka <bukka@php.net> wrote:
> > I feel that this was a bug in the new parser and we must conform to RFC > 7159 ! >
Typo! I meant of course that it was a bug in the old parser (the current json extension)... :)

Paul Dragoonis

11 years ago
On 11 Jan 2015 17:28, "Jakub Zelenka" <bukka@php.net> wrote:
> > Hi, > > This rfc about replacing json with jsond is under discussion. > > https://wiki.php.net/rfc/jsond > > I have finally done some benchmarks. You can find a link in the RFC. > > It's been extensively tested for the last few months and there is just one > BC that I'm aware of. > > The new parser accepts only float number that conforms to RFC 7159 . This > is also not consistent between decoding float number that are and are not > in the array/object ( there are two conversion rules). The following > formats are rejected: > > 1. parsed at the top level: 07, 0xff, .1, -.1 > 2. parser by JSON_Parser: [1.], [1.e1] > > I feel that this was a bug in the new parser and we must conform to RFC > 7159 !
I was just thinking about your jsond work just yesterday actually. I feel if we need to make a minor BC break to get this in, then go with it for PHP 7. I do think that if it's possible for you to detect when users supply this invalid float input that you would throw an informative exception to help people with the transition over to PHP7. What's the rest of the team's thoughts on this ?

Jakub Zelenka

11 years ago
Hi, On Sun, Jan 11, 2015 at 7:07 PM, Paul Dragoonis <dragoonis@gmail.com> wrote:
> > I feel if we need to make a minor BC break to get this in, then go with it > for PHP 7. >
Yes, it's proposed only for PHP 7 as declared in the RFC
> I do think that if it's possible for you to detect when users supply this > invalid float input that you would throw an informative exception to help > people with the transition over to PHP7. >
Let's first show an example and make clear what the whole thing: http://3v4l.org/2D72Q As you can see, the first type (07, 0xff, .1 and -.1) examples are really inconsistent and give a different results if there are in the array (error) or as a scalar. This seems like an obvious bug. The type 2 is consistent. The only problem is that such number format does not conform to RFC 7159 where the number is specified in https://tools.ietf.org/html/rfc7159#section-6 : number = [ minus ] int [ frac ] [ exp ] frac = decimal-point 1*DIGIT As you can see there is required a digit after decimal point... In this case we have a non conforming parser which is not right IMHO. About the special error messages. I could add a new error codes to identify these problem. However I am not sure that the current json error codes gives a useful information that would help with migration. I plan to look on error reporting later and create a different RFC if this one gets in. I have got an already implementation for bison locations in my testing scanner and parser ( you can see some logic in https://github.com/bukka/jso/blob/master/src/jso_parser.y#L160 ) that I used as an inspiration for jsond. The idea is to provide full reporting that includes location where the error is (line and column) and possibly text sample near to the error as well deeper identification of the error. It's not just this where we can give more info about the cause of the error. There is still some work and testing that will need to be done before I propose it however. Cheers Jakub

Andrew Faulds

11 years ago
Hey Jakub,
> On 11 Jan 2015, at 20:10, Jakub Zelenka <bukka@php.net> wrote: > > Let's first show an example and make clear what the whole thing: > http://3v4l.org/2D72Q > > As you can see, the first type (07, 0xff, .1 and -.1) examples are really > inconsistent and give a different results if there are in the array (error) > or as a scalar. This seems like an obvious bug. > > The type 2 is consistent. The only problem is that such number format does > not conform to RFC 7159 where the number is specified in > https://tools.ietf.org/html/rfc7159#section-6 : > > number = [ minus ] int [ frac ] [ exp ] > frac = decimal-point 1*DIGIT > > As you can see there is required a digit after decimal point... In this > case we have a non conforming parser which is not right IMHO.
It’s not even valid according to the train-track diagrams on json.org: http://json.org/number.gif Thanks for your work on this.
-- Andrea Faulds http://ajf.me/

Laruence

11 years ago
Hey: On Mon, Jan 12, 2015 at 1:27 AM, Jakub Zelenka <bukka@php.net> wrote:
> Hi, > > This rfc about replacing json with jsond is under discussion. > > https://wiki.php.net/rfc/jsond > > I have finally done some benchmarks. You can find a link in the RFC. > > It's been extensively tested for the last few months and there is just one > BC that I'm aware of. > > The new parser accepts only float number that conforms to RFC 7159 . This > is also not consistent between decoding float number that are and are not > in the array/object ( there are two conversion rules). The following > formats are rejected: > > 1. parsed at the top level: 07, 0xff, .1, -.1 > 2. parser by JSON_Parser: [1.], [1.e1] >
does master of josnd already works with PHP7? I was plan to benchmark the performance with wordpress, but seems it still not compileable with PHP7? thanks
> I feel that this was a bug in the new parser and we must conform to RFC > 7159 ! > > Cheers > > Jakub
-- Xinchen Hui @Laruence http://www.laruence.com/

Jakub Zelenka

11 years ago
Hey On Mon, Jan 12, 2015 at 3:01 AM, Xinchen Hui <laruence@php.net> wrote:
> > does master of josnd already works with PHP7? > >
There is a patch against PHP master that replaces current json ext. I referenced a branch of my PHP fork in RFC. I have just created a PR as well to make it a bit easier if you or others want to test it and possibly comment on it: https://github.com/php/php-src/pull/993 The jsond pecl ext does not compile on master. I want to keep as an alternative json extension for PHP 5 so it needs to compile on 5. There also are some extra things (e.g. alternative buffer implementation) that I didn't want to have as a part of this patch. I might make it compilable on both later but it's not a priority at the moment. Anyway the patch in the PR should work ok. If not, please let me know and I will fix it ;)
> I was plan to benchmark the performance with wordpress, > >
Great! Please let me know what the results are! ;) Cheers Jakub

Laruence

11 years ago
Hey: On Mon, Jan 12, 2015 at 6:24 PM, Jakub Zelenka <bukka@php.net> wrote:
> Hey > > On Mon, Jan 12, 2015 at 3:01 AM, Xinchen Hui <laruence@php.net> wrote: >> >> >> does master of josnd already works with PHP7? >> > > There is a patch against PHP master that replaces current json ext. I > referenced a branch of my PHP fork in RFC. > > I have just created a PR as well to make it a bit easier if you or others > want to test it and possibly comment on it: > > https://github.com/php/php-src/pull/993 > > The jsond pecl ext does not compile on master. I want to keep as an > alternative json extension for PHP 5 so it needs to compile on 5. There also > are some extra things (e.g. alternative buffer implementation) that I didn't > want to have as a part of this patch. I might make it compilable on both > later but it's not a priority at the moment. > > Anyway the patch in the PR should work ok. If not, please let me know and I > will fix it ;) > >> >> I was plan to benchmark the performance with wordpress, >> > > Great! Please let me know what the results are! ;)
Unfortunately, I don't see performance improvement in wordpress. even slowdown a bit. for IRs , it increased IR by 0.05% while run wordpress homepage for 100 times :< do you think there is some room to be improvement? thanks
> > Cheers > > Jakub >
-- Xinchen Hui @Laruence http://www.laruence.com/

Jakub Zelenka

11 years ago
On Mon, Jan 12, 2015 at 1:40 PM, Xinchen Hui <laruence@php.net> wrote:
> Unfortunately, I don't see performance improvement in wordpress. even > slowdown a bit. > > for IRs , it increased IR by 0.05% while run wordpress homepage for 100 > times :< > > do you think there is some room to be improvement? > > thanks >
To be honest I expected that there won't be a big difference unless json_decode is heavily used for bigger json instances. Anyway the decoder is not fully optimized yet and there definitely is a room for improvents. It's basically just an initial port for PHP 7. But I think that it's already good enough to replace the existing json ext as my benchmarks for instances containing large strings already gives much better results. And, of course, it is a PHP licensed parser... :) Cheers Jakub

Laruence

11 years ago
Hey: On Tue, Jan 13, 2015 at 2:06 AM, Jakub Zelenka <bukka@php.net> wrote:
> On Mon, Jan 12, 2015 at 1:40 PM, Xinchen Hui <laruence@php.net> wrote: >> >> Unfortunately, I don't see performance improvement in wordpress. even >> slowdown a bit. >> >> for IRs , it increased IR by 0.05% while run wordpress homepage for 100 >> times :< >> >> do you think there is some room to be improvement? >> >> thanks > > > To be honest I expected that there won't be a big difference unless > json_decode is heavily used for bigger json instances. > > Anyway the decoder is not fully optimized yet and there definitely is a room > for improvents. It's basically just an initial port for PHP 7. But I think > that it's already good enough to replace the existing json ext as my > benchmarks for instances containing large strings already gives much better > results. > > And, of course, it is a PHP licensed parser... :) >
sure, of course, :) thanks
> Cheers > > Jakub >
-- Xinchen Hui @Laruence http://www.laruence.com/

Pierre Joye

11 years ago
On Mon, Jan 12, 2015 at 6:34 PM, Xinchen Hui <laruence@php.net> wrote:
> Hey: > > On Tue, Jan 13, 2015 at 2:06 AM, Jakub Zelenka <bukka@php.net> wrote: >> On Mon, Jan 12, 2015 at 1:40 PM, Xinchen Hui <laruence@php.net> wrote: >>> >>> Unfortunately, I don't see performance improvement in wordpress. even >>> slowdown a bit. >>> >>> for IRs , it increased IR by 0.05% while run wordpress homepage for 100 >>> times :< >>> >>> do you think there is some room to be improvement? >>> >>> thanks >> >> >> To be honest I expected that there won't be a big difference unless >> json_decode is heavily used for bigger json instances. >> >> Anyway the decoder is not fully optimized yet and there definitely is a room >> for improvents. It's basically just an initial port for PHP 7. But I think >> that it's already good enough to replace the existing json ext as my >> benchmarks for instances containing large strings already gives much better >> results. >> >> And, of course, it is a PHP licensed parser... :) >> > sure, of course, :)
btw, it is important that this point alone justifies this RFC (and us accepting it as there is no impact on perf and could be better). We have an issue with the current extension due to the license of the underlying implementation. Many distros already use alternative implementations. To solve this problem will make everyone work easier.
-- Pierre @pierrejoye | http://www.libgd.org

Remi Collet

11 years ago
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Le 11/01/2015 18:27, Jakub Zelenka a écrit :
> Hi, > > This rfc about replacing json with jsond is under discussion. > > https://wiki.php.net/rfc/jsond >
+1 -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlSzYssACgkQYUppBSnxahjnzwCgtLMbdlQ196QhzwObD5lDdyk8 8XcAoNhmwtT9LGCteUYREHX3d6J+YPec =DPvv -----END PGP SIGNATURE-----

Jakub Zelenka

11 years ago
On Sun, Jan 11, 2015 at 5:27 PM, Jakub Zelenka <bukka@php.net> wrote:
> Hi, > > This rfc about replacing json with jsond is under discussion. > > https://wiki.php.net/rfc/jsond > > >
I have added a bit more info to the RFC about the BC (the same info that was discussed here) and also added few more links. In addition the jsond PR has been updated by merging changes for the preserving fractional part RFC that has been recently accepted. Thanks to Juan Basso for sending the patch. ;) Cheers Jakub