Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhancement: compatibility with memory pools without realloc #12

Open
marcstern opened this issue Sep 29, 2023 · 1 comment
Open

Enhancement: compatibility with memory pools without realloc #12

marcstern opened this issue Sep 29, 2023 · 1 comment

Comments

@marcstern
Copy link

Some memory pool API (like Apache APR) don't implement a realloc function, so it's not possible to use these API with yajl. And it's totally impossible to develop a genric one.

A (quite simple) possibility is to allow to not specify a realloc function and, in this case, use an internal function. We can do that because in all calls to realloc, we know the old size (which we don't know in an external function).

Concretely:

Remove existing tests "afs->yajl_realloc == NULL"

Create the internal "extended" realloc:
void* yajl_realloc2(yajl_alloc_funcs* afs, void* previous, size_t sz, size_t oldsz)
{
void* new = afs->yajl_malloc(afs->ctx, sz);
if (!new) return NULL;
if (!previous) return new;
if (oldsz) memcpy(new, previous, oldsz);
afs->yajl_free(afs->ctx, previous);
return new;
}

Extend the definition of YA_REALLOC:
#define YA_REALLOC(afs, ptr, sz, oldsz) ((afs)->yajl_realloc ? (afs)->yajl_realloc((afs)->ctx, (ptr), (sz)) : yajl_realloc2((afs), (ptr), (sz), (oldsz)))

I attached a complete diff, tested with mod_security2 (APR).
For info, this speeds up the parsing by 250% on big JSON.

yajl_realloc2.zip

@robohack
Copy link
Owner

robohack commented Apr 7, 2024

I do like this idea. I have to think a bit harder still about whether or not it changes the API in any significant way. I don't think it does, but I haven't convinced myself 100% yet.

If we can conclude it is entirely backward compatible API and ABI wise then I think it could be included in the imminent next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants