Re: svn commit: rev 1718 - trunk/subversion/libsvn_client

On Mon, Apr 15, 2002 at 11:49:17AM -0500, cmpilato@tigris.org wrote:
> Author: cmpilato
> Date: 2002-04-15 16:49 GMT
> New Revision: 1718
>
> Modified:
> trunk/subversion/libsvn_client/commit.c
> trunk/subversion/libsvn_client/commit_util.c
> Log:
> Fix some serious memory issues in import and commit.
>
> * subversion/libsvn_client/commit.c
> (struct imported_file): New.
> (import_file): Add new imported_file structure to the hash instead
> of just the file baton. Also, do all file-related work inside a
> subpool of the hash's pool.
> (import): Do file work in the subpool associated with the file
> itself, and destroy that subpool after the file has been processed.

Each pool is a minimum of an 8k allocation (in the current implementation).
Thus, if you are committing 1000 files (and I understand what you're doing),
then this is a minimum of 8 megabytes.

It would seem that you'd want to allocate all the file information in a
single pool, and then use an iteration pool to process them (rather than a
pool associated with that file).

Is the need for a per-file pool related to the pool passed to the editor
interface?

>...
> /* Copy in the contents of the mod structure to the array. Note
> that this is NOT a copy of a pointer reference, but a copy of
> the structure's contents!! */
> - mod.item = item;
> - mod.file_baton = file_baton;
> - (*((struct file_mod_t *) apr_array_push (file_mods))) = mod;
> + mod->subpool = file_pool;
> + mod->item = item;
> + mod->file_baton = file_baton;
> + apr_hash_set (file_mods, item->url->data, item->url->len, (void *)mod);

The comment no longer applies to this code/algorithm.

>...
> + {
> + SVN_ERR (editor->close_file (file_baton));
> + svn_pool_destroy (file_pool);
> + }

This is why I suspect we may still need per-file pools. Hmm... Maybe
apply_textdelta really ought to take a working pool.

Or even more extreme: use a custom pool allocator to get around the 8k
minimum used by the default allocator.

>...
> + /* Get the next entry. */
> + apr_hash_this (hi, &key, &klen, (void **) &mod);

Strictly speaking, a 'void *' might have a different representation from any
other pointer value. And when a cast is performed to/from that 'void *', the
compiler will Do The Right Thing. Thus, it is (strictly) illegal to return a
'void *' pointer into 'something_else *'

[ I'm not sure which architectures might exhibit such a behavior, but Greg
  Hudson might now :-) ]

>...
> +++ trunk/subversion/libsvn_client/commit.c Mon Apr 15 11:49:17 2002
>...
> @@ -320,20 +330,16 @@
> {
> const void *key;
> apr_ssize_t keylen;
> - void *file_baton;
> + struct imported_file *value;
> svn_stringbuf_t *full_path;
> -
> - apr_hash_this (hi, &key, &keylen, &file_baton);
> - full_path = svn_stringbuf_create ((char *) key, subpool);
> - SVN_ERR (send_file_contents (full_path, file_baton, editor, subpool));
> - SVN_ERR (editor->close_file (file_baton));
> -
> - /* Clear our per-iteration subpool. */
> - svn_pool_clear (subpool);
> +
> + apr_hash_this (hi, &key, &keylen, (void **) &value);

A similar case here.

Not sure how strict we want to be, but these "should" look like:

    void *hashval;
    
    apr_hash_this (hi, &key, &keylen, &hashval);
    value = hashval;

> + full_path = svn_stringbuf_create ((char *) key, value->subpool);

The 'key' is noted as 'const void *', so the above would be 'const char *'.
But the cast isn't really needed because the compiler will perform an
implicit cast for the call, since it knows the argument is 'const char *'.

>...
> + /* Destroy the subpool (unless an error occurred, since we'll
> + need to keep the error around for a little while longer). */
> + if (! bump_err)
> + svn_pool_destroy (subpool);

Well, we put all the errors into one pool, but then Greg Hudson pointed out
the possible problems with that, so we were going to go back to the create-
a-pool-for-each-error algorithm (and put the svn_error_free() call back). It
looks like Karl hasn't unwound his change back to that state, though.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org