> -----Original Message-----
> From: stefan2_at_apache.org [mailto:stefan2_at_apache.org]
> Sent: zaterdag 7 december 2013 10:31
> To: commits_at_subversion.apache.org
> Subject: svn commit: r1548823 - in
> /subversion/trunk/subversion/libsvn_fs_x: dag.c id.c id.h temp_serializer.c
> temp_serializer.h
>
> Author: stefan2
> Date: Sat Dec 7 09:30:42 2013
> New Revision: 1548823
>
> URL: http://svn.apache.org/r1548823
> Log:
> Add a pool member to the internal representation of FSX IDs and
> thus allow the ID vtable functions to perform actual data lookups
> in the future. The pool is the one used to allocate the ID.
With WC-NG we explicitly stopped adding a pool in this kind of structures for very good reasons. All these pool reference makes it harder and harder to properly allocate results. This pattern is far too sensitive to pool cleanup ordering problems
Using proper result and scratch pool (where the scratch pool is always the result pool or a descendant of the result pool or at least a pool that is cleaned up *before* the result pool), makes pool cleanup handling much easier to understand...
> This is a first step towards making FSX IDs leaner and smarter.
I would have assumed that an ID is constant... How can a constant value be smart?
It can be chosen smartly, but an id is an identifier not code.
> * subversion/libsvn_fs_x/id.h
> (svn_fs_x__id_deserialize): Add POOL parameter that is already
> available to calling functions.
>
> * subversion/libsvn_fs_x/id.c
> (fs_x__id_t): Extend internal ID representation.
> (svn_fs_x__id_eq): Adjust the amount of data to compare.
> (svn_fs_x__id_txn_create_root,
> svn_fs_x__id_create_root,
> svn_fs_x__id_txn_create,
> svn_fs_x__id_rev_create): Initialize the new struct member.
> (svn_fs_x__id_copy): Ditto, plus use a more efficient duplication func.
> (svn_fs_x__id_parse,
> svn_fs_x__id_deserialize): Initialize the new struct member.
>
> * subversion/libsvn_fs_x/temp_serializer.h
> (svn_fs_x__noderev_deserialize): Add POOL parameter as pass-through.
>
> * subversion/libsvn_fs_x/temp_serializer.c
> (deserialize_dir,
> svn_fs_x__noderev_deserialize,
> svn_fs_x__deserialize_id,
> svn_fs_x__deserialize_node_revision,
> svn_fs_x__extract_dir_entry,
> deserialize_change,
> svn_fs_x__deserialize_changes): Provide everyone who needs it with
> a POOL parameter.
>
> * subversion/libsvn_fs_x/dag.c
> (svn_fs_x__dag_deserialize): Update API caller.
>
> Modified:
> subversion/trunk/subversion/libsvn_fs_x/dag.c
> subversion/trunk/subversion/libsvn_fs_x/id.c
> subversion/trunk/subversion/libsvn_fs_x/id.h
> subversion/trunk/subversion/libsvn_fs_x/temp_serializer.c
> subversion/trunk/subversion/libsvn_fs_x/temp_serializer.h
>
> Modified: subversion/trunk/subversion/libsvn_fs_x/dag.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/da
> g.c?rev=1548823&r1=1548822&r2=1548823&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_x/dag.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_x/dag.c Sat Dec 7 09:30:42 2013
> @@ -1152,10 +1152,11 @@ svn_fs_x__dag_deserialize(void **out,
> node->fs = NULL;
>
> /* fixup all references to sub-structures */
> - svn_fs_x__id_deserialize(node, &node->id);
> + svn_fs_x__id_deserialize(node, &node->id, pool);
> svn_fs_x__id_deserialize(node,
> - (svn_fs_id_t **)&node->fresh_root_predecessor_id);
> - svn_fs_x__noderev_deserialize(node, &node->node_revision);
> + (svn_fs_id_t **)&node->fresh_root_predecessor_id,
> + pool);
> + svn_fs_x__noderev_deserialize(node, &node->node_revision, pool);
> node->node_pool = pool;
>
> svn_temp_deserializer__resolve(node, (void**)&node->created_path);
>
> Modified: subversion/trunk/subversion/libsvn_fs_x/id.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/id.c
> ?rev=1548823&r1=1548822&r2=1548823&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_x/id.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_x/id.c Sat Dec 7 09:30:42 2013
> @@ -41,6 +41,8 @@ typedef struct fs_x__id_t
> svn_fs_x__id_part_t copy_id;
> svn_fs_x__id_part_t txn_id;
> svn_fs_x__id_part_t rev_item;
> +
> + apr_pool_t *pool; /* pool that was used to allocate this struct */
> } fs_x__id_t;
>
>
> @@ -286,7 +288,7 @@ svn_fs_x__id_eq(const svn_fs_id_t *a,
> return TRUE;
>
> return memcmp(&id_a->node_id, &id_b->node_id,
> - sizeof(*id_a) - sizeof(id_a->generic_id)) == 0;
> + 4 * sizeof(svn_fs_x__id_part_t)) == 0;
> }
This doesn't look like it properly handles alignment within the structure. (It probably works now, but the old code was far less sensitive for future changes to this struct)
Bert
>
>
> @@ -356,6 +358,7 @@ svn_fs_x__id_txn_create_root(const svn_f
>
> id->generic_id.vtable = &id_vtable;
> id->generic_id.fsap_data = &id;
> + id->pool = pool;
>
> return (svn_fs_id_t *)id;
> }
> @@ -371,6 +374,7 @@ svn_fs_id_t *svn_fs_x__id_create_root(co
>
> id->generic_id.vtable = &id_vtable;
> id->generic_id.fsap_data = &id;
> + id->pool = pool;
>
> return (svn_fs_id_t *)id;
> }
> @@ -390,6 +394,7 @@ svn_fs_x__id_txn_create(const svn_fs_x__
>
> id->generic_id.vtable = &id_vtable;
> id->generic_id.fsap_data = &id;
> + id->pool = pool;
>
> return (svn_fs_id_t *)id;
> }
> @@ -410,6 +415,7 @@ svn_fs_x__id_rev_create(const svn_fs_x__
>
> id->generic_id.vtable = &id_vtable;
> id->generic_id.fsap_data = &id;
> + id->pool = pool;
>
> return (svn_fs_id_t *)id;
> }
> @@ -419,10 +425,10 @@ svn_fs_id_t *
> svn_fs_x__id_copy(const svn_fs_id_t *source, apr_pool_t *pool)
> {
> fs_x__id_t *id = (fs_x__id_t *)source;
> - fs_x__id_t *new_id = apr_palloc(pool, sizeof(*new_id));
> + fs_x__id_t *new_id = apr_pmemdup(pool, id, sizeof(*id));
>
> - *new_id = *id;
> new_id->generic_id.fsap_data = new_id;
> + new_id->pool = pool;
>
> return (svn_fs_id_t *)new_id;
> }
> @@ -444,6 +450,7 @@ svn_fs_x__id_parse(const char *data,
> id = apr_pcalloc(pool, sizeof(*id));
> id->generic_id.vtable = &id_vtable;
> id->generic_id.fsap_data = &id;
> + id->pool = pool;
>
> /* Now, we basically just need to "split" this data on `.'
> characters. We will use svn_cstring_tokenize, which will put
> @@ -531,7 +538,9 @@ svn_fs_x__id_serialize(svn_temp_serializ
> /* Deserialize an ID inside the BUFFER.
> */
> void
> -svn_fs_x__id_deserialize(void *buffer, svn_fs_id_t **in_out)
> +svn_fs_x__id_deserialize(void *buffer,
> + svn_fs_id_t **in_out,
> + apr_pool_t *pool)
> {
> fs_x__id_t *id;
>
> @@ -549,5 +558,6 @@ svn_fs_x__id_deserialize(void *buffer, s
> /* the stored vtable is bogus at best -> set the right one */
> id->generic_id.vtable = &id_vtable;
> id->generic_id.fsap_data = id;
> + id->pool = pool;
> }
>
>
> Modified: subversion/trunk/subversion/libsvn_fs_x/id.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/id.
> h?rev=1548823&r1=1548822&r2=1548823&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_x/id.h (original)
> +++ subversion/trunk/subversion/libsvn_fs_x/id.h Sat Dec 7 09:30:42 2013
> @@ -161,11 +161,12 @@ svn_fs_x__id_serialize(struct svn_temp_s
> const svn_fs_id_t * const *id);
>
> /**
> - * Deserialize an @a id within the @a buffer.
> + * Deserialize an @a id within the @a buffer and associate it with @a pool.
> */
> void
> svn_fs_x__id_deserialize(void *buffer,
> - svn_fs_id_t **id);
> + svn_fs_id_t **id,
> + apr_pool_t *pool);
>
> #ifdef __cplusplus
> }
>
> Modified: subversion/trunk/subversion/libsvn_fs_x/temp_serializer.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/te
> mp_serializer.c?rev=1548823&r1=1548822&r2=1548823&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_x/temp_serializer.c (original)
> +++ subversion/trunk/subversion/libsvn_fs_x/temp_serializer.c Sat Dec 7
> 09:30:42 2013
> @@ -324,7 +324,7 @@ deserialize_dir(void *buffer, hash_data_
>
> /* pointer fixup */
> svn_temp_deserializer__resolve(entry, (void **)&entry->name);
> - svn_fs_x__id_deserialize(entry, (svn_fs_id_t **)&entry->id);
> + svn_fs_x__id_deserialize(entry, (svn_fs_id_t **)&entry->id, pool);
>
> /* add the entry to the hash */
> svn_hash_sets(result, entry->name, entry);
> @@ -364,7 +364,8 @@ svn_fs_x__noderev_serialize(svn_temp_ser
>
> void
> svn_fs_x__noderev_deserialize(void *buffer,
> - node_revision_t **noderev_p)
> + node_revision_t **noderev_p,
> + apr_pool_t *pool)
> {
> node_revision_t *noderev;
>
> @@ -378,8 +379,10 @@ svn_fs_x__noderev_deserialize(void *buff
> return;
>
> /* fixup of sub-structures */
> - svn_fs_x__id_deserialize(noderev, (svn_fs_id_t **)&noderev->id);
> - svn_fs_x__id_deserialize(noderev, (svn_fs_id_t **)&noderev-
> >predecessor_id);
> + svn_fs_x__id_deserialize(noderev, (svn_fs_id_t **)&noderev->id, pool);
> + svn_fs_x__id_deserialize(noderev,
> + (svn_fs_id_t **)&noderev->predecessor_id,
> + pool);
> svn_temp_deserializer__resolve(noderev, (void **)&noderev-
> >prop_rep);
> svn_temp_deserializer__resolve(noderev, (void **)&noderev->data_rep);
>
> @@ -687,7 +690,7 @@ svn_fs_x__deserialize_id(void **out,
> svn_fs_id_t *id = (svn_fs_id_t *)data;
>
> /* fixup of all pointers etc. */
> - svn_fs_x__id_deserialize(id, &id);
> + svn_fs_x__id_deserialize(id, &id, pool);
>
> /* done */
> *out = id;
> @@ -733,7 +736,7 @@ svn_fs_x__deserialize_node_revision(void
> node_revision_t *noderev = (node_revision_t *)buffer;
>
> /* fixup of all pointers etc. */
> - svn_fs_x__noderev_deserialize(noderev, &noderev);
> + svn_fs_x__noderev_deserialize(noderev, &noderev, pool);
>
> /* done */
> *item = noderev;
> @@ -891,7 +894,8 @@ svn_fs_x__extract_dir_entry(void **out,
> memcpy(new_entry, source, size);
>
> svn_temp_deserializer__resolve(new_entry, (void **)&new_entry-
> >name);
> - svn_fs_x__id_deserialize(new_entry, (svn_fs_id_t **)&new_entry->id);
> + svn_fs_x__id_deserialize(new_entry, (svn_fs_id_t **)&new_entry->id,
> + pool);
> *(svn_fs_dirent_t **)out = new_entry;
> }
>
> @@ -1085,7 +1089,9 @@ serialize_change(svn_temp_serializer__co
> * serialization CONTEXT.
> */
> static void
> -deserialize_change(void *buffer, change_t **change_p)
> +deserialize_change(void *buffer,
> + change_t **change_p,
> + apr_pool_t *pool)
> {
> change_t * change;
>
> @@ -1097,7 +1103,9 @@ deserialize_change(void *buffer, change_
> return;
>
> /* fix-up of sub-structures */
> - svn_fs_x__id_deserialize(change, (svn_fs_id_t **)&change-
> >info.node_rev_id);
> + svn_fs_x__id_deserialize(change,
> + (svn_fs_id_t **)&change->info.node_rev_id,
> + pool);
>
> svn_temp_deserializer__resolve(change, (void **)&change->path.data);
> svn_temp_deserializer__resolve(change, (void **)&change-
> >info.copyfrom_path);
> @@ -1177,7 +1185,8 @@ svn_fs_x__deserialize_changes(void **out
> for (i = 0; i < changes->count; ++i)
> {
> deserialize_change((void*)changes->changes,
> - (change_t **)&changes->changes[i]);
> + (change_t **)&changes->changes[i],
> + pool);
> APR_ARRAY_PUSH(array, change_t *) = changes->changes[i];
> }
>
>
> Modified: subversion/trunk/subversion/libsvn_fs_x/temp_serializer.h
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_x/te
> mp_serializer.h?rev=1548823&r1=1548822&r2=1548823&view=diff
> ==========================================================
> ====================
> --- subversion/trunk/subversion/libsvn_fs_x/temp_serializer.h (original)
> +++ subversion/trunk/subversion/libsvn_fs_x/temp_serializer.h Sat Dec 7
> 09:30:42 2013
> @@ -43,11 +43,13 @@ svn_fs_x__noderev_serialize(struct svn_t
> node_revision_t * const *noderev_p);
>
> /**
> - * Deserialize a @a noderev_p within the @a buffer.
> + * Deserialize a @a noderev_p within the @a buffer and associate it with
> + * @a pool.
> */
> void
> svn_fs_x__noderev_deserialize(void *buffer,
> - node_revision_t **noderev_p);
> + node_revision_t **noderev_p,
> + apr_pool_t *pool);
>
> /**
> * Serialize APR array @a *a within the serialization @a context.
>
Received on 2013-12-07 11:01:22 CET