On 23/09/2020 05:55, Vinnie Falco wrote:

On Tue, Sep 22, 2020 at 1:13 AM Gavin Lambert wrote:

...

Regarding the SAX parser specifically, I dislike the *_part methods in the callback interface. I feel that these should be removed and only the final form retained, called only when the complete text of the value has been determined.

The parser (and serializer) support incremental / streaming operation. To provide the "complete" text would require allocating memory and saving the partial result until enough input has been supplied to finish the text. basic_parser does not currently allocate memory, which was a design goal, and this would change that.

Not necessarily. A small buffer can be statically allocated for a single value-in-progress. It's trivial for all numeric values (except perhaps pathological precision, but that can be ignored). It's harder for string values; that might have to support dynamic allocation, it's true. I just don't see the point in having an interface that can return half of a value -- this just moves the problem of allocation and keeping track of the full value to the parser handler, which seems like an SRP violation.

...

I don't feel it is ever sensible to call back with a partially-parsed value (unless I am misunderstanding what these are for, but there is little explanation in the docs, and I wasn't able to get them called at all in my test).

In order to see partial results, you have to invoke the parser with partial JSON. In other words, break up the input JSON into two or more consecutive buffers of characters, and supply them in order.

I tried that, but the parse simply failed at the end of the first partial buffer and it completely ignored the second buffer. Hence my statement that basic_parser did not operate incrementally. Though see below; this appears to be a failure of example rather than of implementation.

...

I also don't see the point of supplying a size argument to on_key* and on_string, as this can be read from the string_view argument instead.

Right, so this size argument is actually the total number of characters, while the string_view::size is the number of characters in the current piece.

Which is a poor design choice, as I said.

...

parser.write() is incremental but basic_parser.write() is not, which seems odd. I assume this means that parser is storing an incrementally populated buffer of the full document rather than _really_ incrementally parsing.

Everything is incremental. parser::write() has the condition that if there are any leftover characters after receiving a complete JSON, an error is returned. Contrast this with parser::write_some which does not produce an error in this case. basic_parser::write is more similar to parser::write_some. We will be renaming it to basic_parser::write_some, as another reviewer suggested, to make this more clear. There is also a synergy with asio's naming with that change.

