Bug 1177336 - Reduce system calls in mozfile.remove; r=ted
☠☠ backed out by 7f8adbc3897a ☠ ☠
authorGregory Szorc <gps@mozilla.com>
Wed, 01 Jul 2015 09:58:58 -0700
changeset 250900 5154443dcbeddda7580673e076d7b5e6d8b76542
parent 250899 39f41a68332b5ca4ce28088d6eae800516845889
child 250901 7f8adbc3897aebfb7832e45bbf0d975c78cd7756
push id13802
push usergszorc@mozilla.com
push dateWed, 01 Jul 2015 17:00:18 +0000
treeherderfx-team@5154443dcbed [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersted
bugs1177336
milestone42.0a1
Bug 1177336 - Reduce system calls in mozfile.remove; r=ted The os.path.* functions tend to perform os.stat() function calls behind the scenes. Python doesn't cache stat() results, so many of these function calls resulted in Python performing system calls. These system calls aren't exactly free. And, when we perform thousands or even hundreds of thousands of them (as we do when performing clobbers), they add up, especially on Windows. This commit refactors the code in mozfile.remove() to remove a significant number of stat() system calls. We do this by storing the initial stat() result and using the various stat.S_* functions to query its state rather than use the os.path.* functions. This code still isn't optimal. The os.walk() in particular is known to be very inefficient because its implementation will perform an os.stat() on each directory entry to see whether it is a file or directory. And, this stat() result is not passed to the caller, requiring it to perform another os.stat() to query additional metadata. The os.scandir() function (available in Python 3.5 and as a PyPI package) solves this properly. But we stop short of importing it into the tree and introducing a new dependency. On my MacBook Pro, this reduces the clobber of a Firefox objdir from ~6.8s to ~5.3s. Not a bad improvement! I suspect the improvement on Windows will be even more pronounced, as os.stat() there tends to be quite slow.
testing/mozbase/mozfile/mozfile/mozfile.py
--- a/testing/mozbase/mozfile/mozfile/mozfile.py
+++ b/testing/mozbase/mozfile/mozfile/mozfile.py
@@ -181,51 +181,56 @@ def remove(path):
 
                 print '%s() failed for "%s". Reason: %s (%s). Retrying...' % \
                         (func.__name__, args, e.strerror, e.errno)
                 time.sleep(retry_delay)
             else:
                 # If no exception has been thrown it should be done
                 break
 
-    def _update_permissions(path):
+    def _update_permissions(path, st):
         """Sets specified pemissions depending on filetype"""
         if os.path.islink(path):
             # Path is a symlink which we don't have to modify
             # because it should already have all the needed permissions
             return
 
-        stats = os.stat(path)
-
-        if os.path.isfile(path):
-            mode = stats.st_mode | stat.S_IWUSR
-        elif os.path.isdir(path):
-            mode = stats.st_mode | stat.S_IWUSR | stat.S_IXUSR
+        if stat.S_ISREG(st.st_mode):
+            mode = st.st_mode | stat.S_IWUSR
+        elif stat.S_ISDIR(st.st_mode):
+            mode = st.st_mode | stat.S_IWUSR | stat.S_IXUSR
         else:
             # Not supported type
             return
 
         _call_with_windows_retry(os.chmod, (path, mode))
 
-    if not os.path.exists(path):
+    try:
+        st = os.stat(path)
+    except os.error:
         return
 
-    if os.path.isfile(path) or os.path.islink(path):
+    is_link = os.path.islink(path)
+
+    if stat.S_ISREG(st.st_mode) or is_link:
         # Verify the file or link is read/write for the current user
-        _update_permissions(path)
+        if not is_link:
+            _update_permissions(path, st)
+
         _call_with_windows_retry(os.remove, (path,))
 
-    elif os.path.isdir(path):
+    elif stat.S_ISDIR(st.st_mode):
         # Verify the directory is read/write/execute for the current user
-        _update_permissions(path)
+        _update_permissions(path, st)
 
         # We're ensuring that every nested item has writable permission.
         for root, dirs, files in os.walk(path):
             for entry in dirs + files:
-                _update_permissions(os.path.join(root, entry))
+                fpath = os.path.join(root, entry)
+                _update_permissions(fpath, os.stat(fpath))
         _call_with_windows_retry(shutil.rmtree, (path,))
 
 
 def depth(directory):
     """returns the integer depth of a directory or path relative to '/' """
 
     directory = os.path.abspath(directory)
     level = 0