Greg Stein <gstein@lyra.org> writes:
> > * 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?
Well, I was getting ready to write up a proposal for you, but since
you've remarked on the commit, I'll respond inline. Actually, my
instinct is that the need for per-file pools is related to the *lack*
of a pool passed to the editor's apply_textdelta() function.
> >...
> > /* 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.
Yep. Thanks.
> >...
> > + {
> > + 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.
Ding! You've summarized the proposal I was going to write up. :-)
> Or even more extreme: use a custom pool allocator to get around the 8k
> minimum used by the default allocator.
I'm not qualified to comment on the pros/cons of this.
> >...
> > + /* 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 :-) ]
In other words, lose the cast?
> >...
> > +++ 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;
>
Ah. Okay.
> > + 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 *'.
Gotcha.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Apr 15 20:28:37 2002