shell/Windows: Avoid memory allocations and copies when querying icons

IsMemberOf is called for every file (in the ownCloud directory or not) and
with every instance of OCOverlay (we have 5) when displaying a list of
files in Explorer.

Refactor the code to avoid copying the list of watched directories, as
well as creating a wstring from a PWWSTR for files outside watched
directories.

Also change some calls of begin_with to use isDescendantOf since it
properly handles parent paths not ending with a backslash, which could
lead to SocketAPI queries for sibling folders with a name that starts with
a watched folder name.

diff --git a/shell_integration/windows/OCContextMenu/OCContextMenu.cpp b/shell_integration/windows/OCContextMenu/OCContextMenu.cpp
index 65ff203e1d941b701c0d42bc09e4b47f4242d558..a5558fc0a3e83e714009685a909c437fdb3ed69c 100644 (file)
--- a/shell_integration/windows/OCContextMenu/OCContextMenu.cpp
+++ b/shell_integration/windows/OCContextMenu/OCContextMenu.cpp
@@ -152,8 +152,9 @@ IFACEMETHODIMP OCContextMenu::QueryContextMenu(HMENU hMenu, UINT indexMenu, UINT
 
     OCClientInterface::ContextMenuInfo info = OCClientInterface::FetchInfo();
     bool skip = true;
+    size_t selectedFileLength = wcslen(m_szSelectedFile);
     for (const std::wstring path : info.watchedDirectories) {
-        if (StringUtil::begins_with(std::wstring(m_szSelectedFile), path)) {
+        if (StringUtil::isDescendantOf(m_szSelectedFile, selectedFileLength, path)) {
             skip = false;
             break;
         }

diff --git a/shell_integration/windows/OCOverlays/OCOverlay.cpp b/shell_integration/windows/OCOverlays/OCOverlay.cpp
index 1a07205bf5ef0d36252a4206dfbb44f5ec02c039..dd03a8664df5db37017093d802210a2f4a8778c2 100644 (file)
--- a/shell_integration/windows/OCOverlays/OCOverlay.cpp
+++ b/shell_integration/windows/OCOverlays/OCOverlay.cpp
@@ -126,17 +126,19 @@ IFACEMETHODIMP OCOverlay::GetPriority(int *pPriority)
     return S_OK;
 }
 
- IFACEMETHODIMP OCOverlay::IsMemberOf(PCWSTR pwszPath, DWORD dwAttrib)
+IFACEMETHODIMP OCOverlay::IsMemberOf(PCWSTR pwszPath, DWORD dwAttrib)
 {
     RemotePathChecker* checker = getGlobalChecker();
-    auto watchedDirectories = checker->WatchedDirectories();
+    std::shared_ptr<const std::vector<std::wstring>> watchedDirectories = checker->WatchedDirectories();
+
+    if (watchedDirectories->empty()) {
+        return MAKE_HRESULT(S_FALSE, 0, 0);
+    }
 
-    wstring wpath(pwszPath);
-    wpath.append(L"\\");
-    vector<wstring>::iterator it;
     bool watched = false;
-    for (it = watchedDirectories.begin(); it != watchedDirectories.end(); ++it) {
-        if (StringUtil::begins_with(wpath, *it)) {
+    size_t pathLength = wcslen(pwszPath);
+    for (auto it = watchedDirectories->begin(); it != watchedDirectories->end(); ++it) {
+        if (StringUtil::isDescendantOf(pwszPath, pathLength, *it)) {
             watched = true;
         }
     }
@@ -166,20 +168,3 @@ IFACEMETHODIMP OCOverlay::GetOverlayInfo(PWSTR pwszIconFile, int cchMax, int *pI
 
     return S_OK;
 }
-
-
-bool OCOverlay::_IsOverlaysEnabled()
-{
-    //int enable;
-    bool success = false;
-
-    //if(RegistryUtil::ReadRegistry(REGISTRY_ROOT_KEY, REGISTRY_ENABLE_OVERLAY, &enable))
-    //{
-    //  if(enable) {
-    //      success = true;
-    //  }
-    //}
-
-    return success;
-}
-

diff --git a/shell_integration/windows/OCOverlays/OCOverlay.h b/shell_integration/windows/OCOverlays/OCOverlay.h
index fc8bc4104a04c19efee240dba496e6aaa57d7482..af1aaf696be5098867f36b685c8e04f7cf1b5b43 100644 (file)
--- a/shell_integration/windows/OCOverlays/OCOverlay.h
+++ b/shell_integration/windows/OCOverlays/OCOverlay.h
@@ -34,7 +34,6 @@ protected:
     ~OCOverlay();
 
 private:
-    bool _IsOverlaysEnabled();
     long _referenceCount;
     int _state;
 };

diff --git a/shell_integration/windows/OCUtil/RemotePathChecker.cpp b/shell_integration/windows/OCUtil/RemotePathChecker.cpp
index 679686ec7f93ff0fda1c9cf9edccacde74da2249..cf8b72bbdd68b9188fef4b89189b909ea04413d6 100644 (file)
--- a/shell_integration/windows/OCUtil/RemotePathChecker.cpp
+++ b/shell_integration/windows/OCUtil/RemotePathChecker.cpp
@@ -75,25 +75,30 @@ void RemotePathChecker::workerThreadLoop()
             if (StringUtil::begins_with(response, wstring(L"REGISTER_PATH:"))) {
                 wstring responsePath = response.substr(14); // length of REGISTER_PATH:
 
-                {   std::unique_lock<std::mutex> lock(_mutex);
-                    _watchedDirectories.push_back(responsePath);
-                }
-                // We don't keep track of all files and can't know which file is currently visible to the user,
-                // but at least reload the root dir + itself so that any shortcut to the root is updated without
-                // the user needing to navigate.
+                auto sharedPtrCopy = atomic_load(&_watchedDirectories);
+                auto vectorCopy = make_shared<vector<wstring>>(*sharedPtrCopy);
+                vectorCopy->push_back(responsePath);
+                atomic_store(&_watchedDirectories, shared_ptr<const vector<wstring>>(vectorCopy));
+
+                // We don't keep track of all files and can't know which file is currently visible
+                // to the user, but at least reload the root dir so that any shortcut to the root
+                // is updated without the user needing to refresh.
                 SHChangeNotify(SHCNE_UPDATEDIR, SHCNF_PATH | SHCNF_FLUSHNOWAIT, responsePath.data(), NULL);
             } else if (StringUtil::begins_with(response, wstring(L"UNREGISTER_PATH:"))) {
                 wstring responsePath = response.substr(16); // length of UNREGISTER_PATH:
 
+                auto sharedPtrCopy = atomic_load(&_watchedDirectories);
+                auto vectorCopy = make_shared<vector<wstring>>(*sharedPtrCopy);
+                vectorCopy->erase(
+                    std::remove(vectorCopy->begin(), vectorCopy->end(), responsePath),
+                    vectorCopy->end());
+                atomic_store(&_watchedDirectories, shared_ptr<const vector<wstring>>(vectorCopy));
+
                 vector<wstring> removedPaths;
                 {   std::unique_lock<std::mutex> lock(_mutex);
-                    _watchedDirectories.erase(
-                        std::remove(_watchedDirectories.begin(), _watchedDirectories.end(), responsePath),
-                        _watchedDirectories.end());
-
                     // Remove any item from the cache
                     for (auto it = _cache.begin(); it != _cache.end() ; ) {
-                        if (StringUtil::begins_with(it->first, responsePath)) {
+                        if (StringUtil::isDescendantOf(it->first, responsePath)) {
                             removedPaths.emplace_back(move(it->first));
                             it = _cache.erase(it);
                         } else {
@@ -143,8 +148,8 @@ void RemotePathChecker::workerThreadLoop()
         }
 
         if (socket.Event() == INVALID_HANDLE_VALUE) {
+            atomic_store(&_watchedDirectories, make_shared<const vector<wstring>>());
             std::unique_lock<std::mutex> lock(_mutex);
-            _watchedDirectories.clear();
             _connected = connected = false;
 
             // Swap to make a copy of the cache under the mutex and clear the one stored.
@@ -167,7 +172,8 @@ void RemotePathChecker::workerThreadLoop()
 
 
 RemotePathChecker::RemotePathChecker()
-    : _connected(false)
+    : _watchedDirectories(make_shared<const vector<wstring>>())
+    , _connected(false)
     , _newQueries(CreateEvent(NULL, true, true, NULL))
     , _thread([this]{ this->workerThreadLoop(); })
 {
@@ -182,10 +188,9 @@ RemotePathChecker::~RemotePathChecker()
     CloseHandle(_newQueries);
 }
 
-vector<wstring> RemotePathChecker::WatchedDirectories()
+std::shared_ptr<const std::vector<std::wstring>> RemotePathChecker::WatchedDirectories() const
 {
-    std::unique_lock<std::mutex> lock(_mutex);
-    return _watchedDirectories;
+    return atomic_load(&_watchedDirectories);
 }
 
 bool RemotePathChecker::IsMonitoredPath(const wchar_t* filePath, int* state)

diff --git a/shell_integration/windows/OCUtil/RemotePathChecker.h b/shell_integration/windows/OCUtil/RemotePathChecker.h
index e85629f9ad057252ffb0504a905dcc093c332b9b..36bcf1ae9bed655e077394c8fd131f7ea756e2c3 100644 (file)
--- a/shell_integration/windows/OCUtil/RemotePathChecker.h
+++ b/shell_integration/windows/OCUtil/RemotePathChecker.h
@@ -19,6 +19,7 @@
 #include <unordered_map>
 #include <queue>
 #include <thread>
+#include <memory>
 #include <mutex>
 #include <atomic>
 #include <condition_variable>
@@ -37,7 +38,7 @@ public:
     };
     RemotePathChecker();
     ~RemotePathChecker();
-    std::vector<std::wstring> WatchedDirectories();
+    std::shared_ptr<const std::vector<std::wstring>> WatchedDirectories() const;
     bool IsMonitoredPath(const wchar_t* filePath, int* state);
 
 private:
@@ -52,7 +53,9 @@ private:
     std::queue<std::wstring> _pending;
 
     std::unordered_map<std::wstring, FileState> _cache;
-    std::vector<std::wstring> _watchedDirectories;
+    // The vector is const since it will be accessed from multiple threads through OCOverlay::IsMemberOf.
+    // Each modification needs to be made onto a copy and then atomically replaced in the shared_ptr.
+    std::shared_ptr<const std::vector<std::wstring>> _watchedDirectories;
     bool _connected;
 
 

diff --git a/shell_integration/windows/OCUtil/StringUtil.h b/shell_integration/windows/OCUtil/StringUtil.h
index 8614639ac4a4f8a4e5e2931b20ccd8f9e4c8dbd8..0f52d51dd6bc86181929bdda86d27ef0f62ed8ac 100644 (file)
--- a/shell_integration/windows/OCUtil/StringUtil.h
+++ b/shell_integration/windows/OCUtil/StringUtil.h
@@ -29,6 +29,21 @@ public:
         return input.size() >= match.size()
             && std::equal(match.begin(), match.end(), input.begin());
     }
+
+    static bool isDescendantOf(const std::wstring& child, const std::wstring& parent) {
+        return isDescendantOf(child.c_str(), child.size(), parent.c_str(), parent.size());
+    }
+
+    static bool isDescendantOf(PCWSTR child, size_t childLength, const std::wstring& parent) {
+        return isDescendantOf(child, childLength, parent.c_str(), parent.size());
+    }
+
+    static bool isDescendantOf(PCWSTR child, size_t childLength, PCWSTR parent, size_t parentLength) {
+        if (!parentLength)
+            return false;
+        return (childLength == parentLength || childLength > parentLength && (child[parentLength] == L'\\' || child[parentLength - 1] == L'\\'))
+            && wcsncmp(child, parent, parentLength) == 0;
+    }
 };
 
 #endif // STRINGUTIL_H