[haiku-development] Re: Review; JSON Parser Changes
- From: waddlesplash <waddlesplash@xxxxxxxxx>
- To: Haiku Development ML <haiku-development@xxxxxxxxxxxxx>
- Date: Thu, 27 Apr 2017 21:41:15 -0400
On Thu, Apr 27, 2017 at 5:40 PM, Andrew Lindesay <apl@xxxxxxxxxxxxxx> wrote:
In in order to make some improvements in the HaikuDepotServer -
HaikuDepot space, I would like to change the Haiku-side JSON parsing so
that it is streaming rather than necessarily having to render to the
intermediate in-memory form of BMessage.
I have changed the arrangement with "Json.cpp" to use a flex parser with
a listener that can accept parse events. The existing behaviour is
supported by implementing a "BJsonMessageWriter" that renders to
BMessage.
Yes, I have a major question: *Why?*
What exactly is this needed for, really? If you're returning JSON payloads
so large that you should parse them incrementally rather than all in one
go, then your JSON payloads should really be broken up into multiple ones,
or you might be using JSON for something it's not designed for. But if all
you're doing is using a "next element"-style parsing method a la SAX or
QXmlStreamReader or whatever ... yeah, JSON was not designed from that, and
nothing sane uses that model for JSON parsing, mostly because there are
almost never >1MB JSON files (unlike XML where >25MB XML files are not
uncommon -- and even when those happen it's usually for game engines, not
app stores like HaikuDepot.) BJson was designed and created to be a system
to parse JSON files into BMessages, no more, no less. Even JavaScript
engines, where JSON originated, have no stream-based JSON parser built in.
And why a flex-based parser? What was wrong with the old one? Even if you
really needed the stream based parser for whatever reason, my handwritten
parser is pretty well structured such that it should not be difficult at
all to adapt it to stream parsing. So I honestly don't get the rationale
for getting rid of it completely and replacing it. If there's some testcase
you have that it fails to parse, I'd be quite happy to have a look at it.
So, to say I'm diametrically opposed to this change is possibly an
understatement. JSON is supposed to be relatively simple and easy to work
around and with. This looks like a complex beast of a parsing system not
unlike what finds in an XML parser written for XMPP -- and that is not a
good thing at all. Not including tests and the writer, it appears to be a
~2500 line diff, vs the current implementation which is ~350 lines. I see
no feasible benefits and a large increase in complexity and thus decrease
in maintainability.
The class "BJsonStringStreamWriter" will also be able to be
used to render JSON programmatically to a stream, but can also be
hooked-up as a listener to go full-circle and render out JSON from JSON.
A brief glance at this class seems OK to me; I don't like a good number of
the design choices and I'd much prefer a serializer to JSON built into
BMessage rather than a string serializer, but I guess that's a minor
quibble compared to what I have above.
-waddlesplash
Other related posts:
