[OPEN-ILS-DEV] Optimizing uescape()

[Scott McKellar]

After taking a bit of a sabbatical to work on the election, I returned to
the Evergreen project a few days ago.  I was rummaging through utils.c
just to get back into the groove, and lit upon uescape().

This function reads a nul-terminated string, which may contain control
characters or Unicode, and encodes it as a series of printable characters.
Any unprintable characters in the input are expressed either as escaped
characters (such as "\n" and "\f") or in hex (such as "\u0024").

Currently we call it once in osrf_json_object.c and twice in
osrf_legacy_json.c.  The latter is likely dead code, or will be.

I saw an opportunity for a minor optimization by rearranging the order of
some of the character tests.  One thing led to another, and I found ways
to speed up the function severalfold, depending on the input string.

In the fullness of time I shall detail the optimizations.  For now I want
to discuss the signatures of the possible replacements for uescape().
Here is the signature of the current uescape() function, followed by the
signatures of two possible replacements:

    char* uescape( const char* string, int size, int full_escape );
    char* full_uescape( const char* string );
    growing_buffer* buffer_append_uescape( 
        growing_buffer* buf, const char* string );

The function names, of course, can change, but these will do for now.

-------------

Both replacement candidates eliminate the original size parameter.  In
all cases where we currently call uescape(), we determine the value of
size by calling strlen( string ).  We might as well call strlen() inside
the function, and thereby simplify the interface.

One might think that passing a size would enable us to handle strings with
embedded nul bytes.  In fact, however, the only thing uescape() uses the
size parameter for is to decide how big a growing_buffer to allocate for
internal use.  A nul in the input string is always treated as a delimiter,
not as data.

-------------

Both replacement candidates also eliminate the original full_escape
parameter, which is a boolean.  Internally we branch on it in order to
decide whether to encode control characters or leave them unchanged.

In all cases where we call uescape(), we pass a value of 1 for full_escape,
meaning that we encode control characters.  It's a useless parameter 
because in practice it makes no difference.

I don't like boolean parameters because I always have trouble remembering 
whether true means *this* and false means *that*, or the other way around.
If we ever need to provide the "false" behavior we should provide a
separate function for it, with a name that indicates how it differs. 

------------

full_uescape() is a drop-in replacement for uescape(), except that it
only takes one parameter, and (for reasons not discussed here) it's faster.

buffer_append_uescape() is different.

In every case where we currently call uescape(), we immediately append the
result to a growing_buffer.  Since uescape() uses its own growing_buffer
internally, we wind up creating and destroying a growing buffer every
time, incurring two mallocs and two frees.

By contrast, buffer_append_uescape() appends the escaped string directly
to an existing growing buffer, and returns a pointer to the modified
growing_buffer.  (If the input pointer to the growing_buffer is NULL, we
allocate a new growing_buffer).

As a result, buffer_append_uescape() can completely avoid any calls to 
malloc() or free() -- unless of course the buffer needs resizing, which
would have to be done in any case.

-------------

There are some issues to resolve concerning the treatment of errors.

If uescape() or full_uescape() encounters invalid Unicode, it returns
NULL.  The calling code can detect the error by checking for NULL, but
in fact it doesn't.  Without noticing or reporting the error, it just adds
nothing to the growing_buffer that it's building, even if some of the unescaped string was valid.

If that's a sensible way to respond to invalid Unicode, then I can make
buffer_append_uescape() behave the same way.

If we want to be able to detect invalid Unicode, then
buffer_append_uescape() will have to change.  Instead of returning a
pointer to the growing_buffer, it could return a status code.  In that 
case it would have to insist on getting a non-NULL pointer to the
growing_buffer.  That may be a wiser policy anyway.

-------------

My recommendation is to go with some form of buffer_append_uescape().
It incorporates all of the optimizations in full_uescape(), plus it
eliminates two mallocs and two frees, plus it doesn't have to do a
strlen() to guess at a buffer size.

We could also keep the original uescape() around, just in case we ever
need it again.  Otherwise we can reuse the same name for a function with
a different signature.

There are a number of different options.  I'd like some guidance before
I go much further.

Scott McKellar
http://home.swbell.net/mck9/ct/