You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2006/02/16 22:03:12 UTC
WC modification detection is reading whole files
For a locally-modified file with keyword translation enabled, I noticed that
"svn status" processes the locally-modified file every time I run "svn status".
Shouldn't it notice that the file size and date are not as expected and return
a "modified" status immediately?
svn_wc__versioned_file_modcheck() translates the file first (if KW or EOL
translation is necessary), and only after that checks its size.
That's called from svn_wc_text_modified_p2() which first checks the time stamps
and returns early if they are equal; afterwards it updates the time stamp if it
has a write lock.
The upshot is that every touched translatable file (e.g., on Windows, every
touched text file) is read and translated at every "svn status". If we were to
store the untranslated size, we could perhaps avoid this.
We even translate every touched file with eol-style=native on Linux. This is
because:
if (! svn_subst_translation_required (..., force_eol_check = TRUE))
Do we really need that force-eol-check?
Do we care about this inefficiency enough to do anything about it? Should I
file a low-priority issue?
Here's a practical example. In my Subversion trunk tree, with 11 files locally
modified:
$ svn st -q > /dev/null
$ time svn st -q > /dev/null
real 0m0.702s
user 0m0.464s
sys 0m0.228s
$ touch subversion/libsvn_*/*.[ch]
$ svn st -q > /dev/null
$ time svn st -q > /dev/null
real 0m1.764s
user 0m1.204s
sys 0m0.536s
These are in both cases repeatable times, when all the files are in the OS's
disk cache so the disk activity light doesn't even flicker.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: WC modification detection is reading whole files
Posted by kf...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> A lot of the above is discussed in notes/wc-improvements (that file is
> a little out-of-date since some of the property handling has already
> been implemented).
>
> I think caching the size of the unmodified working file would be a
> good thing. However if we stop doing a translation and byte-by-byte
> comparison we need to consider what happens when "modifications"
> disapear, e.g if I modify an expanded keyword and the file shows up as
> modified then what, if anything, gets committed if the file has no
> modifications when converted back to repository form?
Indeed. I just checked to make sure notes/wc-improvements mentioned
that, and it turns out lundblad added it in r18530, probably as a
result of this thread.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: WC modification detection is reading whole files
Posted by Philip Martin <ph...@codematters.co.uk>.
kfogel@collab.net writes:
> David James <dj...@collab.net> writes:
>> So if the timestamp is the same, but the size differs, we return "not
>> modified"? That seems strange to me.
>
> I may have put them in the wrong order. (Actually, I think we may
> have implemented it that way first, then realized that we could get
> the size at the same time as we get the timestamp, so we might as well
> check size first.)
>
> But now that I go look at the code, things are even weirder than that.
> The algorithm is implemented clearly only in libsvn_wc/props.c -- and
> it duplicates the stat() calls, sadly. Comments there confirm this
> was not done unknowingly, at least.
I added those comments to describe the behaviour, but I didn't write
the code.
> What about text bases? Are we really ignoring the filesizes there
> completely? Oh... no, I think I see: follow the logic in
> svn_wc_text_modified_p2() and you'll see what's going on. Again, we
> end up duplicating stat() calls. And we check the timestamp first,
> but only if no force_comparison flag was passed.
>
> This is somewhat dissatisfying. If we had a function that would use
> one stat() call to get both a file's size and its modtime, then we
> could do all this more efficiently. It seems we decided to let API
> cleanliness govern code flow.
>
> Hmmm. I'm not sure I want to do anything about this (other bugs to
> fix), but then again I'm not sure I want to *not* do anything about it
> either. Thoughts?
A lot of the above is discussed in notes/wc-improvements (that file is
a little out-of-date since some of the property handling has already
been implemented).
I think caching the size of the unmodified working file would be a
good thing. However if we stop doing a translation and byte-by-byte
comparison we need to consider what happens when "modifications"
disapear, e.g if I modify an expanded keyword and the file shows up as
modified then what, if anything, gets committed if the file has no
modifications when converted back to repository form?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: WC modification detection is reading whole files
Posted by kf...@collab.net.
David James <dj...@collab.net> writes:
> So if the timestamp is the same, but the size differs, we return "not
> modified"? That seems strange to me.
I may have put them in the wrong order. (Actually, I think we may
have implemented it that way first, then realized that we could get
the size at the same time as we get the timestamp, so we might as well
check size first.)
But now that I go look at the code, things are even weirder than that.
The algorithm is implemented clearly only in libsvn_wc/props.c -- and
it duplicates the stat() calls, sadly. Comments there confirm this
was not done unknowingly, at least.
What about text bases? Are we really ignoring the filesizes there
completely? Oh... no, I think I see: follow the logic in
svn_wc_text_modified_p2() and you'll see what's going on. Again, we
end up duplicating stat() calls. And we check the timestamp first,
but only if no force_comparison flag was passed.
This is somewhat dissatisfying. If we had a function that would use
one stat() call to get both a file's size and its modtime, then we
could do all this more efficiently. It seems we decided to let API
cleanliness govern code flow.
Hmmm. I'm not sure I want to do anything about this (other bugs to
fix), but then again I'm not sure I want to *not* do anything about it
either. Thoughts?
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: WC modification detection is reading whole files
Posted by David James <dj...@collab.net>.
On 16 Feb 2006 20:51:16 -0600, kfogel@collab.net <kf...@collab.net> wrote:
> Julian Foad <ju...@btopenworld.com> writes:
> > >>Shouldn't it notice that the file size and date are not as expected
> > >>and return a "modified" status immediately?
> > > Do we record the unmodified working file size for translated files in
> > > .svn/entries? If so, then we could detect modified-ness this way.
> >
> > We don't currently store any size information there. However, I'm now
> > thinking it's not so easy. (It would have been done if it were.)
> > Even if you modify the file by changing some keyword value or EOL
> > style, as long as it translates back to the same pristine text we must
> > report it as unchanged. Therefore the size doesn't tell us anything.
>
> Not so fast...
>
> A size check can tell you if something is definitely modified. It's
> only if the size comes back the same as what you recorded before that
> you have to do further investigation. The algorithm is:
>
> if (timestamp is same)
> return not_modified;
> else if (size differs)
> return modified;
> else
> go_into_expensive_further_investigation();
So if the timestamp is the same, but the size differs, we return "not
modified"? That seems strange to me.
--David
--
David James -- http://www.cs.toronto.edu/~james
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: WC modification detection is reading whole files
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
>>>>Shouldn't it notice that the file size and date are not as expected
>>>>and return a "modified" status immediately?
>>>
>>>Do we record the unmodified working file size for translated files in
>>>.svn/entries? If so, then we could detect modified-ness this way.
>>
>>We don't currently store any size information there. However, I'm now
>>thinking it's not so easy. (It would have been done if it were.)
>>Even if you modify the file by changing some keyword value or EOL
>>style, as long as it translates back to the same pristine text we must
>>report it as unchanged. Therefore the size doesn't tell us anything.
>
> Not so fast...
>
> A size check can tell you if something is definitely modified. It's
> only if the size comes back the same as what you recorded before that
> you have to do further investigation. The algorithm is:
>
> if (timestamp is same)
> return not_modified;
> else if (size differs)
> return modified;
> else
> go_into_expensive_further_investigation();
This is indeed the algorithm that we use, and it's fine for determining whether
a file without keyword or EOL translations matches its text base.
It doesn't work for determining whether a translated working file (that is, one
in which keyword expansions or EOL conversions are in effect) still matches its
pristine copy. If such a working file's time stamp and size have both changed
since the last time we looked at it, then certainly it's been changed, but we
don't know whether it will match its text base. It might, so we have to read
it and see.
That's the case I meant by "the size doesn't tell us anything", I didn't mean
in all cases.
David James wrote:
> So if the timestamp is the same, but the size differs, we return "not
> modified"? That seems strange to me.
Yes, but it's quite reasonable in practice. For one thing, it is only in
unusual situations that a file is modified but its time stamp is kept the same.
For another thing, if a file is modified but its time stamp is not, its size
doesn't necessarily change, and we don't intend to detect that case, so also
not detecting cases when the size has changed is only worse in magnitude, not
in concept.
That said, if we can read both time stamp and size together as Karl suggests,
then we certainly should do better by checking them both before making any
decision.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: WC modification detection is reading whole files
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> >>Shouldn't it notice that the file size and date are not as expected
> >>and return a "modified" status immediately?
> > Do we record the unmodified working file size for translated files in
> > .svn/entries? If so, then we could detect modified-ness this way.
>
> We don't currently store any size information there. However, I'm now
> thinking it's not so easy. (It would have been done if it were.)
> Even if you modify the file by changing some keyword value or EOL
> style, as long as it translates back to the same pristine text we must
> report it as unchanged. Therefore the size doesn't tell us anything.
Not so fast...
A size check can tell you if something is definitely modified. It's
only if the size comes back the same as what you recorded before that
you have to do further investigation. The algorithm is:
if (timestamp is same)
return not_modified;
else if (size differs)
return modified;
else
go_into_expensive_further_investigation();
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: WC modification detection is reading whole files
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> Julian Foad <ju...@btopenworld.com> writes:
>
>>For a locally-modified file with keyword translation enabled, I
>>noticed that "svn status" processes the locally-modified file every
>>time I run "svn status".
>>
>>Shouldn't it notice that the file size and date are not as expected
>>and return a "modified" status immediately?
>
> Do we record the unmodified working file size for translated files in
> .svn/entries? If so, then we could detect modified-ness this way.
We don't currently store any size information there. However, I'm now thinking
it's not so easy. (It would have been done if it were.) Even if you modify
the file by changing some keyword value or EOL style, as long as it translates
back to the same pristine text we must report it as unchanged. Therefore the
size doesn't tell us anything.
It's the time stamp that's useful. I think perhaps this is just a special case
of the general problem that we always re-read files that have been touched
because we don't update the stored time stamp during read-only operations.
That's issue #791, "text_modified_p should reset entry timestamps". It was
partly addressed by r7565 which (I feel, though it's not stated) made several
commands repair time stamps, and r12012 which added "cleanup" to the set of
commands that repair time stamps. No re-only operation (such as "status") has
been made to do so, and Erik closed the issue on the basis that we can't do any
better, but I think we can if we want to. (In the issue, Philip's list of
reasons why we might not be able to do so in some cases is perfectly
reasonable, and doesn't exclude most common cases.)
>> If we were to store the untranslated size, we could perhaps avoid this.
>
> Do you mean "store the translated size"? Or are you using
> "translated" to mean "translated back from working file to text base"?
> (I think of "translated" as being the base->working direction.)
I use "translated" to mean "converted from one form to another", just like wot
my dictionary says, of course :-)
I'll try to remember to use "pristine" (or "base") and "working" to refer to
the two particular forms, since translation happens in both directions and is
thus the term is frequently ambiguous.
> The timings you show seem pretty significant. I think it's worth
> doing something about, yes.
Maybe I should re-open issue #791. Maybe I should just learn that I can use
"svn cleanup" to restore fast operation. I'll think about it and maybe
experiment a bit more before doing anything.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: WC modification detection is reading whole files
Posted by kf...@collab.net.
Julian Foad <ju...@btopenworld.com> writes:
> For a locally-modified file with keyword translation enabled, I
> noticed that "svn status" processes the locally-modified file every
> time I run "svn status".
>
> Shouldn't it notice that the file size and date are not as expected
> and return a "modified" status immediately?
Do we record the unmodified working file size for translated files in
.svn/entries? If so, then we could detect modified-ness this way.
> svn_wc__versioned_file_modcheck() translates the file first (if KW
> or EOL translation is necessary), and only after that checks its
> size.
>
> That's called from svn_wc_text_modified_p2() which first checks the
> time stamps and returns early if they are equal; afterwards it
> updates the time stamp if it has a write lock.
>
> The upshot is that every touched translatable file (e.g., on
> Windows, every touched text file) is read and translated at every
> "svn status". If we were to store the untranslated size, we could
> perhaps avoid this.
Do you mean "store the translated size"? Or are you using
"translated" to mean "translated back from working file to text base"?
(I think of "translated" as being the base->working direction.)
> We even translate every touched file with eol-style=native on Linux.
> This is because:
>
> if (! svn_subst_translation_required (..., force_eol_check = TRUE))
>
> Do we really need that force-eol-check?
>
> Do we care about this inefficiency enough to do anything about it?
> Should I file a low-priority issue?
The timings you show seem pretty significant. I think it's worth
doing something about, yes.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: WC modification detection is reading whole files
Posted by Ivan Zhakov <ch...@gmail.com>.
On 2/17/06, Ivan Zhakov <ch...@gmail.com> wrote:
> On 2/17/06, Julian Foad <ju...@btopenworld.com> wrote:
> > For a locally-modified file with keyword translation enabled, I noticed that
> > "svn status" processes the locally-modified file every time I run "svn status".
[...]
> >
> > Here's a practical example. In my Subversion trunk tree, with 11 files locally
> > modified:
> >
> > $ svn st -q > /dev/null
> > $ time svn st -q > /dev/null
> > real 0m0.702s
> > user 0m0.464s
> > sys 0m0.228s
> >
> > $ touch subversion/libsvn_*/*.[ch]
> >
> > $ svn st -q > /dev/null
> > $ time svn st -q > /dev/null
> > real 0m1.764s
> > user 0m1.204s
> > sys 0m0.536s
> >
> > These are in both cases repeatable times, when all the files are in the OS's
> > disk cache so the disk activity light doesn't even flicker.
> Yes, this very bad. Many users reports that Subversion slow and most
> of such "slow" is WC.
>
> What about use new Erik's translation API in particular function
> svn_subst_stream_translated(). It returns stream for translated file,
> so we can avoid creating temporary translated files and compare text
> with text-base "on fly"?
> Yes, this doesn't help in #791 problem, but anyway improve performance
> in your case.
I have tested my idea and it works very well:
Original subversion trunk. WC with broken timestamps for all files:
C:\tmp\trans-test\wc\1.3.0>ptime svn st subversion
ptime 1.0 for Win32, Freeware - http://www.pc-tools.net/
Copyright(C) 2002, Jem Berkes <jb...@pc-tools.net>
=== svn st subversion ===
M subversion\include\svn_subst.h
M subversion\libsvn_wc\log.c
M subversion\libsvn_wc\adm_ops.c
M subversion\libsvn_wc\questions.c
M subversion\libsvn_subr\subst.c
Execution time: 1.769 s
Modified subversion as I proposed. WC with broken timestamps for all files also.
C:\tmp\trans-test\wc\trunk>ptime svn st subversion
ptime 1.0 for Win32, Freeware - http://www.pc-tools.net/
Copyright(C) 2002, Jem Berkes <jb...@pc-tools.net>
=== svn st subversion ===
M subversion\include\svn_subst.h
M subversion\libsvn_wc\log.c
M subversion\libsvn_wc\adm_ops.c
M subversion\libsvn_wc\questions.c
M subversion\libsvn_subr\subst.c
Execution time: 0.762 s
Same as Julian, I ran status several times, when all the files are in the OS's
disk cache.
But anyway more than 2 times improvement looks reasonable for me. I
need some tests with symlinks (I haven't it on my Windows) and code
cleanups. I'll commit this change or post patch in week.
--
Ivan Zhakov
Re: WC modification detection is reading whole files
Posted by Ivan Zhakov <ch...@gmail.com>.
On 2/17/06, Julian Foad <ju...@btopenworld.com> wrote:
> For a locally-modified file with keyword translation enabled, I noticed that
> "svn status" processes the locally-modified file every time I run "svn status".
>
> Shouldn't it notice that the file size and date are not as expected and return
> a "modified" status immediately?
>
> svn_wc__versioned_file_modcheck() translates the file first (if KW or EOL
> translation is necessary), and only after that checks its size.
In this we should store translated text base size in entries file.
Because translated text and text base sizes can be differrent, but
this doesn't mean that file modified.
>
> That's called from svn_wc_text_modified_p2() which first checks the time stamps
> and returns early if they are equal; afterwards it updates the time stamp if it
> has a write lock.
>
> The upshot is that every touched translatable file (e.g., on Windows, every
> touched text file) is read and translated at every "svn status". If we were to
> store the untranslated size, we could perhaps avoid this.
This wouldn't help, because same file size doesn't mean that file
content is equal.
>
> We even translate every touched file with eol-style=native on Linux. This is
> because:
>
> if (! svn_subst_translation_required (..., force_eol_check = TRUE))
>
> Do we really need that force-eol-check?
>
> Do we care about this inefficiency enough to do anything about it? Should I
> file a low-priority issue?
>
>
> Here's a practical example. In my Subversion trunk tree, with 11 files locally
> modified:
>
> $ svn st -q > /dev/null
> $ time svn st -q > /dev/null
> real 0m0.702s
> user 0m0.464s
> sys 0m0.228s
>
> $ touch subversion/libsvn_*/*.[ch]
>
> $ svn st -q > /dev/null
> $ time svn st -q > /dev/null
> real 0m1.764s
> user 0m1.204s
> sys 0m0.536s
>
> These are in both cases repeatable times, when all the files are in the OS's
> disk cache so the disk activity light doesn't even flicker.
Yes, this very bad. Many users reports that Subversion slow and most
of such "slow" is WC.
What about use new Erik's translation API in particular function
svn_subst_stream_translated(). It returns stream for translated file,
so we can avoid creating temporary translated files and compare text
with text-base "on fly"?
Yes, this doesn't help in #791 problem, but anyway improve performance
in your case.
--
Ivan Zhakov
Re: WC modification detection is reading whole files
Posted by Peter Samuelson <pe...@p12n.org>.
[Ph. Marek]
> How about storing an MD5 of, say, every 64kB of the translated file?
> Then a comparision wouldn't need to compare N bytes to N bytes (with
> approx. 2*N/2 read on average), but could quit after N/2 bytes read
> ... Seems an easy change to me.
I think that's overkill. Caching the mtime and size of the WC file
("translated") ought to be sufficient. Anyone who edits a file but
resets the mtime to its original value counts as "trying to break the
tools" and doesn't deserve special consideration.
I do note that mtime can legitimately go both forward and backward,
like if you use patch -Z or -T on your working copy. So, unlike
'make', the check needs to be for strict equality. (This is a flaw in
the classic 'make' behavior, IMO, but can't be fixed there as 'make'
keeps no state.)
As for resetting the timestamps to match the WC, I think I'm satisfied
knowing that this happens on 'svn cleanup'. This should perhaps be
more prominently documented as a way to speed up the client when you've
touched timestamps unnecessarily in your WC.
Re: WC modification detection is reading whole files
Posted by "Ph. Marek" <ph...@bmlv.gv.at>.
On Tuesday 21 February 2006 00:26, Branko Čibej wrote:
> >> These are in both cases repeatable times, when all the files are in the
> >> OS's disk cache so the disk activity light doesn't even flicker.
> >
> > How about storing an MD5 of, say, every 64kB of the translated file?
> > Then a comparision wouldn't need to compare N bytes to N bytes (with
> > approx. 2*N/2 read on average),
>
> Did you count the cost of computing the MD5?
As long as in cache:
$ ls -la /usr/bin/blender-bin
-rwxr-xr-x 1 root root 7336064 2006-02-07 12:43 /usr/bin/blender-bin
$ time md5sum /usr/bin/blender-bin
acd4c030ec56a84003e904adad9a8cd0 /usr/bin/blender-bin
real 0m0.064s
user 0m0.047s
sys 0m0.009s
on a P4 1.7GHz ... makes no time difference.
If it's not in cache ... harddisk is slower.
Regards,
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: WC modification detection is reading whole files
Posted by Branko Čibej <br...@xbc.nu>.
Ph. Marek wrote:
> On Thursday 16 February 2006 23:03, Julian Foad wrote:
>
>> For a locally-modified file with keyword translation enabled, I noticed
>> that "svn status" processes the locally-modified file every time I run "svn
>> status".
>>
> ...
>
>> Here's a practical example. In my Subversion trunk tree, with 11 files
>> locally modified:
>>
>> $ svn st -q > /dev/null
>> $ time svn st -q > /dev/null
>> real 0m0.702s
>> user 0m0.464s
>> sys 0m0.228s
>>
>> $ touch subversion/libsvn_*/*.[ch]
>>
>> $ svn st -q > /dev/null
>> $ time svn st -q > /dev/null
>> real 0m1.764s
>> user 0m1.204s
>> sys 0m0.536s
>>
>> These are in both cases repeatable times, when all the files are in the
>> OS's disk cache so the disk activity light doesn't even flicker.
>>
> How about storing an MD5 of, say, every 64kB of the translated file?
> Then a comparision wouldn't need to compare N bytes to N bytes (with approx.
> 2*N/2 read on average),
Did you count the cost of computing the MD5?
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: WC modification detection is reading whole files
Posted by "Ph. Marek" <ph...@bmlv.gv.at>.
On Thursday 16 February 2006 23:03, Julian Foad wrote:
> For a locally-modified file with keyword translation enabled, I noticed
> that "svn status" processes the locally-modified file every time I run "svn
> status".
...
> Here's a practical example. In my Subversion trunk tree, with 11 files
> locally modified:
>
> $ svn st -q > /dev/null
> $ time svn st -q > /dev/null
> real 0m0.702s
> user 0m0.464s
> sys 0m0.228s
>
> $ touch subversion/libsvn_*/*.[ch]
>
> $ svn st -q > /dev/null
> $ time svn st -q > /dev/null
> real 0m1.764s
> user 0m1.204s
> sys 0m0.536s
>
> These are in both cases repeatable times, when all the files are in the
> OS's disk cache so the disk activity light doesn't even flicker.
How about storing an MD5 of, say, every 64kB of the translated file?
Then a comparision wouldn't need to compare N bytes to N bytes (with approx.
2*N/2 read on average), but could quit after N/2 bytes read ...
Seems an easy change to me.
Of course, an arbitrary number of MD5-sums would have to be stored in the wc -
but having 16Byte for every 64kB is a factor of 1:4096, and that could be
changed to 1:N ... perhaps depending on the filesize (non-translated)?
That would speed-up smaller files too, if a file was cut in eg. 16 pieces and
each MD5'ed separately.
Or is there a reason why that wouldn't work?
Regards,
Phil
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org