[CC dev@ this time]
On Fri, Aug 31, 2018 at 3:20 PM Yann Ylavic <[email protected]> wrote:
>
> On Fri, Aug 31, 2018 at 2:41 PM Graham Leggett <[email protected]> wrote:
> >
> > On 31 Aug 2018, at 14:26, Yann Ylavic <[email protected]> wrote:
> >
> > > I wonder if we shouldn't change the apr_hash_t (currently used to
> > > handle JSON objects) to an apr_table_t, such that key doublons are not
> > > an issue (this isn't one in JSON specification AFAICT).
> > >
> > > Then we could get rid of 'klen' handling in several places (NUL
> > > terminated string is fine for keys IMHO, the question is more about
> > > strdup or not) while APR_JSON_FLAGS_STRICT would still do its job for
> > > overlays (and could be extended to apr_json_object_set w/ something
> > > like APR_JSON_FLAGS_MULTI, mutually exclusive).
> >
> > I wondered about this for a while, I’m keen to keep the code as it is, to
> > follow the ap_hash_t interface with key and key length as closely as
> > possible.
> >
> > When decoding JSON, the original strings are referenced in place, and this
> > saves both time and memory. If the strings became NUL terminated we would
> > have to strndup each one, which would slow us down and almost double the
> > memory footprint.
> >
>
> I was thinking of something like the attached patch.
> We don't really need to duplicate if not asked to (i.e. given len ==
> APR_JSON_VALUE_STRING).
> Wouldn't that work?
Actually this updated patch, rather.
There is no more copy with an apr_table than with an apr_hash (we
still point to the given key).
I also added apr_json_object_add() for explicitely allowing doublons,
while apr_json_object_set() still replaces as before, similarly to
apr_table_add() vs apr_table_set().
WDYT?
Regards,
Yann.
Index: include/apr_json.h
===================================================================
--- include/apr_json.h (revision 1839763)
+++ include/apr_json.h (working copy)
@@ -28,7 +28,6 @@
#include "apr.h"
#include "apr_pools.h"
#include "apr_tables.h"
-#include "apr_hash.h"
#include "apr_strings.h"
#include "apr_buckets.h"
@@ -169,7 +168,7 @@ struct apr_json_object_t {
/** The key value pairs in the object are in this list */
APR_RING_HEAD(apr_json_object_list_t, apr_json_kv_t) list;
/** JSON object */
- apr_hash_t *hash;
+ apr_table_t *table;
};
/**
@@ -198,12 +197,14 @@ APR_DECLARE(apr_json_value_t *) apr_json_value_cre
*
* @param pool The pool to allocate from.
* @param val The UTF-8 encoded string value.
- * @param len The length of the string, or APR_JSON_VALUE_STRING.
+ * @param len The length of the string, or APR_JSON_VALUE_STRING;
+ * the former duplicates (and NUL terminates) val up to
+ * the given len, while the latter uses the pointer only.
* @return The apr_json_value_t structure.
*/
-APR_DECLARE(apr_json_value_t *)
- apr_json_string_create(apr_pool_t *pool, const char *val,
- apr_ssize_t len) __attribute__((nonnull(1)));
+APR_DECLARE(apr_json_value_t *) apr_json_string_create(apr_pool_t *pool,
+ const char *val, apr_ssize_t len)
+ __attribute__((nonnull(1)));
/**
* Allocate and return a JSON array.
@@ -269,11 +270,9 @@ APR_DECLARE(apr_json_value_t *)
__attribute__((nonnull(1)));
/**
- * Associate a value with a key in a JSON object.
+ * Associate a value with a key in a JSON object, replacing doublon(s).
* @param obj The JSON object.
* @param key Pointer to the key.
- * @param klen Length of the key, or APR_JSON_VALUE_STRING if NUL
- * terminated.
* @param val Value to associate with the key.
* @param pool Pool to use.
* @return APR_SUCCESS on success, APR_EINVAL if the key is
@@ -281,22 +280,42 @@ APR_DECLARE(apr_json_value_t *)
* @remark If the value is NULL the key value pair is deleted.
*/
APR_DECLARE(apr_status_t) apr_json_object_set(apr_json_value_t *obj,
- const char *key, apr_ssize_t klen, apr_json_value_t *val,
- apr_pool_t *pool) __attribute__((nonnull(1, 2, 5)));
+ const char *key, apr_json_value_t *val, apr_pool_t *pool)
+ __attribute__((nonnull(1, 2, 4)));
/**
+ * Associate a value with a key in a JSON object, with possible doublon(s).
+ * @param obj The JSON object.
+ * @param key Pointer to the key.
+ * @param val Value to associate with the key.
+ * @param pool Pool to use.
+ * @return APR_SUCCESS on success, APR_EINVAL if the key is
+ * NULL or not a string, or the object is not an APR_JSON_OBJECT.
+ * @remark If the value is NULL the key value pair is deleted.
+ */
+APR_DECLARE(apr_status_t) apr_json_object_add(apr_json_value_t *obj,
+ const char *key, apr_json_value_t *val, apr_pool_t *pool)
+ __attribute__((nonnull(1, 2, 4)));
+
+/**
* Look up the value associated with a key in a JSON object.
* @param obj The JSON object.
* @param key Pointer to the key.
- * @param klen Length of the key, or APR_JSON_VALUE_STRING if NUL
- * terminated.
* @return Returns NULL if the key is not present.
*/
APR_DECLARE(apr_json_kv_t *) apr_json_object_get(apr_json_value_t *obj,
- const char *key, apr_ssize_t klen)
+ const char *key)
__attribute__((nonnull(1, 2)));
/**
+ * Size of a JSON object.
+ * @param obj The JSON object.
+ * @return Returns The number of associated values.
+ */
+APR_DECLARE(apr_size_t) apr_json_object_size(apr_json_value_t *obj)
+ __attribute__((nonnull(1)));
+
+/**
* Get the first value associated with an object.
*
* If the value is an object, this function returns the first key value pair.
Index: json/apr_json.c
===================================================================
--- json/apr_json.c (revision 1839779)
+++ json/apr_json.c (working copy)
@@ -45,18 +45,28 @@ apr_json_value_t *apr_json_object_create(apr_pool_
json->type = APR_JSON_OBJECT;
json->value.object = object = apr_pcalloc(pool, sizeof(apr_json_object_t));
APR_RING_INIT(&object->list, apr_json_kv_t, link);
- object->hash = apr_hash_make(pool);
+ object->table = apr_table_make(pool, 5);
return json;
}
apr_json_value_t *apr_json_string_create(apr_pool_t *pool, const char *val,
- apr_ssize_t len) {
+ apr_ssize_t len)
+{
apr_json_value_t *json = apr_json_value_create(pool);
if (json) {
if (val) {
json->type = APR_JSON_STRING;
+ if (len == APR_JSON_VALUE_STRING) {
+ len = strlen(val);
+ }
+ else {
+ val = apr_pstrmemdup(pool, val, len);
+ if (!val) {
+ return NULL;
+ }
+ }
json->value.string.p = val;
json->value.string.len = len;
} else {
@@ -129,18 +139,22 @@ apr_json_value_t *apr_json_null_create(apr_pool_t
return json;
}
-apr_status_t apr__json_object_set(apr_json_value_t *object,
- apr_json_value_t *key, apr_json_value_t *val, apr_pool_t *pool);
+apr_status_t apr__json_object_put(apr_json_value_t *object,
+ apr_json_value_t *key, apr_json_value_t *val,
+ int multi, apr_pool_t *pool);
-apr_status_t apr__json_object_set(apr_json_value_t *object,
- apr_json_value_t *key, apr_json_value_t *val, apr_pool_t *pool)
+apr_status_t apr__json_object_put(apr_json_value_t *object,
+ apr_json_value_t *key, apr_json_value_t *val,
+ int multi, apr_pool_t *pool)
{
- apr_json_kv_t *kv;
- apr_hash_t *hash;
+ apr_json_kv_t *kv = NULL;
+ apr_table_t *table;
- hash = object->value.object->hash;
+ table = object->value.object->table;
- kv = apr_hash_get(hash, key->value.string.p, key->value.string.len);
+ if (!multi) {
+ kv = (apr_json_kv_t *)apr_table_get(table, key->value.string.p);
+ }
if (!kv) {
kv = apr_palloc(pool, sizeof(apr_json_kv_t));
if (!kv) {
@@ -149,7 +163,12 @@ apr_json_value_t *apr_json_null_create(apr_pool_t
APR_RING_ELEM_INIT(kv, link);
APR_JSON_OBJECT_INSERT_TAIL(object->value.object, kv);
- apr_hash_set(hash, key->value.string.p, key->value.string.len, kv);
+ if (multi) {
+ apr_table_addn(table, key->value.string.p, (void *)kv);
+ }
+ else {
+ apr_table_setn(table, key->value.string.p, (void *)kv);
+ }
}
kv->k = key;
@@ -158,8 +177,10 @@ apr_json_value_t *apr_json_null_create(apr_pool_t
return APR_SUCCESS;
}
-apr_status_t apr_json_object_set(apr_json_value_t *object, const char *key,
- apr_ssize_t klen, apr_json_value_t *val, apr_pool_t *pool)
+static APR_INLINE
+apr_status_t object_put(apr_json_value_t *object,
+ const char *key, apr_json_value_t *val,
+ int multi, apr_pool_t *pool)
{
apr_json_value_t *k;
@@ -168,35 +189,56 @@ apr_json_value_t *apr_json_null_create(apr_pool_t
}
if (!val) {
- apr_hash_t *hash;
+ apr_table_t *table;
apr_json_kv_t *kv;
- hash = object->value.object->hash;
- kv = apr_hash_get(hash, key, klen);
+ table = object->value.object->table;
+ kv = (apr_json_kv_t *)apr_table_get(table, key);
if (kv) {
- apr_hash_set(hash, key, klen, NULL);
+ apr_table_unset(table, key);
APR_RING_REMOVE((kv), link);
}
return APR_SUCCESS;
}
- k = apr_json_string_create(pool, key, klen);
+ k = apr_json_string_create(pool, key, APR_JSON_VALUE_STRING);
if (!k) {
return APR_ENOMEM;
}
- return apr__json_object_set(object, k, val, pool);
+ return apr__json_object_put(object, k, val, multi, pool);
}
-apr_json_kv_t *apr_json_object_get(apr_json_value_t *object, const char *key, apr_ssize_t klen)
+apr_status_t apr_json_object_set(apr_json_value_t *object, const char *key,
+ apr_json_value_t *val, apr_pool_t *pool)
{
+ return object_put(object, key, val, 0, pool);
+}
+
+apr_status_t apr_json_object_add(apr_json_value_t *object, const char *key,
+ apr_json_value_t *val, apr_pool_t *pool)
+{
+ return object_put(object, key, val, 1, pool);
+}
+
+apr_json_kv_t *apr_json_object_get(apr_json_value_t *object, const char *key)
+{
if (object->type != APR_JSON_OBJECT) {
return NULL;
}
- return apr_hash_get(object->value.object->hash, key, klen);
+ return (apr_json_kv_t *)apr_table_get(object->value.object->table, key);
}
+apr_size_t apr_json_object_size(apr_json_value_t *object)
+{
+ if (object->type != APR_JSON_OBJECT) {
+ return 0;
+ }
+
+ return apr_table_elts(object->value.object->table)->nelts;
+}
+
apr_json_kv_t *apr_json_object_first(apr_json_value_t *obj)
{
apr_json_kv_t *kv;
@@ -318,11 +360,11 @@ apr_json_value_t *apr_json_overlay(apr_pool_t *p,
return overlay;
}
- oc = apr_hash_count(overlay->value.object->hash);
+ oc = apr_table_elts(overlay->value.object->table)->nelts;
if (!oc) {
return base;
}
- bc = apr_hash_count(base->value.object->hash);
+ bc = apr_table_elts(base->value.object->table)->nelts;
if (!bc) {
return overlay;
}
@@ -333,12 +375,9 @@ apr_json_value_t *apr_json_overlay(apr_pool_t *p,
kv != APR_RING_SENTINEL(&(base->value.object)->list, apr_json_kv_t, link);
kv = APR_RING_NEXT((kv), link)) {
- if (!apr_hash_get(overlay->value.object->hash, kv->k->value.string.p,
- kv->k->value.string.len)) {
-
- apr_json_object_set(res, kv->k->value.string.p,
- kv->k->value.string.len, kv->v, p);
-
+ if (!apr_table_get(overlay->value.object->table,
+ kv->k->value.string.p)) {
+ apr__json_object_put(res, kv->k, kv->v, 1, p);
}
else if (APR_JSON_FLAGS_STRICT & flags) {
return NULL;
@@ -350,9 +389,7 @@ apr_json_value_t *apr_json_overlay(apr_pool_t *p,
kv != APR_RING_SENTINEL(&(overlay->value.object)->list, apr_json_kv_t, link);
kv = APR_RING_NEXT((kv), link)) {
- apr_json_object_set(res, kv->k->value.string.p,
- kv->k->value.string.len, kv->v, p);
-
+ apr__json_object_put(res, kv->k, kv->v, 1, p);
}
return res;
Index: json/apr_json_decode.c
===================================================================
--- json/apr_json_decode.c (revision 1839779)
+++ json/apr_json_decode.c (working copy)
@@ -424,8 +424,9 @@ static apr_status_t apr_json_decode_array(apr_json
return status;
}
-apr_status_t apr__json_object_set(apr_json_value_t *object,
- apr_json_value_t *key, apr_json_value_t *val, apr_pool_t *pool);
+apr_status_t apr__json_object_put(apr_json_value_t *object,
+ apr_json_value_t *key, apr_json_value_t *val,
+ int multi, apr_pool_t *pool);
static apr_status_t apr_json_decode_object(apr_json_scanner_t * self,
apr_json_value_t *json, apr_json_object_t ** retval)
@@ -435,7 +436,7 @@ static apr_status_t apr_json_decode_object(apr_jso
apr_json_object_t *object = apr_pcalloc(self->pool,
sizeof(apr_json_object_t));
APR_RING_INIT(&object->list, apr_json_kv_t, link);
- object->hash = apr_hash_make(self->pool);
+ object->table = apr_table_make(self->pool, 5);
*retval = object;
@@ -503,7 +504,7 @@ static apr_status_t apr_json_decode_object(apr_jso
if ((status = apr_json_decode_value(self, &value)))
goto out;
- apr__json_object_set(json, key, value, self->pool);
+ apr__json_object_put(json, key, value, 1, self->pool);
if (self->p == self->e) {
status = APR_EOF;
Index: test/testjson.c
===================================================================
--- test/testjson.c (revision 1839763)
+++ test/testjson.c (working copy)
@@ -63,26 +63,26 @@ static void test_json_identity(abts_case * tc, voi
ABTS_INT_EQUAL(tc, len, offset);
ABTS_INT_EQUAL(tc, APR_JSON_OBJECT, json->type);
- image = apr_hash_get(json->value.object->hash, "Image", 5);
+ image = apr_json_object_get(json, "Image");
ABTS_PTR_NOTNULL(tc, image);
- width = apr_hash_get(image->v->value.object->hash, "Width", 5);
+ width = apr_json_object_get(image->v, "Width");
ABTS_PTR_NOTNULL(tc, width);
ABTS_INT_EQUAL(tc, APR_JSON_LONG, width->v->type);
ABTS_INT_EQUAL(tc, 800, width->v->value.lnumber);
- ids = apr_hash_get(image->v->value.object->hash, "IDs", 3);
+ ids = apr_json_object_get(image->v, "IDs");
ABTS_PTR_NOTNULL(tc, ids);
ABTS_INT_EQUAL(tc, APR_JSON_ARRAY, ids->v->type);
- title = apr_hash_get(image->v->value.object->hash, "Title", 5);
+ title = apr_json_object_get(image->v, "Title");
ABTS_PTR_NOTNULL(tc, title);
ABTS_INT_EQUAL(tc, APR_JSON_STRING, title->v->type);
- animated = apr_hash_get(image->v->value.object->hash, "Animated", 8);
+ animated = apr_json_object_get(image->v, "Animated");
ABTS_PTR_NOTNULL(tc, animated);
ABTS_INT_EQUAL(tc, APR_JSON_BOOLEAN, animated->v->type);
ABTS_TRUE(tc, !animated->v->value.boolean);
- thumbnail = apr_hash_get(image->v->value.object->hash, "Thumbnail", 9);
+ thumbnail = apr_json_object_get(image->v, "Thumbnail");
ABTS_PTR_NOTNULL(tc, thumbnail);
ABTS_INT_EQUAL(tc, APR_JSON_OBJECT, thumbnail->v->type);
- height = apr_hash_get(image->v->value.object->hash, "Height", 6);
+ height = apr_json_object_get(image->v, "Height");
ABTS_PTR_NOTNULL(tc, height);
ABTS_INT_EQUAL(tc, APR_JSON_LONG, height->v->type);
ABTS_INT_EQUAL(tc, 600, height->v->value.lnumber);
@@ -170,7 +170,7 @@ static void test_json_overlay(abts_case * tc, void
ABTS_INT_EQUAL(tc, APR_SUCCESS, status);
res = apr_json_overlay(p, overlay, base, APR_JSON_FLAGS_NONE);
- ABTS_INT_EQUAL(tc, 5, apr_hash_count(res->value.object->hash));
+ ABTS_INT_EQUAL(tc, 5, apr_json_object_size(res));
res = apr_json_overlay(p, overlay, base, APR_JSON_FLAGS_STRICT);
ABTS_ASSERT(tc, "overlay strict should return NULL",
