Re: [Monotone-devel] slow update/status on NFS

[Markus Schiltknecht]

Hi,

first of all, many thanks to Zbynek and Matt, who pointed me to yet
another nice C++ feature I didn't know before (mutable).

I've implemented such a status cache for any_path in the attached patch.
 It finally works, except for the 'clone' command, for which this patch
breaks the test. I didn't care to fix those anymore...

The problem with this approach is, that upon modification of the
underlying file (including creation or removal), the cache for that path
needs to be cleared. That's done in file_io.cc, probably even more often
than really needed now, but at least, most test cases are fine now.

Anyway, this sort of 'cacheing' is prone to errors, because you always
need to remember to clear the cache. Plus it does not take external
modifications into account. As Nathaniel pointed out, it's probably much
better to reduce the stat() calls in our code, instead of putting yet
another complex system on top of what we have.

Is anyone opposed to Benoit's original patch? I think that goes towards
the right direction of removing the stat() calls in the current code.

Benoît Dejean wrote:
> > I just looked at the patch, and although I can see that it fixes
> > things for status, I think it could be taken one step further (and
> > thereby generalising the solution) by modifying the file_path class to
> > have a status cache.
>
> I'll try then.

Uh.. what exactly do you want to try?

Regards

Markus

#
# old_revision [389d9063f9f846eb147c1df1caeebc9b706eee08]
#
# patch "cmd_netsync.cc"
#  from [c2bc11a37824a01bb4ba40bbda1736e0301cd41c]
#    to [117afacde2a237d50cf269327765e8227767e300]
# 
# patch "database.cc"
#  from [d2603c50782ad0ee0f86fa96f58846608a6d9748]
#    to [bc51c275ef82195e9dbaa9a50be6f9716ab008a0]
# 
# patch "file_io.cc"
#  from [1edaa600f491b3cddb7628be8f0ab5d218971815]
#    to [16ba7a66b520c2575c9468b36e8d6b32dd320cb7]
# 
# patch "paths.cc"
#  from [0b81dfd4a65442de417c635cd8f003994af44ee1]
#    to [1ada7a63fbfaedd2b05a42b3b4905464830883a6]
# 
# patch "paths.hh"
#  from [5f2909f8cb250e4a5ddbb66eae949517c644d91f]
#    to [5524b98239406b688cdba136a1cde3c722a3a07c]
# 
# patch "platform.hh"
#  from [8d9be396f1a2d884491c63d92cf5c517145cab13]
#    to [af6aa39cca00886e6327015e9c9efea189d81754]
# 
# patch "work.cc"
#  from [9b78a7cb064436d0eed809eaa14c46b548b46096]
#    to [46e8fd7b421250c7a5347ec7480cf31b83d7d5f1]
#
============================================================
--- cmd_netsync.cc      c2bc11a37824a01bb4ba40bbda1736e0301cd41c
+++ cmd_netsync.cc      117afacde2a237d50cf269327765e8227767e300
@@ -248,7 +248,7 @@ CMD(clone, "clone", "", CMD_REF(network)
   else
     app.set_database(app.opts.dbname);
 
-  if (get_path_status(app.db.get_filename()) == path::nonexistent)
+  if (app.db.get_filename().get_status() == path::nonexistent)
     app.db.initialize();
 
   app.db.ensure_open();
============================================================
--- database.cc d2603c50782ad0ee0f86fa96f58846608a6d9748
+++ database.cc bc51c275ef82195e9dbaa9a50be6f9716ab008a0
@@ -3342,7 +3342,7 @@ database::check_db_exists()
 void
 database::check_db_exists()
 {
-  switch (get_path_status(filename))
+  switch (filename.get_status())
     {
     case path::nonexistent:
       N(false, F("database %s does not exist") % filename);
============================================================
--- file_io.cc  1edaa600f491b3cddb7628be8f0ab5d218971815
+++ file_io.cc  16ba7a66b520c2575c9468b36e8d6b32dd320cb7
@@ -55,19 +55,19 @@ assert_path_is_nonexistent(any_path cons
 void
 assert_path_is_nonexistent(any_path const & path)
 {
-  I(get_path_status(path) == path::nonexistent);
+  I(path.get_status() == path::nonexistent);
 }
 
 void
 assert_path_is_file(any_path const & path)
 {
-  I(get_path_status(path) == path::file);
+  I(path.get_status() == path::file);
 }
 
 void
 assert_path_is_directory(any_path const & path)
 {
-  I(get_path_status(path) == path::directory);
+  I(path.get_status() == path::directory);
 }
 
 void
@@ -82,7 +82,7 @@ require_path_is_file(any_path const & pa
                      i18n_format const & message_if_nonexistent,
                      i18n_format const & message_if_directory)
 {
-  switch (get_path_status(path))
+  switch (path.get_status())
     {
     case path::nonexistent:
       N(false, message_if_nonexistent);
@@ -100,7 +100,7 @@ require_path_is_directory(any_path const
                           i18n_format const & message_if_nonexistent,
                           i18n_format const & message_if_file)
 {
-  switch (get_path_status(path))
+  switch (path.get_status())
     {
     case path::nonexistent:
       N(false, message_if_nonexistent);
@@ -116,19 +116,19 @@ path_exists(any_path const & p)
 bool
 path_exists(any_path const & p)
 {
-  return get_path_status(p) != path::nonexistent;
+  return p.get_status() != path::nonexistent;
 }
 
 bool
 directory_exists(any_path const & p)
 {
-  return get_path_status(p) == path::directory;
+  return p.get_status() == path::directory;
 }
 
 bool
 file_exists(any_path const & p)
 {
-  return get_path_status(p) == path::file;
+  return p.get_status() == path::file;
 }
 
 bool
@@ -187,6 +187,7 @@ mkdir(any_path const & p)
 static fs::path
 mkdir(any_path const & p)
 {
+  p.clean_status_cache();
   return fs::path(p.as_external(), fs::native);
 }
 
@@ -195,13 +196,14 @@ mkdir_p(any_path const & p)
 {
   try
     {
+      p.clean_status_cache();
       fs::create_directories(mkdir(p));
     }
   catch (FS_ERROR & err)
     {
       // check for this case first, because in this case, the next line will
       // print "could not create directory: Success".  Which is unhelpful.
-      E(get_path_status(p) != path::file,
+      E(p.get_status() != path::file,
         F("could not create directory '%s'\nit is a file") % p);
       E(false,
         F("could not create directory '%s'\n%s")
@@ -215,6 +217,7 @@ make_dir_for(any_path const & p)
 void
 make_dir_for(any_path const & p)
 {
+  p.clean_status_cache();
   fs::path tmp(p.as_external(), fs::native);
   if (tmp.has_branch_path())
     {
@@ -285,6 +288,8 @@ move_file(any_path const & old_path,
                          "-- bug in monotone?") % old_path);
   require_path_is_nonexistent(new_path,
                               F("rename target '%s' already exists") % 
new_path);
+  old_path.clean_status_cache();
+  new_path.clean_status_cache();
   fs::rename(mkdir(old_path), mkdir(new_path));
 }
 
@@ -298,6 +303,8 @@ move_dir(any_path const & old_path,
                               "-- bug in monotone?") % old_path);
   require_path_is_nonexistent(new_path,
                               F("rename target '%s' already exists") % 
new_path);
+  old_path.clean_status_cache();
+  new_path.clean_status_cache();
   fs::rename(mkdir(old_path), mkdir(new_path));
 }
 
@@ -305,7 +312,7 @@ move_path(any_path const & old_path,
 move_path(any_path const & old_path,
           any_path const & new_path)
 {
-  switch (get_path_status(old_path))
+  switch (old_path.get_status())
     {
     case path::nonexistent:
       N(false, F("rename source path '%s' does not exist") % old_path);
@@ -403,6 +410,7 @@ write_data_impl(any_path const & p,
 
   make_dir_for(p);
 
+  p.clean_status_cache();
   write_data_worker(p.as_external(), dat(), tmp.as_external(), user_private);
 }
 
@@ -536,7 +544,7 @@ walk_tree(file_path const & path,
       return;
     }
 
-  switch (get_path_status(path))
+  switch (path.get_status())
     {
     case path::nonexistent:
       N(false, F("no such file or directory: '%s'") % path);
@@ -556,7 +564,7 @@ ident_existing_file(file_path const & p,
 bool
 ident_existing_file(file_path const & p, file_id & ident)
 {
-  switch (get_path_status(p))
+  switch (p.get_status())
     {
     case path::nonexistent:
       return false;
============================================================
--- paths.cc    0b81dfd4a65442de417c635cd8f003994af44ee1
+++ paths.cc    1ada7a63fbfaedd2b05a42b3b4905464830883a6
@@ -288,6 +288,7 @@ file_path::file_path(file_path::source_t
 }
 
 file_path::file_path(file_path::source_type type, string const & path)
+  : any_path()
 {
   string normalized;
   MM(path);
@@ -308,6 +309,7 @@ bookkeeping_path::bookkeeping_path(strin
 }
 
 bookkeeping_path::bookkeeping_path(string const & path)
+  : any_path()
 {
   I(fully_normalized_path(path));
   I(in_bookkeeping_dir(path));
@@ -345,6 +347,7 @@ file_path::file_path(split_path const & 
 //   p[0]/p[1]/.../p[n]
 //
 file_path::file_path(split_path const & sp)
+  : any_path()
 {
   split_path::const_iterator i = sp.begin();
   I(i != sp.end());
@@ -452,6 +455,20 @@ any_path::as_external() const
 #endif
 }
 
+path::status
+any_path::get_status() const
+{
+  if (cached_status == path::unknown)
+    cached_status = get_path_status(*this);
+  return cached_status;
+}
+
+void
+any_path::clean_status_cache() const
+{
+  cached_status = path::unknown;
+}
+
 ///////////////////////////////////////////////////////////////////////////
 // writing out paths
 ///////////////////////////////////////////////////////////////////////////
@@ -546,6 +563,7 @@ system_path::system_path(any_path const 
 }
 
 system_path::system_path(any_path const & other, bool in_true_workspace)
+  : any_path()
 {
   if (is_absolute_here(other.as_internal()))
     // another system_path.  the normalizing isn't really necessary, but it
@@ -573,11 +591,13 @@ system_path::system_path(string const & 
 }
 
 system_path::system_path(string const & path)
+  : any_path()
 {
   data = utf8(const_system_path(utf8(path)));
 }
 
 system_path::system_path(utf8 const & path)
+  : any_path()
 {
   data = utf8(const_system_path(utf8(path)));
 }
============================================================
--- paths.hh    5f2909f8cb250e4a5ddbb66eae949517c644d91f
+++ paths.hh    5524b98239406b688cdba136a1cde3c722a3a07c
@@ -124,6 +124,15 @@ null_name(path_component pc)
   return pc == the_null_component;
 }
 
+namespace path
+{
+  typedef enum { nonexistent = 0, directory = 1, file = 2 } status;
+
+  // Used for caching only - any_path::get_status() will never return status
+  // 'unknown', thus this value is not included in the enum above.
+  const status unknown = status(nonexistent - 1);
+};
+
 bool
 workspace_root(split_path const & sp);
 
@@ -143,13 +152,17 @@ public:
   { return data(); }
   bool empty() const
   { return data().empty(); }
+  path::status get_status() const;
+  void clean_status_cache() const;
 protected:
   utf8 data;
-  any_path() {}
+  mutable path::status cached_status;
+  any_path()
+    : cached_status(path::unknown) {}
   any_path(any_path const & other)
-    : data(other.data) {}
+    : data(other.data), cached_status(path::unknown) {}
   any_path & operator=(any_path const & other)
-  { data = other.data; return *this; }
+  { data = other.data; cached_status = other.cached_status; return *this; }
 };
 
 std::ostream & operator<<(std::ostream & o, any_path const & a);
@@ -158,7 +171,7 @@ public:
 class file_path : public any_path
 {
 public:
-  file_path() {}
+  file_path() : any_path() {}
   // join a file_path out of pieces
   file_path(split_path const & sp);
 
@@ -203,7 +216,7 @@ public:
 class bookkeeping_path : public any_path
 {
 public:
-  bookkeeping_path() {};
+  bookkeeping_path() : any_path() {};
   // path _should_ contain the leading _MTN/
   // and _should_ look like an internal path
   // usually you should just use the / operator as a constructor!
@@ -228,7 +241,7 @@ public:
 class system_path : public any_path
 {
 public:
-  system_path() {};
+  system_path() : any_path() {};
   system_path(system_path const & other) : any_path(other) {};
   // the optional argument takes some explanation.  this constructor takes a
   // path relative to the workspace root.  the question is how to interpret
============================================================
--- platform.hh 8d9be396f1a2d884491c63d92cf5c517145cab13
+++ platform.hh af6aa39cca00886e6327015e9c9efea189d81754
@@ -18,6 +18,8 @@
 #include <string>
 #include <stdio.h>
 
+#include "paths.hh"
+
 void read_password(std::string const & prompt, char * buf, size_t bufsz);
 void get_system_flavour(std::string & ident);
 bool is_executable(const char *path);
@@ -114,10 +116,7 @@ std::string get_homedir();
 std::string tilde_expand(std::string const & path);
 std::string get_default_confdir();
 std::string get_homedir();
-namespace path
-{
-  typedef enum { nonexistent, directory, file } status;
-};
+
 path::status get_path_status(std::string const & path);
 
 void rename_clobberingly(std::string const & from, std::string const & to);
============================================================
--- work.cc     9b78a7cb064436d0eed809eaa14c46b548b46096
+++ work.cc     46e8fd7b421250c7a5347ec7480cf31b83d7d5f1
@@ -581,7 +581,7 @@ addition_builder::add_node_for(split_pat
   file_path path(sp);
 
   node_id nid = the_null_node;
-  switch (get_path_status(path))
+  switch (path.get_status())
     {
     case path::nonexistent:
       return;
@@ -1307,7 +1307,7 @@ workspace::perform_additions(path_set co
           // in the case where we're just handled a set of paths, we use the 
builder
           // in this strange way.
           file_path path(*i);
-          switch (get_path_status(path))
+          switch (path.get_status())
             {
             case path::nonexistent:
               N(false, F("no such file or directory: '%s'") % path);
@@ -1464,7 +1464,7 @@ workspace::perform_rename(set<file_path>
       src_paths.begin()->split(s);
       N(new_roster.has_node(s),
         F("source file %s is not versioned") % s);
-      N(get_path_status(dst_path) != path::directory,
+      N(dst_path.get_status() != path::directory,
         F("destination name %s already exists as an unversioned directory") % 
dst);
       renames.insert( make_pair(s, dst) );
       add_parent_dirs(dst, new_roster, nis, db, lua);