You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Bert Huijben <be...@qqmail.nl> on 2012/11/22 10:30:42 UTC
RE: svn commit: r1412418 - in /subversion/trunk/subversion: include/private/svn_string_private.h libsvn_subr/string.c
> -----Original Message-----
> From: brane@apache.org [mailto:brane@apache.org]
> Sent: donderdag 22 november 2012 06:21
> To: commits@subversion.apache.org
> Subject: svn commit: r1412418 - in /subversion/trunk/subversion:
> include/private/svn_string_private.h libsvn_subr/string.c
>
> Author: brane
> Date: Thu Nov 22 05:20:51 2012
> New Revision: 1412418
>
> URL: http://svn.apache.org/viewvc?rev=1412418&view=rev
> Log:
> Groundwork for issue #4261. Invent memory buffers.
Issue #4261 is called "Setting unknown svn: propnames should require 'force'", so I think you want to reference a different issue number here.
Bert
Re: svn commit: r1412418 - in /subversion/trunk/subversion: include/private/svn_string_private.h libsvn_subr/string.c
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> On 22.11.2012 16:08, Julian Foad wrote:
>> Branko Čibej wrote:
>>> +/** A self-contained memory buffer of known size.
>>> + *
>>> + * Intended to be used where a single variable-sized buffer is needed
>>> + * within an iteration, a scratch pool is available and we want to
>>> + * avoid the cost of creating another pool just for the iteration.
>>> + */
>>> +typedef struct svn_membuf_t
>> What does that comment about scratch pools and iterpools mean?
>>
>> I don't follow what characteristics this new memory buffer has, and
>> how it compares with svn_stringbuf_t. You realize svn_stringbuf_t
>> already supports arbitrary data with null characters in it?
>>
>> svn_stringbuf_t seems to be all of this, and more because it tracks
>> the length of the user's data separately from the allocated length.
>> What are the advantages of this new buffer?
>
> Imagine the case of this LCS computation, that needs to allocate some
> memory for the algorithm. Typically we'd do that by passing a scratch
> pool to the function and let it allocate from that. And if the caller
> did this in a loop, it would create an iterpool, which would get cleared
> in every iteration.
>
> This is fine as long as the buffer allocation and pool clearing are
> cheap relative to the rest of the loop, and if the overhead of creating
> a pool just for that isn't a problem. But there are cases where this
> simply does not hold, and it's nice to be able to use the same buffer
> over and over again, without having to reallocate it every time.
OK, I follow that, but you're basically just saying "this buffer type is designed be
re-used efficiently". The mention of loops and scratch pools has little to do with the buffer
concept itself.
> I have a similar situation on the wc-collate-path branch (in the key
> comparator), for which I used stringbufs. But I ran into a few snags:
>
> * Stringbufs always allocate the extra \0
Why is that a snag?
> * Resizing a stringbuf will always copy its contents
Depends what you mean by "resizing". It only re-allocs memory and copies the contents when the required "user data size" surpasses the allocated size, and then it doubles the allocation so it won't do it too often. stringbuf_setempty() followed by stringbuf_ensure(whatever) won't copy anything.
> * Stringbuf's data pointer is a char*
So what? The inconvenience of casting to/from (something_else *)?
> I solved the second by adding an svn_stringbuf__resize private function,
> but couldn't really solve the first without completely reimplementing
> stringbufs. The third required a bunch of yucky casts.
I don't get it. (Haven't looked at your __resize() yet.)
> My intention is to try to share implementation between stringbufs and
> membufs. If that doesn't work, I'll probably just throw these membufs
> away; but I'm hoping that won't be necessary. Another alternative is to
> turn this around and make the membuf API a wrapper around stringbufs;
> but that seems just wrong given that membufs are the more generic concept.
Cool.
- Julian
Re: svn commit: r1412418 - in /subversion/trunk/subversion: include/private/svn_string_private.h
libsvn_subr/string.c
Posted by Branko Čibej <br...@wandisco.com>.
On 22.11.2012 16:08, Julian Foad wrote:
> Branko Čibej wrote:
>> +/** A self-contained memory buffer of known size.
>> + *
>> + * Intended to be used where a single variable-sized buffer is needed
>> + * within an iteration, a scratch pool is available and we want to
>> + * avoid the cost of creating another pool just for the iteration.
>> + */
>> +typedef struct svn_membuf_t
> What does that comment about scratch pools and iterpools mean?
>
> I don't follow what characteristics this new memory buffer has, and how it compares with svn_stringbuf_t. You realize svn_stringbuf_t already supports arbitrary data with null characters in it?
>
> svn_stringbuf_t seems to be all of this, and more because it tracks the length of the user's data separately from the allocated length. What are the advantages of this new buffer?
Imagine the case of this LCS computation, that needs to allocate some
memory for the algorithm. Typically we'd do that by passing a scratch
pool to the function and let it allocate from that. And if the caller
did this in a loop, it would create an iterpool, which would get cleared
in every iteration.
This is fine as long as the buffer allocation and pool clearing are
cheap relative to the rest of the loop, and if the overhead of creating
a pool just for that isn't a problem. But there are cases where this
simply does not hold, and it's nice to be able to use the same buffer
over and over again, without having to reallocate it every time.
I have a similar situation on the wc-collate-path branch (in the key
comparator), for which I used stringbufs. But I ran into a few snags:
* Stringbufs always allocate the extra \0
* Resizing a stringbuf will always copy its contents
* Stringbuf's data pointer is a char*
I solved the second by adding an svn_stringbuf__resize private function,
but couldn't really solve the first without completely reimplementing
stringbufs. The third required a bunch of yucky casts.
My intention is to try to share implementation between stringbufs and
membufs. If that doesn't work, I'll probably just throw these membufs
away; but I'm hoping that won't be necessary. Another alternative is to
turn this around and make the membuf API a wrapper around stringbufs;
but that seems just wrong given that membufs are the more generic concept.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: svn commit: r1412418 - in /subversion/trunk/subversion: include/private/svn_string_private.h libsvn_subr/string.c
Posted by Julian Foad <ju...@btopenworld.com>.
Branko Čibej wrote:
> On 22.11.2012 10:30, Bert Huijben wrote:
>>> URL: http://svn.apache.org/viewvc?rev=1412418&view=rev
>>> Log:
>>> Groundwork for issue #4261. Invent memory buffers.
>> Issue #4261 is called "Setting unknown svn: propnames should require
> 'force'", so I think you want to reference a different issue number
> here.
>
> Nope, this is exactly what I'm laying the groundwork for. As will become
> obvious in the next couple days.
It's kind to the reader if we include the issue's subject line when quoting an issue number out of context like this. Of course, if all our systems automatically annotated and hyperlinked issue number references, that wouldn't be necessary.
> +/** A self-contained memory buffer of known size.
> + *
> + * Intended to be used where a single variable-sized buffer is needed
> + * within an iteration, a scratch pool is available and we want to
> + * avoid the cost of creating another pool just for the iteration.
> + */
> +typedef struct svn_membuf_t
What does that comment about scratch pools and iterpools mean?
I don't follow what characteristics this new memory buffer has, and how it compares with svn_stringbuf_t. You realize svn_stringbuf_t already supports arbitrary data with null characters in it?
svn_stringbuf_t seems to be all of this, and more because it tracks the length of the user's data separately from the allocated length. What are the advantages of this new buffer?
- Julian
Re: svn commit: r1412418 - in /subversion/trunk/subversion: include/private/svn_string_private.h
libsvn_subr/string.c
Posted by Branko Čibej <br...@wandisco.com>.
On 22.11.2012 18:43, Mark Phippard wrote:
> On Nov 22, 2012, at 12:27 PM, Branko Čibej <br...@wandisco.com> wrote:
>
>> On 22.11.2012 18:18, Mark Phippard wrote:
>>> Just a thought. We do not have many svn: props. Couldn't we just list all of them? If we knew they were setting on a folder or file we could further limit it to that subset. Seems a lot easier and just as useful, perhaps even more useful.
>> We have 14 user-visible node props and 10 revision props. Surely you
>> don't want to see 10 lines of error message instead of 1 because of a typo?
> No, but I would be fine with 5.
>
> Fwiw, I was looking here:
>
> http://svnbook.red-bean.com/en/1.7/svn.ref.properties.html
>
> I was also assuming that we would not ever suggest the props we do not expect a user to set manually.
Indeed, I didn't count the hidden WC props. Everything else is up for
grabs, IMO.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: svn commit: r1412418 - in /subversion/trunk/subversion: include/private/svn_string_private.h libsvn_subr/string.c
Posted by Mark Phippard <ma...@gmail.com>.
On Nov 22, 2012, at 12:27 PM, Branko Čibej <br...@wandisco.com> wrote:
> On 22.11.2012 18:18, Mark Phippard wrote:
>> Just a thought. We do not have many svn: props. Couldn't we just list all of them? If we knew they were setting on a folder or file we could further limit it to that subset. Seems a lot easier and just as useful, perhaps even more useful.
>
> We have 14 user-visible node props and 10 revision props. Surely you
> don't want to see 10 lines of error message instead of 1 because of a typo?
No, but I would be fine with 5.
Fwiw, I was looking here:
http://svnbook.red-bean.com/en/1.7/svn.ref.properties.html
I was also assuming that we would not ever suggest the props we do not expect a user to set manually.
Mark
Re: svn commit: r1412418 - in /subversion/trunk/subversion: include/private/svn_string_private.h
libsvn_subr/string.c
Posted by Branko Čibej <br...@wandisco.com>.
On 22.11.2012 18:18, Mark Phippard wrote:
> Just a thought. We do not have many svn: props. Couldn't we just list all of them? If we knew they were setting on a folder or file we could further limit it to that subset. Seems a lot easier and just as useful, perhaps even more useful.
We have 14 user-visible node props and 10 revision props. Surely you
don't want to see 10 lines of error message instead of 1 because of a typo?
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Re: svn commit: r1412418 - in /subversion/trunk/subversion: include/private/svn_string_private.h libsvn_subr/string.c
Posted by Mark Phippard <ma...@gmail.com>.
On Nov 22, 2012, at 11:50 AM, Branko Čibej <br...@wandisco.com> wrote:
> On 22.11.2012 15:19, Bert Huijben wrote:
>>
>>> -----Original Message-----
>>> From: Branko Čibej [mailto:brane@wandisco.com]
>>> Sent: donderdag 22 november 2012 11:04
>>> To: dev@subversion.apache.org
>>> Subject: Re: svn commit: r1412418 - in /subversion/trunk/subversion:
>>> include/private/svn_string_private.h libsvn_subr/string.c
>>>
>>> On 22.11.2012 10:30, Bert Huijben wrote:
>>>>> -----Original Message-----
>>>>> From: brane@apache.org [mailto:brane@apache.org]
>>>>> Sent: donderdag 22 november 2012 06:21
>>>>> To: commits@subversion.apache.org
>>>>> Subject: svn commit: r1412418 - in /subversion/trunk/subversion:
>>>>> include/private/svn_string_private.h libsvn_subr/string.c
>>>>>
>>>>> Author: brane
>>>>> Date: Thu Nov 22 05:20:51 2012
>>>>> New Revision: 1412418
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1412418&view=rev
>>>>> Log:
>>>>> Groundwork for issue #4261. Invent memory buffers.
>>>> Issue #4261 is called "Setting unknown svn: propnames should require
>>> 'force'", so I think you want to reference a different issue number here.
>>>
>>> Nope, this is exactly what I'm laying the groundwork for. As will become
>>> obvious in the next couple days.
>> Thanks Daniel:
>> 15:04 <@danielsh> Bert: the last comment on #4261
>> 15:04 <@danielsh> where brane says he's getting carried away and implementing a spellchecker
>> 15:05 <@Bert> "the spelling in the error message"?
>> 15:05 <@danielsh> "Unknown propname 'svn:ingore'; did you mean 'svn:ignore'?"
>> 15:17 <@Bert> The problem would only be that we don't show an error on typos in the 'svn:' part.
>
> How did you come to the conclusion that we don't, I mean won't? :)
>
>> 15:17 <@Bert> But thanks for explaining. Now it finally makes sense.
>
> I'll try to remember to explain things a bit more proactively and
> prophetically
Just a thought. We do not have many svn: props. Couldn't we just list all of them? If we knew they were setting on a folder or file we could further limit it to that subset. Seems a lot easier and just as useful, perhaps even more useful.
Mark
Re: svn commit: r1412418 - in /subversion/trunk/subversion: include/private/svn_string_private.h
libsvn_subr/string.c
Posted by Branko Čibej <br...@wandisco.com>.
On 22.11.2012 15:19, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: Branko Čibej [mailto:brane@wandisco.com]
>> Sent: donderdag 22 november 2012 11:04
>> To: dev@subversion.apache.org
>> Subject: Re: svn commit: r1412418 - in /subversion/trunk/subversion:
>> include/private/svn_string_private.h libsvn_subr/string.c
>>
>> On 22.11.2012 10:30, Bert Huijben wrote:
>>>> -----Original Message-----
>>>> From: brane@apache.org [mailto:brane@apache.org]
>>>> Sent: donderdag 22 november 2012 06:21
>>>> To: commits@subversion.apache.org
>>>> Subject: svn commit: r1412418 - in /subversion/trunk/subversion:
>>>> include/private/svn_string_private.h libsvn_subr/string.c
>>>>
>>>> Author: brane
>>>> Date: Thu Nov 22 05:20:51 2012
>>>> New Revision: 1412418
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1412418&view=rev
>>>> Log:
>>>> Groundwork for issue #4261. Invent memory buffers.
>>> Issue #4261 is called "Setting unknown svn: propnames should require
>> 'force'", so I think you want to reference a different issue number here.
>>
>> Nope, this is exactly what I'm laying the groundwork for. As will become
>> obvious in the next couple days.
> Thanks Daniel:
> 15:04 <@danielsh> Bert: the last comment on #4261
> 15:04 <@danielsh> where brane says he's getting carried away and implementing a spellchecker
> 15:05 <@Bert> "the spelling in the error message"?
> 15:05 <@danielsh> "Unknown propname 'svn:ingore'; did you mean 'svn:ignore'?"
> 15:17 <@Bert> The problem would only be that we don't show an error on typos in the 'svn:' part.
How did you come to the conclusion that we don't, I mean won't? :)
> 15:17 <@Bert> But thanks for explaining. Now it finally makes sense.
I'll try to remember to explain things a bit more proactively and
prophetically in future.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
RE: svn commit: r1412418 - in /subversion/trunk/subversion: include/private/svn_string_private.h libsvn_subr/string.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Branko Čibej [mailto:brane@wandisco.com]
> Sent: donderdag 22 november 2012 11:04
> To: dev@subversion.apache.org
> Subject: Re: svn commit: r1412418 - in /subversion/trunk/subversion:
> include/private/svn_string_private.h libsvn_subr/string.c
>
> On 22.11.2012 10:30, Bert Huijben wrote:
> >
> >> -----Original Message-----
> >> From: brane@apache.org [mailto:brane@apache.org]
> >> Sent: donderdag 22 november 2012 06:21
> >> To: commits@subversion.apache.org
> >> Subject: svn commit: r1412418 - in /subversion/trunk/subversion:
> >> include/private/svn_string_private.h libsvn_subr/string.c
> >>
> >> Author: brane
> >> Date: Thu Nov 22 05:20:51 2012
> >> New Revision: 1412418
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1412418&view=rev
> >> Log:
> >> Groundwork for issue #4261. Invent memory buffers.
> > Issue #4261 is called "Setting unknown svn: propnames should require
> 'force'", so I think you want to reference a different issue number here.
>
> Nope, this is exactly what I'm laying the groundwork for. As will become
> obvious in the next couple days.
Thanks Daniel:
15:04 <@danielsh> Bert: the last comment on #4261
15:04 <@danielsh> where brane says he's getting carried away and implementing a spellchecker
15:05 <@Bert> "the spelling in the error message"?
15:05 <@danielsh> "Unknown propname 'svn:ingore'; did you mean 'svn:ignore'?"
15:17 <@Bert> The problem would only be that we don't show an error on typos in the 'svn:' part.
15:17 <@Bert> But thanks for explaining. Now it finally makes sense.
Bert
Re: svn commit: r1412418 - in /subversion/trunk/subversion: include/private/svn_string_private.h
libsvn_subr/string.c
Posted by Branko Čibej <br...@wandisco.com>.
On 22.11.2012 10:30, Bert Huijben wrote:
>
>> -----Original Message-----
>> From: brane@apache.org [mailto:brane@apache.org]
>> Sent: donderdag 22 november 2012 06:21
>> To: commits@subversion.apache.org
>> Subject: svn commit: r1412418 - in /subversion/trunk/subversion:
>> include/private/svn_string_private.h libsvn_subr/string.c
>>
>> Author: brane
>> Date: Thu Nov 22 05:20:51 2012
>> New Revision: 1412418
>>
>> URL: http://svn.apache.org/viewvc?rev=1412418&view=rev
>> Log:
>> Groundwork for issue #4261. Invent memory buffers.
> Issue #4261 is called "Setting unknown svn: propnames should require 'force'", so I think you want to reference a different issue number here.
Nope, this is exactly what I'm laying the groundwork for. As will become
obvious in the next couple days.
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com