You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2009/10/24 15:42:21 UTC
Re: svn commit: r40218 - trunk/subversion/svn
On Sat, Oct 24, 2009 at 08:35, Hyrum K. Wright <hy...@hyrumwright.org> wrote:
>...
> +++ trunk/subversion/svn/proplist-cmd.c Sat Oct 24 05:35:12 2009 (r40218)
>...
> else /* operate on normal, versioned properties (not revprops) */
> {
> - apr_pool_t *subpool = svn_pool_create(pool);
> + apr_pool_t *iterpool;
> svn_proplist_receiver_t pl_receiver;
>
> if (opt_state->xml)
> {
> - SVN_ERR(svn_cl__xml_print_header("properties", pool));
> + SVN_ERR(svn_cl__xml_print_header("properties", scratch_pool));
> pl_receiver = proplist_receiver_xml;
> }
> else
> @@ -181,6 +180,7 @@ svn_cl__proplist(apr_getopt_t *os,
> if (opt_state->depth == svn_depth_unknown)
> opt_state->depth = svn_depth_empty;
>
> + iterpool = svn_pool_create(scratch_pool);
> for (i = 0; i < targets->nelts; i++)
If you construct the iterpool in the declaration (as before), then you
can use it for the call to svn_cl__xml_print_header(), as its
scratch_pool. Any mem used by the call will be cleared on the first
iteration of the loop.
>...
> @@ -204,17 +204,16 @@ svn_cl__proplist(apr_getopt_t *os,
> opt_state->depth,
> opt_state->changelists,
> pl_receiver, &pl_baton,
> - ctx, subpool),
> + ctx, iterpool),
> NULL, opt_state->quiet,
> SVN_ERR_UNVERSIONED_RESOURCE,
> SVN_ERR_ENTRY_NOT_FOUND,
> SVN_NO_ERROR));
> }
> + svn_pool_destroy(iterpool);
>
> if (opt_state->xml)
> - SVN_ERR(svn_cl__xml_print_footer("properties", pool));
> -
> - svn_pool_destroy(subpool);
> + SVN_ERR(svn_cl__xml_print_footer("properties", scratch_pool));
> }
>
> return SVN_NO_ERROR;
And if you don't destroy it so soon, then you get to use it for that
last call, too.
>...
> +++ trunk/subversion/svn/resolve-cmd.c Sat Oct 24 05:35:12 2009 (r40218)
>...
> @@ -52,7 +52,7 @@ svn_cl__resolve(apr_getopt_t *os,
> svn_error_t *err;
> apr_array_header_t *targets;
> int i;
> - apr_pool_t *subpool;
> + apr_pool_t *iterpool;
There is a similar opportunity in this function.
>...
> +++ trunk/subversion/svn/resolved-cmd.c Sat Oct 24 05:35:12 2009 (r40218)
> @@ -44,48 +44,48 @@
> svn_error_t *
> svn_cl__resolved(apr_getopt_t *os,
> void *baton,
> - apr_pool_t *pool)
> + apr_pool_t *scratch_pool)
> {
> svn_cl__opt_state_t *opt_state = ((svn_cl__cmd_baton_t *) baton)->opt_state;
> svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
> svn_error_t *err;
> apr_array_header_t *targets;
> + apr_pool_t *iterpool;
> int i;
> - apr_pool_t *subpool;
>
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> - ctx, pool));
> + ctx, scratch_pool));
And obviously in this function, too.
>...
> +++ trunk/subversion/svn/status-cmd.c Sat Oct 24 05:35:12 2009 (r40218)
> @@ -170,23 +170,23 @@ print_status(void *baton,
> svn_error_t *
> svn_cl__status(apr_getopt_t *os,
> void *baton,
> - apr_pool_t *pool)
> + apr_pool_t *scratch_pool)
> {
> svn_cl__opt_state_t *opt_state = ((svn_cl__cmd_baton_t *) baton)->opt_state;
> svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
> apr_array_header_t *targets;
> - apr_pool_t *subpool;
> - apr_hash_t *master_cl_hash = apr_hash_make(pool);
> + apr_pool_t *iterpool;
> + apr_hash_t *master_cl_hash = apr_hash_make(scratch_pool);
> int i;
> svn_opt_revision_t rev;
> struct status_baton sb;
>
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> - ctx, pool));
> + ctx, scratch_pool));
And here, too!
>...
> @@ -267,9 +266,9 @@ svn_cl__status(apr_getopt_t *os,
> svn_stringbuf_t *buf;
>
> if (opt_state->xml)
> - buf = svn_stringbuf_create("", pool);
> + buf = svn_stringbuf_create("", scratch_pool);
>
> - for (hi = apr_hash_first(pool, master_cl_hash); hi;
> + for (hi = apr_hash_first(scratch_pool, master_cl_hash); hi;
> hi = apr_hash_next(hi))
> {
Seems that this function should be using the iterpool?
Tho some of the XML output may need to remain in scratch_pool or
something, but there are a couple cases to use iterpool.
>...
> + svn_pool_destroy(iterpool);
>
> - svn_pool_destroy(subpool);
> if (opt_state->xml && (! opt_state->incremental))
> - SVN_ERR(svn_cl__xml_print_footer("status", pool));
> + SVN_ERR(svn_cl__xml_print_footer("status", scratch_pool));
>
> return SVN_NO_ERROR;
> }
And to not destroy it so soon.
>...
> +++ trunk/subversion/svn/upgrade-cmd.c Sat Oct 24 05:35:12 2009 (r40218)
> @@ -42,37 +42,37 @@
> svn_error_t *
> svn_cl__upgrade(apr_getopt_t *os,
> void *baton,
> - apr_pool_t *pool)
> + apr_pool_t *scratch_pool)
> {
> svn_cl__opt_state_t *opt_state = ((svn_cl__cmd_baton_t *) baton)->opt_state;
> svn_client_ctx_t *ctx = ((svn_cl__cmd_baton_t *) baton)->ctx;
> apr_array_header_t *targets;
> - apr_pool_t *subpool;
> + apr_pool_t *iterpool;
> int i;
>
> SVN_ERR(svn_cl__args_to_target_array_print_reserved(&targets, os,
> opt_state->targets,
> - ctx, pool));
> + ctx, scratch_pool));
And here!
>...
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411052
Re: svn commit: r40218 - trunk/subversion/svn
Posted by Greg Stein <gs...@gmail.com>.
You don't know when scratch_pool will be cleared, and it is best to keep the
high water mark as low as possible. That means clearing (iter)pools, and it
means putting stuff it the most short-lived pool possible.
On Oct 26, 2009 5:06 PM, "Hyrum K. Wright" <hy...@mail.utexas.edu>
wrote:
On Oct 26, 2009, at 4:31 PM, Greg Stein wrote: > On Mon, Oct 26, 2009 at
04:32, Julian Foad <julia...
But if we already have a scratch_pool, why use the iterpool? *Both* are
going to be cleared Real Soon Now, and using the scratch_pool (over the
iterpool) fits better with our pool usage semantics.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411526
Re: svn commit: r40218 - trunk/subversion/svn
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On Oct 26, 2009, at 4:31 PM, Greg Stein wrote:
> On Mon, Oct 26, 2009 at 04:32, Julian Foad
> <ju...@btopenworld.com> wrote:
>> Greg Stein wrote:
>>> On Sat, Oct 24, 2009 at 18:42, Julian Foad <julianfoad@btopenworld.com
>>> > wrote:
>>>> Greg Stein wrote:
>>>>> If you construct the iterpool in the declaration (as before),
>>>>> then you
>>>>> can use it for the call to svn_cl__xml_print_header(), as its
>>>>> scratch_pool. Any mem used by the call will be cleared on the
>>>>> first
>>>>> iteration of the loop.
>> [...]
>>>>> And if you don't destroy it so soon, then you get to use it for
>>>>> that
>>>>> last call, too.
>>>> [...]
>>>>
>>>> That's true, there is the opportunity to do that, but is it
>>>> really worth
>>>> breaking the well-defined "iterpool" pattern for such a little
>>>> extra
>>>> optimisation? I don't think so. Keep it simple.
>>>
>>> Eh? How does it break it?
>>>
>>> You alloc the iterpool. clear it at the start of each iteration.
>>> destroy it later. I see no change to that.
>>
>> "The pool you use also helps readers of the code understand object
>> lifetimes. Is a given object used only during one iteration of the
>> loop,
>> or will it need to last beyond the end of the loop?"
>>
>> Quoted from <http://subversion.tigris.org/hacking.html#apr-pools>.
>
> Okay. So what? The iterpool is still an iterpool, used within the loop
> and cleared each time. How does allocating other objects in it "break"
> the pattern?
>
> (and yes, we use iterpools outside of the loop in plenty of other
> places; it makes a great scratch pool)
But if we already have a scratch_pool, why use the iterpool? *Both*
are going to be cleared Real Soon Now, and using the scratch_pool
(over the iterpool) fits better with our pool usage semantics.
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411525
Re: svn commit: r40218 - trunk/subversion/svn
Posted by Julian Foad <ju...@btopenworld.com>.
On Mon, 2009-10-26 at 11:31 -0400, Greg Stein wrote:
> On Mon, Oct 26, 2009 at 04:32, Julian Foad <ju...@btopenworld.com> wrote:
> > Greg Stein wrote:
> >> On Sat, Oct 24, 2009 at 18:42, Julian Foad <ju...@btopenworld.com> wrote:
> >> > Greg Stein wrote:
> >> >> If you construct the iterpool in the declaration (as before), then you
> >> >> can use it for the call to svn_cl__xml_print_header(), as its
> >> >> scratch_pool. Any mem used by the call will be cleared on the first
> >> >> iteration of the loop.
> > [...]
> >> >> And if you don't destroy it so soon, then you get to use it for that
> >> >> last call, too.
> >> > [...]
> >> >
> >> > That's true, there is the opportunity to do that, but is it really worth
> >> > breaking the well-defined "iterpool" pattern for such a little extra
> >> > optimisation? I don't think so. Keep it simple.
> >>
> >> Eh? How does it break it?
> >>
> >> You alloc the iterpool. clear it at the start of each iteration.
> >> destroy it later. I see no change to that.
> >
> > "The pool you use also helps readers of the code understand object
> > lifetimes. Is a given object used only during one iteration of the loop,
> > or will it need to last beyond the end of the loop?"
> >
> > Quoted from <http://subversion.tigris.org/hacking.html#apr-pools>.
>
> Okay. So what? The iterpool is still an iterpool, used within the loop
> and cleared each time. How does allocating other objects in it "break"
> the pattern?
>
> (and yes, we use iterpools outside of the loop in plenty of other
> places; it makes a great scratch pool)
We can talk in person tomorrow. Easier than email :-)
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411484
Re: svn commit: r40218 - trunk/subversion/svn
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Oct 26, 2009 at 04:32, Julian Foad <ju...@btopenworld.com> wrote:
> Greg Stein wrote:
>> On Sat, Oct 24, 2009 at 18:42, Julian Foad <ju...@btopenworld.com> wrote:
>> > Greg Stein wrote:
>> >> If you construct the iterpool in the declaration (as before), then you
>> >> can use it for the call to svn_cl__xml_print_header(), as its
>> >> scratch_pool. Any mem used by the call will be cleared on the first
>> >> iteration of the loop.
> [...]
>> >> And if you don't destroy it so soon, then you get to use it for that
>> >> last call, too.
>> > [...]
>> >
>> > That's true, there is the opportunity to do that, but is it really worth
>> > breaking the well-defined "iterpool" pattern for such a little extra
>> > optimisation? I don't think so. Keep it simple.
>>
>> Eh? How does it break it?
>>
>> You alloc the iterpool. clear it at the start of each iteration.
>> destroy it later. I see no change to that.
>
> "The pool you use also helps readers of the code understand object
> lifetimes. Is a given object used only during one iteration of the loop,
> or will it need to last beyond the end of the loop?"
>
> Quoted from <http://subversion.tigris.org/hacking.html#apr-pools>.
Okay. So what? The iterpool is still an iterpool, used within the loop
and cleared each time. How does allocating other objects in it "break"
the pattern?
(and yes, we use iterpools outside of the loop in plenty of other
places; it makes a great scratch pool)
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411432
Re: svn commit: r40218 - trunk/subversion/svn
Posted by Julian Foad <ju...@btopenworld.com>.
Greg Stein wrote:
> On Sat, Oct 24, 2009 at 18:42, Julian Foad <ju...@btopenworld.com> wrote:
> > Greg Stein wrote:
> >> If you construct the iterpool in the declaration (as before), then you
> >> can use it for the call to svn_cl__xml_print_header(), as its
> >> scratch_pool. Any mem used by the call will be cleared on the first
> >> iteration of the loop.
[...]
> >> And if you don't destroy it so soon, then you get to use it for that
> >> last call, too.
> > [...]
> >
> > That's true, there is the opportunity to do that, but is it really worth
> > breaking the well-defined "iterpool" pattern for such a little extra
> > optimisation? I don't think so. Keep it simple.
>
> Eh? How does it break it?
>
> You alloc the iterpool. clear it at the start of each iteration.
> destroy it later. I see no change to that.
"The pool you use also helps readers of the code understand object
lifetimes. Is a given object used only during one iteration of the loop,
or will it need to last beyond the end of the loop?"
Quoted from <http://subversion.tigris.org/hacking.html#apr-pools>.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411293
Re: svn commit: r40218 - trunk/subversion/svn
Posted by Greg Stein <gs...@gmail.com>.
On Sat, Oct 24, 2009 at 18:42, Julian Foad <ju...@btopenworld.com> wrote:
> Greg Stein wrote:
>> On Sat, Oct 24, 2009 at 08:35, Hyrum K. Wright <hy...@hyrumwright.org> wrote:
>> >...
>> > +++ trunk/subversion/svn/proplist-cmd.c Sat Oct 24 05:35:12 2009 (r40218)
>> >...
>> > else /* operate on normal, versioned properties (not revprops) */
>> > {
>> > - apr_pool_t *subpool = svn_pool_create(pool);
>> > + apr_pool_t *iterpool;
>> > svn_proplist_receiver_t pl_receiver;
>> >
>> > if (opt_state->xml)
>> > {
>> > - SVN_ERR(svn_cl__xml_print_header("properties", pool));
>> > + SVN_ERR(svn_cl__xml_print_header("properties", scratch_pool));
>> > pl_receiver = proplist_receiver_xml;
>> > }
>> > else
>> > @@ -181,6 +180,7 @@ svn_cl__proplist(apr_getopt_t *os,
>> > if (opt_state->depth == svn_depth_unknown)
>> > opt_state->depth = svn_depth_empty;
>> >
>> > + iterpool = svn_pool_create(scratch_pool);
>> > for (i = 0; i < targets->nelts; i++)
>>
>> If you construct the iterpool in the declaration (as before), then you
>> can use it for the call to svn_cl__xml_print_header(), as its
>> scratch_pool. Any mem used by the call will be cleared on the first
>> iteration of the loop.
>>
>> >...
>> > @@ -204,17 +204,16 @@ svn_cl__proplist(apr_getopt_t *os,
>> > opt_state->depth,
>> > opt_state->changelists,
>> > pl_receiver, &pl_baton,
>> > - ctx, subpool),
>> > + ctx, iterpool),
>> > NULL, opt_state->quiet,
>> > SVN_ERR_UNVERSIONED_RESOURCE,
>> > SVN_ERR_ENTRY_NOT_FOUND,
>> > SVN_NO_ERROR));
>> > }
>> > + svn_pool_destroy(iterpool);
>> >
>> > if (opt_state->xml)
>> > - SVN_ERR(svn_cl__xml_print_footer("properties", pool));
>> > -
>> > - svn_pool_destroy(subpool);
>> > + SVN_ERR(svn_cl__xml_print_footer("properties", scratch_pool));
>> > }
>> >
>> > return SVN_NO_ERROR;
>>
>> And if you don't destroy it so soon, then you get to use it for that
>> last call, too.
> [...]
>
> That's true, there is the opportunity to do that, but is it really worth
> breaking the well-defined "iterpool" pattern for such a little extra
> optimisation? I don't think so. Keep it simple.
Eh? How does it break it?
You alloc the iterpool. clear it at the start of each iteration.
destroy it later. I see no change to that.
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411124
Re: svn commit: r40218 - trunk/subversion/svn
Posted by Julian Foad <ju...@btopenworld.com>.
Greg Stein wrote:
> On Sat, Oct 24, 2009 at 08:35, Hyrum K. Wright <hy...@hyrumwright.org> wrote:
> >...
> > +++ trunk/subversion/svn/proplist-cmd.c Sat Oct 24 05:35:12 2009 (r40218)
> >...
> > else /* operate on normal, versioned properties (not revprops) */
> > {
> > - apr_pool_t *subpool = svn_pool_create(pool);
> > + apr_pool_t *iterpool;
> > svn_proplist_receiver_t pl_receiver;
> >
> > if (opt_state->xml)
> > {
> > - SVN_ERR(svn_cl__xml_print_header("properties", pool));
> > + SVN_ERR(svn_cl__xml_print_header("properties", scratch_pool));
> > pl_receiver = proplist_receiver_xml;
> > }
> > else
> > @@ -181,6 +180,7 @@ svn_cl__proplist(apr_getopt_t *os,
> > if (opt_state->depth == svn_depth_unknown)
> > opt_state->depth = svn_depth_empty;
> >
> > + iterpool = svn_pool_create(scratch_pool);
> > for (i = 0; i < targets->nelts; i++)
>
> If you construct the iterpool in the declaration (as before), then you
> can use it for the call to svn_cl__xml_print_header(), as its
> scratch_pool. Any mem used by the call will be cleared on the first
> iteration of the loop.
>
> >...
> > @@ -204,17 +204,16 @@ svn_cl__proplist(apr_getopt_t *os,
> > opt_state->depth,
> > opt_state->changelists,
> > pl_receiver, &pl_baton,
> > - ctx, subpool),
> > + ctx, iterpool),
> > NULL, opt_state->quiet,
> > SVN_ERR_UNVERSIONED_RESOURCE,
> > SVN_ERR_ENTRY_NOT_FOUND,
> > SVN_NO_ERROR));
> > }
> > + svn_pool_destroy(iterpool);
> >
> > if (opt_state->xml)
> > - SVN_ERR(svn_cl__xml_print_footer("properties", pool));
> > -
> > - svn_pool_destroy(subpool);
> > + SVN_ERR(svn_cl__xml_print_footer("properties", scratch_pool));
> > }
> >
> > return SVN_NO_ERROR;
>
> And if you don't destroy it so soon, then you get to use it for that
> last call, too.
[...]
That's true, there is the opportunity to do that, but is it really worth
breaking the well-defined "iterpool" pattern for such a little extra
optimisation? I don't think so. Keep it simple.
- Julian
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2411102