Re: object-model: Return by value, reference or pointer? (or something else?)
Hello Hyrum!
On Thursday 14 October 2010, Hyrum K. Wright wrote:
The following is a somewhat naïve implementation, but does it jive
with your suggestion?
...
inline const char *c_str() const
{
if (isNull)
return NULL;
else
return std::string::c_str();
}
It might be easier for callers if this did
if (isNull)
return ;
so that the value could just be used in printf() and similar without
explicit checking.
Furthermore I don't know how many instances of these strings will be active
simultaneously; but the bool isNull could cost up to 8 bytes (because of
aligning on 64bit).
If that is a concern, how about using a specific, static value for the
std::string instead that is compared against?
I know, microoptimization again, but I wanted to mention the idea.
Regards,
Phil
Re: object-model: Return by value, reference or pointer? (or something else?)
Philipp Marek philipp.ma...@linbit.com: It might be easier for callers if this did if (isNull) return ; so that the value could just be used in printf() and similar without explicit checking. If that was ok to do, one might as well represent a NULL with an empty string.
Re: svn commit: r1004286 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
Ramkumar Ramachandra artag...@gmail.com writes: Hi Philip, [sorry about the delayed reply: I had a bad internet connection] Philip Martin writes: If we are going to use the APR atomic interface then the two reads should use apr_atomic_read32. It would be better to use svn_atomic__init_once. It's a clear indication that we are doing once only initialisation, so we don't need all the comments, and it avoids any problems related to the size of apr_fileperms_t. Also if enhancements are required (more memory barriers say) then svn_atomic__init_once is the place to do it. Hm. I read up a little more about this, but what confuses me is- shouldn't the rest of the code already be needing this? I don't understand your questions. To what does rest of the code refer? Why are we re-thinking everything from scratch? To what does everything refer? -- Philip
Re: [PATCH] Simplify WC DB function start_directory_update_txn()
Greg Stein wrote: Seems fine. Thanks. Committed revision 1022862. - Julian On Thu, Oct 14, 2010 at 18:32, Julian Foad julian.f...@wandisco.com wrote: Hi Bert. Any objection to me simplifying start_directory_update_txn(), as in the attached patch? It appears that it's doing relatively a lot of work every time just to decide whether the repos_relpath is going to be the same as before, and thus decide whether to use a different update statement that omits that parameter. It seems to that the work done, and the code complexity, must far outweigh the cost of simply providing the parameter every time. Am I missing something? I haven't profiled it, as it looks like an obvious win. I'll commit if no objection. - Julian
Re: object-model: Return by value, reference or pointer? (or something else?)
On 14.10.2010 20:39, Hyrum K. Wright wrote: The following is a somewhat naïve implementation, but does it jive with your suggestion? Roughly yes, see the other comment. On reflection, though, I like the suggestion of returning an std::pairstd::string, bool. Make a typedef of that so that users can declare return-value variables, and use it where it's absolutely necessary to know that it's not an empty string but a null string. Saves a lot of trouble with the string subclassing, too. And better than pulling in 90% of boost just to get a poor-man's replacement for null references. -- Brane
Re: object-model: Return by value, reference or pointer? (or something else?)
Branko Čibej br...@xbc.nu: std::string also doesn't have any virtual functions, nor does a bool member require a destructor. Memory deallocation is OK. std::string is actually a POD and tricky rules apply that allow you do do this safely, IIRC ... Nevertheless, deleting a derived object from a base class pointer, when there isn't a virtual constructor has undefined behaviour. And IMO venturing into a language's undefined behaviour area is not a good choice for a public API.
Re: object-model: Return by value, reference or pointer? (or something else?)
Steinar Bang s...@dod.no: Philipp Marek philipp.ma...@linbit.com: It might be easier for callers if this did if (isNull) return ; so that the value could just be used in printf() and similar without explicit checking. If that was ok to do, one might as well represent a NULL with an empty string. As I said in a different message in the thread, if you need NULL, I think returning auto_ptrstring is the simplest way that is both standards compliant, won't leak memory, and doesn't force you to handle object life cycle for the returned strings.
Re: trunk failing tests on Windows XP (32 bit): prop-tests.py 33, stat-tests.py 5, upgrade-tests.py 11
On Sun, Oct 10, 2010 at 3:07 PM, Johan Corveleyn jcor...@gmail.com wrote: On Sat, Oct 2, 2010 at 11:24 AM, Bert Huijben b...@qqmail.nl wrote: -Original Message- From: Paul Burba [mailto:ptbu...@gmail.com] Sent: vrijdag 1 oktober 2010 15:46 To: Bert Huijben Cc: Johan Corveleyn; Subversion Development Subject: Re: trunk failing tests on Windows XP (32 bit): prop-tests.py 33, stat-tests.py 5, upgrade-tests.py 11 On Thu, Sep 30, 2010 at 8:10 PM, Bert Huijben b...@qqmail.nl wrote: -Original Message- From: Johan Corveleyn [mailto:jcor...@gmail.com] Sent: vrijdag 1 oktober 2010 1:51 To: Subversion Development Subject: trunk failing tests on Windows XP (32 bit): prop-tests.py 33, stat-tests.py 5, upgrade-tests.py 11 Hi devs, The following tests fail on my machine (Windows XP 32-bit, built with VCE 2008, ra_local): - prop-tests.py 33: test properties of obstructed subdirectories svn: Can't open directory 'C:\research\svn\client_build\svn_trunk\Release\subversion\tests\cmdlin e\svn-test-work\working_copies\prop_tests-33\A\C': The directory name is invalid. - stat-tests.py 5: status on versioned items whose type has changed svn: Can't open directory 'C:\research\svn\client_build\svn_trunk\Release\subversion\tests\cmdlin e\svn-test-work\working_copies\stat_tests-5\A': The directory name is invalid. - upgrade_tests.py 11: missing directories and obstructing files svn: Can't open directory 'C:\research\svn\client_build\svn_trunk\Release\subversion\tests\cmdlin e\svn-test-work\working_copies\upgrade_tests-11\A\D': The directory name is invalid. They all seem to try to open some path as a directory, but it isn't a directory (but an empty file or something). Am I the only one seeing this? Is this known/normal? This seems to be related to the APR version you use. (Paul Burba reported this same problem earlier this week). Hi Johan, Bert is correct, I saw these failure earlier this week while using APR 1.3.12. All the failures occur when a file unexpectedly obstructs a directory. The APR macro APR_STATUS_IS_ENOTDIR in 1.3.x does not handle the ERROR_DIRECTORY 267 The directory name is invalid that Windows raises in this case. This was fixed in APR in http://svn.apache.org/viewvc?view=revisionrevision=821306 and is in APR 1.4.2. I think we should just add this error to the SVN__APR_STATUS_IS_ENOTDIR() macro to fix compatibility with older apr versions, but I would like to know why this didn't fail a few weeks ago. (I haven't been very active for the last week, but I haven't seen any relevant commits on the commit list in the last few weeks) FWIW: I don't know if anybody else has tried to pinpoint when/why this started to fail on trunk, but I've taken a look. It seems to have started already quite some time ago: it started with r991236 (31 August), which was the enabling of single-DB mode. I didn't have time yet to investigate further with SVN_WC__SINGLE_DB and SINGLE_DB defined to see which commit caused it with single-db already enabled before r991236. If nobody beats me to it, I'll continue the search when I have some more time next week. Gut feeling: maybe it has something to do with the actual removal of directories-scheduled-for-deletion in single-db, as opposed to the wc-1 behavior? I saw that the problem was fixed in r102170 by Bert, by changing the SVN__APR_STATUS_IS_ENOTDIR() macro. Thanks, the tests all pass now, with APR 1.3.8 on my WinXP. Now, some root-cause analysis :-), in case this is still useful... Going further back than r991236, but with SVN_WC__SINGLE_DB and SINGLE_DB explicitly defined, I found the following: 1) prop_tests.py 33 - First fails in r980406 with (Can't open directory ... : The directory name is invalid.) - Before r980406, it fails with a more normal failure: EXCEPTION: SVNTreeUnequal. 2) stat_tests.py 5 - First fails in r990818 with (Can't open directory ... : The directory name is invalid.) - Before r990818, the test succeeded. 3) upgrade_tests.py 11 - First fails in r991160 with (Can't open directory ... : The directory name is invalid.) - Before r991160, it fails with an assertion: (svn: In file '..\..\..\subversion\libsvn_wc\wc_db.c' line 6558: assertion failed ((pdh)-wcroot != NULL (pdh)-wcroot-format == SVN_WC__VERSION)). Now it's up to someone else to take a closer look at those revisions, and try to figure out why they introduced the directory name is invalid failures. That's a bit out of my league :-). Cheers, -- Johan
Re: trunk failing tests on Windows XP (32 bit): prop-tests.py 33, stat-tests.py 5, upgrade-tests.py 11
On Fri, Oct 15, 2010 at 12:20 PM, Johan Corveleyn jcor...@gmail.com wrote: I saw that the problem was fixed in r102170 by Bert s/r102170/r1021760/ I really should install that Undo send google labs extension :) -- Johan
Re: object-model: Return by value, reference or pointer? (or something else?)
On Fri, Oct 15, 2010 at 4:35 AM, Branko Čibej br...@xbc.nu wrote: On 14.10.2010 20:39, Hyrum K. Wright wrote: The following is a somewhat naïve implementation, but does it jive with your suggestion? Roughly yes, see the other comment. On reflection, though, I like the suggestion of returning an std::pairstd::string, bool. Make a typedef of that so that users can declare return-value variables, and use it where it's absolutely necessary to know that it's not an empty string but a null string. Saves a lot of trouble with the string subclassing, too. And better than pulling in 90% of boost just to get a poor-man's replacement for null references. Yeah, after that little implementation exercise, and hearing here all the nuances of subclassing std::string, I'm leaning in this direction as well. Having a separate NullableString type would also help folks know which strings are guaranteed to have values and which ones are optional via the API. (I hope our C docs point this out explicitly, too.) -Hyrum
RE: svn commit: r1022707 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c
-Original Message- From: Stefan Sperling [mailto:s...@elego.de] Sent: donderdag 14 oktober 2010 23:39 To: dev@subversion.apache.org Subject: Re: svn commit: r1022707 - in /subversion/trunk: ./ subversion/libsvn_subr/io.c On Thu, Oct 14, 2010 at 09:00:44PM -, hwri...@apache.org wrote: Author: hwright Date: Thu Oct 14 21:00:43 2010 New Revision: 1022707 URL: http://svn.apache.org/viewvc?rev=1022707view=rev Log: Merge r985472 from the performance branch, which see for more information. This revision randomizes the numerical suffix of temporary files, in an effort to make finding one easier. @@ -384,17 +393,35 @@ svn_io_open_uniquely_named(apr_file_t ** This is good, since 1 would misleadingly imply that the second attempt was actually the first... and if someone's got conflicts on their conflicts, we probably don't want to - add to their confusion :-). */ + add to their confusion :-). + + Also, the randomization used to minimize the number of re-try + cycles will interfere with certain tests that compare working + copies etc. + */ if (i == 1) unique_name = apr_psprintf(scratch_pool, %s%s, path, suffix); else -unique_name = apr_psprintf(scratch_pool, %s.%u%s, path, i, suffix); +unique_name = apr_psprintf(scratch_pool, %s.%u_%x%s, path, i, rand(), suffix); -1 on using rand() here, for several reasons: The intent of svn_io_open_uniquely_named() is to provide user-facing temporary files. The order of numbers actually carry meaning in some cases. With files named svn-commit.tmp, svn-commit.2.tmp, svn-commit.3.tmp, users can easily tell which one is the newest by looking at the number. Names with random numbers don't provide this information. We already have a different function to create truely randomly-named temporary files, svn_io_open_unique_file3(). It should be used instead of svn_io_open_uniquely_named() wherever doing so doesn't hurt user convenience (i.e. for any tempfile a user will never directly work with). Also, rand() isn't thread-safe, and this is library code. +1 on this reasoning and the -1. This merge should be reverted and performance critical code should use svn_io_open_unique_file3() instead of this function. (I think we already did this in most cases. In 1.6 this function was a severe performance problem) Bert
Re: svn commit: r1022931 - in /subversion/trunk/subversion/libsvn_wc: status.c wc-queries.sql wc_db.c wc_db.h
On Fri, Oct 15, 2010 at 4:19 PM, phi...@apache.org wrote: Author: philip Date: Fri Oct 15 14:19:36 2010 New Revision: 1022931 URL: http://svn.apache.org/viewvc?rev=1022931view=rev Log: Implement status using per-dir queries. On my machine (Linux, local disk) this improves the speed of status on a Subversion trunk working copy by a factor of 3, with a hot-cache, and about 30% with a cold cache. 1.7 is still slower than 1.6 for the hot-cache (but it's less than 100%), but for cold-cache 1.7 is now faster than 1.6. Now, that's more like it! On my machine (WinXP, 32 bit, local disk 5400 rpm), with a medium size working copy (944 dirs, 10286 files; same as I used in previous tests ([1])): - cold cache: 1.7 is almost 50% faster than 1.6 1.7: 22s 1.6: 42s - hot cache: 1.7 is just about on par with 1.6 (only 20% slower) 1.7: 0.86s 1.6: 0.72s Great work, Philip (and all the others who brought us up to this point of course)! Cheers, -- Johan [1] http://svn.haxx.se/dev/archive-2010-09/0067.shtml
RE: svn commit: r1022931 - in /subversion/trunk/subversion/libsvn_wc: status.c wc-queries.sql wc_db.c wc_db.h
On Fri, Oct 15, 2010 at 4:19 PM, phi...@apache.org wrote: Author: philip Date: Fri Oct 15 14:19:36 2010 New Revision: 1022931 URL: http://svn.apache.org/viewvc?rev=1022931view=rev Log: Implement status using per-dir queries. On my machine (Linux, local disk) this improves the speed of status on a Subversion trunk working copy by a factor of 3, with a hot-cache, and about 30% with a cold cache. 1.7 is still slower than 1.6 for the hot-cache (but it's less than 100%), but for cold-cache 1.7 is now faster than 1.6. Now, that's more like it! On my machine (WinXP, 32 bit, local disk 5400 rpm), with a medium size working copy (944 dirs, 10286 files; same as I used in previous tests ([1])): - cold cache: 1.7 is almost 50% faster than 1.6 1.7: 22s 1.6: 42s - hot cache: 1.7 is just about on par with 1.6 (only 20% slower) 1.7: 0.86s 1.6: 0.72s What do you guys mean by cold cache and hot cache? If they mean what I think they mean, wouldn't hot cache be faster that cold cache ? BOb
Re: svn commit: r1022931 - in /subversion/trunk/subversion/libsvn_wc: status.c wc-queries.sql wc_db.c wc_db.h
- cold cache: 1.7 is almost 50% faster than 1.6 1.7: 22s 1.6: 42s - hot cache: 1.7 is just about on par with 1.6 (only 20% slower) 1.7: 0.86s 1.6: 0.72s What do you guys mean by cold cache and hot cache? If they mean what I think they mean, wouldn't hot cache be faster that cold cache ? I think they are what you think. 22 seconds is slower than 1s, isn't it? Bye, Erik.
RE: svn commit: r1022931 - in /subversion/trunk/subversion/libsvn_wc: status.c wc-queries.sql wc_db.c wc_db.h
- cold cache: 1.7 is almost 50% faster than 1.6 1.7: 22s 1.6: 42s - hot cache: 1.7 is just about on par with 1.6 (only 20% slower) 1.7: 0.86s 1.6: 0.72s What do you guys mean by cold cache and hot cache? If they mean what I think they mean, wouldn't hot cache be faster that cold cache ? I think they are what you think. 22 seconds is slower than 1s, isn't it? DOH! I missed the .86s v 22s ... it's Friday afternoon... that is my excuse. Cheers, BOb
Re: svn commit: r1022931 - in /subversion/trunk/subversion/libsvn_wc: status.c wc-queries.sql wc_db.c wc_db.h
On 10/15/2010 05:26 PM, Bob Archer wrote: - cold cache: 1.7 is almost 50% faster than 1.6 1.7: 22s 1.6: 42s - hot cache: 1.7 is just about on par with 1.6 (only 20% slower) 1.7: 0.86s 1.6: 0.72s What do you guys mean by cold cache and hot cache? If they mean what I think they mean, wouldn't hot cache be faster that cold cache ? I think they are what you think. 22 seconds is slower than 1s, isn't it? DOH! I missed the .86s v 22s ... it's Friday afternoon... that is my excuse. Don't sweat it. Hey, with some luck, you're Saturday and Sunday will run slower than usual, too. I hear that works wonders on the mind. -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Distributed Development On Demand
Re: FREE Apache Subversion Meetup: Thursday, November 4, at ApacheCon NA in Atlanta, GA
woot! I'll be there, most definitely! On Mon, Oct 4, 2010 at 09:45, C. Michael Pilato cmpil...@collab.net wrote: For those who don't know, ApacheCon North America comes to Atlanta, GA, November 1-5 this year. You can find out more about the conference via the official conference website at http://na.apachecon.com/c/acna2010/. In addition to a full slate of scheduled presentations, talks, training sessions, etc., the ApacheCon organizers are again offering slots for MeetUps, which are somewhat less formal gatherings organized by a particular project's membership for the purpose of discussing the project itself with, ideally, a diverse group of participants. Oh, and they're free of charge to everyone -- you needn't even be a conference attendee to come to the Meetups! CollabNet is sponsoring an Apache Subversion Meetup at ApacheCon NA this year, currently scheduled for Thursday evening (Nov. 4). I'll be there, and I'd love to see as many of you as possible -- devs and users alike -- at the event. For the sake of efficiency, it's probably best to proceed with a pre-determined agenda, but that agenda is wide open to brief (15-minutes or less) presentations or discussion topics about anything Subversion-related (technical, social, or otherwise), to be offered by anyone who is willing and able to do so. The idea here is to use this two-hour window for whatever purposes best suit the project. And if we can do so while enjoying each other's company at a social level, too, bonus! The official ApacheCon NA 2010 Meetups page is here: http://wiki.apache.org/apachecon/ApacheMeetupsNa10 There, you can learn more about what the Meetups tend to look like, what other Meetups are planned for this years conference, and so on. You'll also find a link to the Subversion Meetup wiki page: http://subversion.open.collab.net/wiki/ApacheConNA2010Meetup I've populated that page with some information and some possible topics of interest. If you want to present on or lead the discussion about one of those, feel free to claim it. Got a new idea? Add it. We've got about a month to work out the details here. See you all in Atlanta! -- C. Michael Pilato cmpil...@collab.net CollabNet www.collab.net Distributed Development On Demand
