subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bert Huijben <b...@qqmail.nl>
Subject RE: Queries about rep cache: get_shared_rep()
Date Tue, 26 May 2015 22:08:42 GMT
On Windows opening the file is sensitive to outside interactions and may trigger a retry loop.
E.g. A virusscanber that scans every file before opening by hooking the OS. A filestat is
+- a constant time operation that doesn't have these problems.

So this really depends on how common all cases are. If not existing or possibly locked is
common, then statting first could certainly be useful... But if it is +- an error if the file
doesn't exist and the file must be opened in almost every case then the open and handle errors
keeps the code clean.

Not sure what case applies here, but without looking at the code I would guess that in >
99.9% of the cases we can assume the cache is correct... And all else is an exception that
might have a slight performance hit.

Bert

-----Original Message-----
From: "Julian Foad" <julianfoad@gmail.com>
Sent: ‎26-‎5-‎2015 23:16
To: "dev" <dev@subversion.apache.org>
Subject: Queries about rep cache: get_shared_rep()

Index: subversion/libsvn_fs_fs/transaction.c
===================================================================
--- subversion/libsvn_fs_fs/transaction.c    (revision 1681856)
+++ subversion/libsvn_fs_fs/transaction.c    (working copy)
@@ -2243,12 +2243,14 @@ (representation_t **old_re
       const char *file_name
         = path_txn_sha1(fs, &rep->txn_id, rep->sha1_digest, scratch_pool);

       /* in our txn, is there a rep file named with the wanted SHA1?
          If so, read it and use that rep.
        */
+      /* ### Would it be faster (and so better) to just try reading it,
+             and handle ENOENT, instead of first checking for presence? */
       SVN_ERR(svn_io_check_path(file_name, &kind, scratch_pool));
       if (kind == svn_node_file)
         {
           svn_stringbuf_t *rep_string;
           SVN_ERR(svn_stringbuf_from_file2(&rep_string, file_name,
                                            scratch_pool));
@@ -2262,14 +2264,20 @@ get_shared_rep(representation_t **old_re

   /* A simple guard against general rep-cache induced corruption. */
   if ((*old_rep)->expanded_size != rep->expanded_size)
     {
       /* Make the problem show up in the server log.

-         Because not sharing reps is always a save option,
+         Because not sharing reps is always a safe option,
          terminating the request would be inappropriate.
+
+         ### [JAF] Why should we assume the cache is corrupt rather than the
+                   new rep is corrupt? Is this really the logic we want?
+
+                   Should we do something more forceful -- flag the cache as
+                   unusable, perhaps -- rather than just logging a warning?
        */
       svn_checksum_t checksum;
       checksum.digest = rep->sha1_digest;
       checksum.kind = svn_checksum_sha1;

       err = svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,


- Julian

Mime
View raw message