I wrote a print_parser (which is just like the null_parser example but prints the method calls and parameters) and invoked it like so: json::error_code ec; print_parser parser; std::string data1 = R"j({"foo":14,"bar":1)j"; std::string data2 = R"j(8,"baz":"bo)j"; std::string data3 = R"j(b"})j"; parser.write(&data1[0], data1.size(), ec); parser.write(&data2[0], data2.size(), ec); parser.write(&data3[0], data3.size(), ec); It stopped after calling on_int64(1, "1") and did not continue parsing, completely ignoring the second buffer, never calling any _part methods and reporting the wrong integer value. It parses correctly only if both are supplied. Hence, as I said, it is not behaving incrementally. However I have since noticed that the missing link is that it must pass 'true' as the 'more' parameter to basic_parser.write, which is not demonstrated in any example; it may be helpful to show that. (In fact this resolves my other concern about multi-document parsing as well, which also could use an explicit example.) Once this is done, then (as expected by the docs) it reports these calls (in part): on_number_part("1") on_int64(18, "8") on_string_part("bo", 2) on_string("b", 3) So I reiterate that the number and string handling is inconsistent and these part methods are more confusing than helpful -- numbers are parsed into the complete value but strings are not, and neither give you the full text of the value (which may be important if the goal is to parse into an arbitrary-precision value type). I do regard this as a very poor design choice, but not sufficiently so to reject the library. (I still also regard SAX-style parsing itself as a poor design choice [vs pull parsing], due to poor composability, but that is not a failure of the library itself, except perhaps in offering it at all. But I can understand why.)

Well, yeah, that's exactly what it is. Consider a network server that delivers JSON to clients. We don't want a completion handler to be stuck serializing 100MB of JSON while another client is waiting who only needs 1MB serialized. The way to ensure fairness here is to serialize incrementally with some reasonable buffer size, say 32kb. The light client will finish in ~33 i/o cycles while the heavy client still makes progress.

I was assuming that a "deep" JSON (many nested objects) may have a different cost per output character than a wide but shallow one. But it's not a big deal.

To store the comments would mean storing them in the DOM, which would have a significant impact on performance as the space for everything is tightly controlled.

I don't see why. It's just an extra string associated with any value. FWIW, I don't think it needs to *precisely* round-trip the comments -- it's ok to convert comments to /**/ style (that's mandatory if the output isn't pretty-printed) and to consolidate all comments associated with one value to a single comment that's always output after that value (or perhaps have 'before' and 'after' slots). This should be much more manageable. As it stands, the library is completely useless for the case of writing a JSON config file intended for human use. It's ok if you don't want that to be a goal, but this should be made clear up front so that people don't waste time considering the library for use cases it's not intended for. (Though it would be preferable if you did support it.)

...

The basic_parser examples do not compile out of the box -- with the suggested json.hpp and src.hpp includes there are link errors with basic_parser symbols. Additionally including basic_parser.hpp resolves these but starts complaining about missing max_{key,string,array,object}_size members of the handler type, which are not documented. After these are supplied then it works as expected. I don't see why these should be needed, however.

Those max sizes should be documented in the handler exemplar:

https://master.json.cpp.al/json/ref/boost__json__basic_parser.html#json.ref....

The docs I am referring to are the ones referenced in the original pre-announcement post (although now I notice that the main announcement post has a different link): http://vinniefalco.github.io/doc/json/json/ref/boost__json__basic_parser.htm... This is also where the "using numbers" section is entirely blank.

The reason they are required is for optimization. Handling the limit in the basic_parser leads to performance gains.

I don't really understand why. basic_parser doesn't actually store any of the keys/values (especially as it doesn't merge value parts), so it should have no reason to impose any kind of limit. This just seems like it's doing an extra count/comparison that could be removed entirely, and removing it would naturally improve performance. Unless you're saying that moving the check to basic_parser improves the performance of parser, in which case: you're doing it wrong. Imposing restrictions on all users of basic_parser for something that only parser cares about is the wrong design choice.

On 23/09/2020 12:52, Vinnie Falco wrote:

It is the opposite of SRP violation. basic_parser is concerned only with parsing and not keeping track of the full value. The responsibility of tracking full values is moved to the caller. That way, basic_parser does not impose its opinions on how the full value should be stored. This was litigated in earlier posts if I recall (Peter?).

Except it does, because it parses into int/uint/double, which is inherently a storage decision. You might have a valid argument if it left that step to the handler to determine as well. (i.e. only providing on_number_part and on_number, just the text without any attempt to parse into a numeric type.) Making that change would probably make the arbitrary-precision-parsing people happier as well.

The interfaces are built up in layers. At the lowest layer you have basic_parser which concerns itself only with processing the input buffer and turning it into semantic components. It does _not_ have the responsibility of buffering input to present it as a larger component, that's the job of the handler. basic_parser is exposed to users so they can use their own schemes for dealing with the semantic components.

Above basic_parser, you have parser which uses value_stack. The value_stack does take responsibility for buffering input, and producing DOM values. As with basic_parser, both of these components are exposed to users so they can write their own layers on top.

My point is that this seems like the wrong choice of layers. As it stands, consumers should never care about the basic_parser layer, and instead will actually want a different layer (not currently exposed) that is between the two, which exposes complete values that are not in any DOM. The current form of basic_parser could still exist, if you like, but it should be made an implementation detail and the complete-value basic_parser actually exposed to consumers.

...

parser.write(&data1[0], data1.size(), ec); parser.write(&data2[0], data2.size(), ec); parser.write(&data3[0], data3.size(), ec);

You have to call basic_parser::reset in order to start parsing a new "document."

This is not parsing new documents, but new document fragments. As such it must not reset.

No it is not a poor design choice, it is the correct design choice. If basic_parser were to buffer the input, then users who have no need of buffered input (such as value_stack) would be paying for it for nothing. This interface is driven first and foremost for performance and the incremental/streaming functionality. If callers want to see full text, they can append the partial strings into a data member until the final on_string or on_int64 / on_uint64 / on_double and then interact with it that way. But they are not obliged to do so.

There should never be any such consumers. Partial values are not useful to anyone. If they exist in the library, that is because things are structured improperly.

If we were to follow your "correct design" to make basic_parser buffer the input, the implementation would be needlessly allocating memory and copying characters.

It has to happen somewhere, regardless. Why are you pushing that reponsibility onto the consumer?

And I think this is the right choice. The library makes common things easy, e.g. call parse(). If you need more complexity, you instantiate a `parser` whose API requires a few more steps. If you really want to go low level, you use `basic_parser` which being low level can seem esoteric.

And I think this is okay, because the percentage of users who will use basic_parser is incredibly tiny. The percentage of `parser` users much greater. While the percentage of users of the parse() free function will be closer to 100% than anything else. In other words the library favors performance and utility at the lowest levels over theoretical interface correctness, where the number of users is naturally smaller (and their domain-expertise naturally higher).

So why provide basic_parser at all, if its interface is deliberately incorrect?

Le mercredi 23 septembre 2020 à 14:08 +1200, Gavin Lambert via Boost a écrit :

Mere moments ago, quoth I:

...

On 23/09/2020 12:52, Vinnie Falco wrote:

...

It is the opposite of SRP violation. basic_parser is concerned only with parsing and not keeping track of the full value. The responsibility of tracking full values is moved to the caller. That way, basic_parser does not impose its opinions on how the full value should be stored. This was litigated in earlier posts if I recall (Peter?).

Except it does, because it parses into int/uint/double, which is inherently a storage decision. You might have a valid argument if it left that step to the handler to determine as well. (i.e. only providing on_number_part and on_number, just the text without any attempt to parse into a numeric type.)

It's not just a storage *decision*; in order to implement this interface with the observed behaviour it also means that basic_parser must currently actually have *storage* for partial numbers updated across multiple fragments. This is inconsistent with how it treats keys and strings.

The base inconsistency is the fact that storing a number, whatever large it is, takes a fixed amount of storage (talking about doubles or longs, which are the only types handled by boost.json), whereas for a string it is not the case. A non-allocating parser can safely handle integers and double itself (and it can do it in a more efficient way than if it's done in an upper layer), but has no other choice than splitting strings (unless there is an upper limit on string size).

I still think that it would be better to instead provide an interface that precomposes the text fragments, but that is a separate argument from the prior one, that can be treated separately.

Perhaps you could provide both -- basic_parser as-is (but with the number parsing/storage removed), and another layer on top (custom_parser, perhaps) that is nearly identical but omits the _part methods to only provide precomposed keys/values. This is the one that most consumers of a SAX-style interface would be expected to actually use.

From what i've seen, such an interface would be quite easy to implement. That would break the purpose of the non-allocating parser, though, so i'm not sure it would be that useful.

Regards, Julien

Le 2020-09-23 10:17, Gavin Lambert via Boost a écrit :

On 23/09/2020 18:31, Julien Blanc wrote:

And if anything, it should improve performance (by not doing it when not needed); I don't see why you think it'd be less efficient in an upper layer.

The number would need to be parsed twice: first in the json parser, to check it is a valid number, secondly in the handler, to give an actual number. Given that some json parsing libraries implements their own number parsing to avoid this double parsing, constructing the value on the fly, i’m not alone in thinking it makes a difference (from what i've seen, boost::json authors also are on that boat, which makes their interface choice a logical one). Regards, Julien

On Tue, Sep 29, 2020 at 6:27 PM Gavin Lambert via Boost wrote:

As such I strongly recommend that json::basic_parser be turned into an implementation detail and not part of the public-facing part of the library.

Does anyone else agree? Thanks

On Wed, Sep 30, 2020 at 8:30 AM Peter Dimov wrote:

I don't. If you don't like basic_parser, don't use it. It's not clear to me what anyone would gain from preventing others from using it.

If Boost.JSON is accepted and we are targeting December ship date, I am weakly in favor of making basic_parser private for one release so we can work out more details. We might need to tune the API to achieve the space savings we want. Thanks

On Wed, 30 Sep 2020 at 17:15, Vinnie Falco via Boost wrote:

On Tue, Sep 29, 2020 at 6:27 PM Gavin Lambert via Boost

wrote:

...

As such I strongly recommend that json::basic_parser be turned into an

...

implementation detail and not part of the public-facing part of the

...

library.

Does anyone else agree?

Emphatically not. Without basic_parser it would not be possible to parse directly to struct and avoid building a redundant DOM.

Thanks

_______________________________________________

Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

--

Richard Hodges hodges.r@gmail.com office: +442032898513 home: +376841522 mobile: +376380212

On Tue, Sep 22, 2020 at 7:48 PM Gavin Lambert via Boost < boost@lists.boost.org> wrote:

On 23/09/2020 05:55, Vinnie Falco wrote:

...

...

I also don't see the point of supplying a size argument to on_key* and on_string, as this can be read from the string_view argument instead.

Right, so this size argument is actually the total number of characters, while the string_view::size is the number of characters in the current piece.

Which is a poor design choice, as I said.

How? The goal is to provide as much information as possible to the handler. Forcing users to track it themselves would introduce redundancy.

I don't really understand why. basic_parser doesn't actually store any

...

of the keys/values (especially as it doesn't merge value parts), so it should have no reason to impose any kind of limit. This just seems like it's doing an extra count/comparison that could be removed entirely, and removing it would naturally improve performance.

basic_parser only checks the limit; it does not enforce it. The handler is responsible for determining what that limit is. I suppose that we could add a little TMP that elides the check statically, but I don't see too much of a point in doing so. On Tue, Sep 22, 2020 at 7:48 PM Gavin Lambert via Boost < boost@lists.boost.org> wrote:

On 23/09/2020 05:55, Vinnie Falco wrote:

...

On Tue, Sep 22, 2020 at 1:13 AM Gavin Lambert wrote:

...

Regarding the SAX parser specifically, I dislike the *_part methods in the callback interface. I feel that these should be removed and only the final form retained, called only when the complete text of the value has been determined.

The parser (and serializer) support incremental / streaming operation. To provide the "complete" text would require allocating memory and saving the partial result until enough input has been supplied to finish the text. basic_parser does not currently allocate memory, which was a design goal, and this would change that.

Not necessarily. A small buffer can be statically allocated for a single value-in-progress. It's trivial for all numeric values (except perhaps pathological precision, but that can be ignored). It's harder for string values; that might have to support dynamic allocation, it's true.

I just don't see the point in having an interface that can return half of a value -- this just moves the problem of allocation and keeping track of the full value to the parser handler, which seems like an SRP violation.

...

...

I don't feel it is ever sensible to call back with a partially-parsed value (unless I am misunderstanding what these are for, but there is little explanation in the docs, and I wasn't able to get them called at all in my test).

In order to see partial results, you have to invoke the parser with partial JSON. In other words, break up the input JSON into two or more consecutive buffers of characters, and supply them in order.

I tried that, but the parse simply failed at the end of the first partial buffer and it completely ignored the second buffer. Hence my statement that basic_parser did not operate incrementally. Though see below; this appears to be a failure of example rather than of implementation.

...

...

I also don't see the point of supplying a size argument to on_key* and on_string, as this can be read from the string_view argument instead.

Right, so this size argument is actually the total number of characters, while the string_view::size is the number of characters in the current piece.

Which is a poor design choice, as I said.

...

...

parser.write() is incremental but basic_parser.write() is not, which seems odd. I assume this means that parser is storing an incrementally populated buffer of the full document rather than _really_ incrementally parsing.

Everything is incremental. parser::write() has the condition that if there are any leftover characters after receiving a complete JSON, an error is returned. Contrast this with parser::write_some which does not produce an error in this case. basic_parser::write is more similar to parser::write_some. We will be renaming it to basic_parser::write_some, as another reviewer suggested, to make this more clear. There is also a synergy with asio's naming with that change.

I wrote a print_parser (which is just like the null_parser example but prints the method calls and parameters) and invoked it like so:

json::error_code ec; print_parser parser; std::string data1 = R"j({"foo":14,"bar":1)j"; std::string data2 = R"j(8,"baz":"bo)j"; std::string data3 = R"j(b"})j"; parser.write(&data1[0], data1.size(), ec); parser.write(&data2[0], data2.size(), ec); parser.write(&data3[0], data3.size(), ec);

It stopped after calling on_int64(1, "1") and did not continue parsing, completely ignoring the second buffer, never calling any _part methods and reporting the wrong integer value. It parses correctly only if both are supplied. Hence, as I said, it is not behaving incrementally.

However I have since noticed that the missing link is that it must pass 'true' as the 'more' parameter to basic_parser.write, which is not demonstrated in any example; it may be helpful to show that. (In fact this resolves my other concern about multi-document parsing as well, which also could use an explicit example.)

Once this is done, then (as expected by the docs) it reports these calls (in part):

on_number_part("1") on_int64(18, "8")

on_string_part("bo", 2) on_string("b", 3)

So I reiterate that the number and string handling is inconsistent and these part methods are more confusing than helpful -- numbers are parsed into the complete value but strings are not, and neither give you the full text of the value (which may be important if the goal is to parse into an arbitrary-precision value type).

I do regard this as a very poor design choice, but not sufficiently so to reject the library.

(I still also regard SAX-style parsing itself as a poor design choice [vs pull parsing], due to poor composability, but that is not a failure of the library itself, except perhaps in offering it at all. But I can understand why.)

...

Well, yeah, that's exactly what it is. Consider a network server that delivers JSON to clients. We don't want a completion handler to be stuck serializing 100MB of JSON while another client is waiting who only needs 1MB serialized. The way to ensure fairness here is to serialize incrementally with some reasonable buffer size, say 32kb. The light client will finish in ~33 i/o cycles while the heavy client still makes progress.

I was assuming that a "deep" JSON (many nested objects) may have a different cost per output character than a wide but shallow one. But it's not a big deal.

...

To store the comments would mean storing them in the DOM, which would have a significant impact on performance as the space for everything is tightly controlled.

I don't see why. It's just an extra string associated with any value.

FWIW, I don't think it needs to *precisely* round-trip the comments -- it's ok to convert comments to /**/ style (that's mandatory if the output isn't pretty-printed) and to consolidate all comments associated with one value to a single comment that's always output after that value (or perhaps have 'before' and 'after' slots). This should be much more manageable.

As it stands, the library is completely useless for the case of writing a JSON config file intended for human use. It's ok if you don't want that to be a goal, but this should be made clear up front so that people don't waste time considering the library for use cases it's not intended for. (Though it would be preferable if you did support it.)

...

...

The basic_parser examples do not compile out of the box -- with the suggested json.hpp and src.hpp includes there are link errors with basic_parser symbols. Additionally including basic_parser.hpp resolves these but starts complaining about missing max_{key,string,array,object}_size members of the handler type, which are not documented. After these are supplied then it works as expected. I don't see why these should be needed, however.

Those max sizes should be documented in the handler exemplar:

< https://master.json.cpp.al/json/ref/boost__json__basic_parser.html#json.ref....

The docs I am referring to are the ones referenced in the original pre-announcement post (although now I notice that the main announcement post has a different link):

http://vinniefalco.github.io/doc/json/json/ref/boost__json__basic_parser.htm...

This is also where the "using numbers" section is entirely blank.

...

The reason they are required is for optimization. Handling the limit in the basic_parser leads to performance gains.

I don't really understand why. basic_parser doesn't actually store any of the keys/values (especially as it doesn't merge value parts), so it should have no reason to impose any kind of limit. This just seems like it's doing an extra count/comparison that could be removed entirely, and removing it would naturally improve performance.

Unless you're saying that moving the check to basic_parser improves the performance of parser, in which case: you're doing it wrong. Imposing restrictions on all users of basic_parser for something that only parser cares about is the wrong design choice.

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

On Wed, Sep 23, 2020 at 1:40 AM Gavin Lambert via Boost wrote:

I've walked back on that one a little bit; I now think that it's ok for basic_parser to do that (to retain a zero-allocation guarantee) but in that case it should treat numbers the same way (which it currently doesn't), and ideally there should be a standard slightly-higher-level parser that has a similar interface but does preassemble the values, eliminating the _part callbacks (for people who prefer ease of use over raw performance).

I had that originally, it was called number_parser, it was a public interface, and it was one of the first things to go during our 3-month journey of optimization. You can fish it out of the commit log if you care to see it. Thanks

On 23/09/2020 13:56, Hadriel Kaplan wrote:

Out of curiosity, why would you want to round-trip the comments from a config file?

Wouldn’t you want to write the comments anew each time you write the file, in case your code version had newer or corrected info? (I’m assuming the comments are like “// the following setting enables self-destruct (default=false)”)

I would think you’d want such “comments” to come from a schema, not the DOM. (Of course that would require schema-based serialization, or some form of serialization control, but that’s a separate topic)

No, I wasn't thinking of comments as software-provided documentation. But that could still work if the serialiser set the comment before writing the file. Though I guess it'd have to be more careful if there were human-specified comments as well.

Or do you want the human to be able to change comments and/or write their own, that get saved back? We have something like that at my job, but it’s a first-class/normal citizen in the DOM, using normal json-object string field entries named “description”. That way users can set descriptions for config objects regardless whether they edit config files by hand, or use a GUI, or whatever.

Yes, I was envisaging the human to be the one creating the comments, either as notes for themselves, or more commonly commenting out a particular setting because they don't want it to take effect but don't want to actually delete it. You can work around these things in strict JSON by altering the key name (or using a reserved key name, as you suggested), and that's fine, provided that it round-trips unknown keys. But it's more awkward than comments.