[RFC] jsond - Externals

9 years ago by Jakub Zelenka — view source

unread

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:

parsed at the top level: 07, 0xff, .1, -.1
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

9 years ago by Leigh — view source

unread

I feel that this was a bug in the new parser and we must conform to RFC
7159 !

I agree. +1

9 years ago by Jakub Zelenka — view source

unread

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)... :)

9 years ago by Paul Dragoonis — view source

unread

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:

parsed at the top level: 07, 0xff, .1, -.1

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 ?

Cheers

Jakub

9 years ago by Jakub Zelenka — view source

unread

Hi,

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

9 years ago by Andrea Faulds — view source

unread

Hey Jakub,

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/

9 years ago by Xinchen Hui — view source

unread

Hey:

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:

parsed at the top level: 07, 0xff, .1, -.1

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/

9 years ago by Jakub Zelenka — view source

unread

Hey

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

9 years ago by Xinchen Hui — view source

unread

Hey:

Hey

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/

9 years ago by Jakub Zelenka — view source

unread

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

9 years ago by Xinchen Hui — view source

unread

Hey:

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/

9 years ago by Pierre Joye — view source

unread

Hey:

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

9 years ago by Remi Collet — view source

unread

-----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

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlSzYssACgkQYUppBSnxahjnzwCgtLMbdlQ196QhzwObD5lDdyk8
8XcAoNhmwtT9LGCteUYREHX3d6J+YPec
=DPvv
-----END PGP SIGNATURE

9 years ago by Jakub Zelenka — view source

unread

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