On Sat, Feb 28, 2009 at 2:34 AM, Bert Huijben <bert_at_vmoo.com> wrote:
>> > ctx->resource_url = svn_uri_canonicalize(uri.path, ctx->pool);
>> > +
>> > + if (ctx->resource_url == uri.path)
>> > + ctx->resource_url = apr_pstrdup(ctx->pool, ctx-
>> >resource_url);
>> Hi Bert,
>>
>> Sorry but I don't like this fix. It looks like premature optimization
>> which is always bad thing.
>>
>> We should follow contract of svn_uri_canonicalize function that could
>> theoretically return pointer to other than first character in provided
>> buffer.
>>
>> So it's much better just add apr_pstrdup() in all cases from point of
>> readability and preventing memory errors.
>
> Feel free to fix this yourself after you spend a few days profiling. (You can
> also fix the about a dozen of other locations in our code that use the same
> construct). We try to do most memory consuming operations in scratch
> pools and this context pool is certainly not short lived.
>
> I'd personally suggest fixing the documentation of the svn_(uri/dirent)_canonicalize
> function to say that it will always copy or return a static value (for ""). The current
> canonicalization rules (including case fixing) make it expensive not to copy while
> canonicalizing.
>
> The optimization of our code should be in reducing these expensive canonicalization
> calls and just assuming everything passed is canonical, not in doing the operation
> twice to reduce a copy.
Hi Bert, it seems you slightly misunderstand me. I've just noticed
that code you wrote does not follow svn_uri_canonicalize() contract.
After your commit r36204 everything is great. And now you can remove
code to apr_pstrdup() since result will be guaranteed to be allocated
in ctx->pool or statically allocated :
[[
if (ctx->resource_url == uri.path)
ctx->resource_url = apr_pstrdup(ctx->pool, ctx-resource_url);
]]
Btw Greg pointed that uri should be canonicalized when passed to RA layer.
--
Ivan Zhakov
VisualSVN Team
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1243695
Received on 2009-02-28 10:10:03 CET