RE: Queries about rep cache: get_shared_rep()

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_at_gmail.com>
Sent: ‎26-‎5-‎2015 23:16
To: "dev" <dev_at_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
Received on 2015-05-27 00:09:40 CEST