[OPEN-ILS-DEV] PATCH: osrf_json.c (rewrite of jsonObjectToJSON[Raw])

[Bill Erickson]

On Mon, 17 Nov 2008 23:32:21 -0500, Scott McKellar <mck9 at swbell.net> wrote:

> This patch is a rewrite of the jsonObjectToJSON and jsonObjectToJSONRaw  
> functions.  It is dependent on my previous patch to utils.c and utils.h,
> adding the new buffer_append_uescape function.
>
> One purpose is to replace a call to the uescape function with a call to
> the faster buffer_append_uescape function.  The other purpose is to
> introduce a faster way to translate a jsonObject into a string.
>
> (Also in one spot I broke up a very long string literal into several
> smaller pieces so that it wouldn't wrap around in the editor.)
>
> In the existing jsonObjectToJSON function, we receive a pointer to a
> jsonObject and return a string of JSON.  However we don't translate the
> original jsonObject directly.  Instead, we create a modified clone of the
> original, inserting an additional JSON_HASH node wherever we find a
> classname.  Then we translate the clone, and finally destroy it.
>
> It always struck me as an egregious waste to create and destroy a whole
> parallel object just so that we could turn it into a string.  So I looked
> for a way to eliminate the cloning.
>
> The result is a modification of add_json_to_buffer(), a local function
> that recursively traverses and translates the jsonObject.  When it sees a
> classname (and it has been asked to expand classnames), the new version
> inserts additional gibberish into the output text and then continues the
> traversal, without modifying or copying the original jsonObject at all.
>
> In my benchmark, this new approach was faster than the original by a
> factor of about 5.  When I combined this change with the use of the new
> buffer_append_uencode function, it was faster by a factor of about 7.2.
> The benchmark used a moderately complex jsonObject about 5 or 6 levels
> deep, with both hashes and arrays, with classnames at several levels.
> The performance gain will no doubt depend on the contents of the
> jsonObject,but I haven't tried to isolate the variables.
>
> The new version is a bit trickier and harder to read than the old.  In my
> opinion the speedup is worth the obscurity, because a lot of places in
> Evergreen will benefit.
>
> ----------------
>
> With this change, the jsonObjectEncodeClass function, which had been
> called only from jsonObjectToJSON, is no longer called from anywhere.
> It's the function that created the modified copy of the original
> jsonObject.  I see no reason to keep it around.  After a decent interval,
> if no one objects I shall submit a patch to eliminate it.

Agreed.

>
> Likewise the jsonObjectToJSONRaw function is no longer called from
> anywhere.  It translates a jsonObject to JSON without expanding  
> classnames.
> In this rewritten version it is identical to jsonObjectToJSON except that
> it calls add_json_to_buffer() with a different parameter value.
>
> If we eliminate jsonObjectToJSONRaw, I can simplify add_json_to_buffer()
> a bit.  However we might someday want to be able to translate a  
> jsonObject
> without expanding classnames.  If nobody minds, I'll leave this function
> where it is.

Also agreed.  We (or other code using OpenSRF) may need the Raw() version  
in the future.

This code path is used /a lot/, so the speedups are greatly appreciated.

I've run into one issue with these last 2 patches, in particular, there  
seems to be a subtle difference in uescape() and buffer_append_uescape().   
The majority of the JSON parsing succeeds, but parsing certain  
bibliographic data (MARC records) always results in NULL when attempting  
to encode the containing object (biblio_record entry objects).    If I  
replace the call to buffer_append_uescape() with a call to uescape() in  
osrf_json_object.c, then all is well.

I would send you some sample data to test, but I've yet to recreate the  
issue outside of a running system.  For now, I'm adding some logging to  
see if I can track it down.  I just wanted to pass this on in case any  
lightbulbs went off.  I'll let you if I find something...

Thanks, Scott

-b


-- 
Bill Erickson
| VP, Software Development & Integration
| Equinox Software, Inc. / The Evergreen Experts
| phone: 877-OPEN-ILS (673-6457)
| email: erickson at esilibrary.com
| web: http://esilibrary.com