URL: https://github.com/freeipa/freeipa/pull/488
Author: tiran
 Title: #488: Speed up client schema cache
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/488/head:pr488
git checkout pr488
From d3c4393c7c97c6084435a6b072cf9dc553bdf4dd Mon Sep 17 00:00:00 2001
From: Christian Heimes <chei...@redhat.com>
Date: Mon, 20 Feb 2017 20:09:13 +0100
Subject: [PATCH 1/2] Speed up client schema cache

It's inefficient to open a zip file over and over again. By loading all
members of the schema cache file at once, the ipa CLI script starts
about 25 to 30% faster for simple cases like help and ping.

Before:

$ time for i in {1..20}; do ./ipa ping >/dev/null; done

real    0m13.608s
user    0m10.316s
sys     0m1.121s

After:

$ time for i in {1..20}; do ./ipa ping >/dev/null; done

real    0m9.330s
user    0m7.635s
sys     0m1.146s

https://fedorahosted.org/freeipa/ticket/6690

Signed-off-by: Christian Heimes <chei...@redhat.com>
---
 ipaclient/remote_plugins/schema.py | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index 15c03f4..13bdee4 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -458,11 +458,15 @@ def _read_schema(self, fingerprint):
         with self._open(fingerprint, 'rb') as f:
             self._file.write(f.read())
 
+        # It's more efficient to read zip file members at once than to open
+        # the zip file a couple of times, see #6690.
         with zipfile.ZipFile(self._file, 'r') as schema:
             for name in schema.namelist():
                 ns, _slash, key = name.partition('/')
                 if ns in self.namespaces:
-                    self._dict[ns][key] = None
+                    self._dict[ns][key] = schema.read(name)
+                elif name == '_help':
+                    self._help = schema.read(name)
 
     def __getitem__(self, key):
         try:
@@ -520,16 +524,12 @@ def _write_schema(self, fingerprint):
             f.truncate(0)
             f.write(self._file.read())
 
-    def _read(self, path):
-        with zipfile.ZipFile(self._file, 'r') as zf:
-            return json.loads(zf.read(path).decode('utf-8'))
-
     def read_namespace_member(self, namespace, member):
         value = self._dict[namespace][member]
 
-        if value is None:
-            path = '{}/{}'.format(namespace, member)
-            value = self._dict[namespace][member] = self._read(path)
+        if isinstance(value, bytes):
+            value = json.loads(value.decode('utf-8'))
+            self._dict[namespace][member] = value
 
         return value
 
@@ -537,8 +537,8 @@ def iter_namespace(self, namespace):
         return iter(self._dict[namespace])
 
     def get_help(self, namespace, member):
-        if not self._help:
-            self._help = self._read('_help')
+        if isinstance(self._help, bytes):
+            self._help = json.loads(self._help.decode('utf-8'))
 
         return self._help[namespace][member]
 

From 98f596083e1469915f5fd78b9a01164b5beb9d19 Mon Sep 17 00:00:00 2001
From: Christian Heimes <chei...@redhat.com>
Date: Tue, 28 Feb 2017 10:38:07 +0100
Subject: [PATCH 2/2] Drop in-memory copy of schema zip file

The schema cache used a BytesIO buffer to read/write schema cache before
it got flushed to disk. Since the schema cache is now loaded in one go,
the temporary buffer is no longer needed.

File locking has been replaced with a temporary file and atomic rename.

Signed-off-by: Christian Heimes <chei...@redhat.com>
---
 ipaclient/remote_plugins/schema.py | 49 ++++++++++++++------------------------
 1 file changed, 18 insertions(+), 31 deletions(-)

diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index 13bdee4..0cdce9d 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -3,13 +3,11 @@
 #
 
 import collections
-import contextlib
 import errno
-import fcntl
-import io
 import json
 import os
 import sys
+import tempfile
 import types
 import zipfile
 
@@ -374,7 +372,6 @@ def __init__(self, client, fingerprint=None):
         self._dict = {}
         self._namespaces = {}
         self._help = None
-        self._file = six.BytesIO()
 
         for ns in self.namespaces:
             self._dict[ns] = {}
@@ -404,21 +401,6 @@ def __init__(self, client, fingerprint=None):
         self.fingerprint = fingerprint
         self.ttl = ttl
 
-    @contextlib.contextmanager
-    def _open(self, filename, mode):
-        path = os.path.join(self._DIR, filename)
-
-        with io.open(path, mode) as f:
-            if mode.startswith('r'):
-                fcntl.flock(f, fcntl.LOCK_SH)
-            else:
-                fcntl.flock(f, fcntl.LOCK_EX)
-
-            try:
-                yield f
-            finally:
-                fcntl.flock(f, fcntl.LOCK_UN)
-
     def _fetch(self, client, ignore_cache=False):
         if not client.isconnected():
             client.connect(verbose=False)
@@ -454,13 +436,10 @@ def _fetch(self, client, ignore_cache=False):
         return (fp, ttl,)
 
     def _read_schema(self, fingerprint):
-        self._file.truncate(0)
-        with self._open(fingerprint, 'rb') as f:
-            self._file.write(f.read())
-
         # It's more efficient to read zip file members at once than to open
         # the zip file a couple of times, see #6690.
-        with zipfile.ZipFile(self._file, 'r') as schema:
+        filename = os.path.join(self._DIR, fingerprint)
+        with zipfile.ZipFile(filename, 'r') as schema:
             for name in schema.namelist():
                 ns, _slash, key = name.partition('/')
                 if ns in self.namespaces:
@@ -502,8 +481,21 @@ def _write_schema(self, fingerprint):
             if e.errno != errno.EEXIST:
                 raise
 
-        self._file.truncate(0)
-        with zipfile.ZipFile(self._file, 'w', zipfile.ZIP_DEFLATED) as schema:
+        with tempfile.NamedTemporaryFile('wb', prefix=fingerprint,
+                                         dir=self._DIR, delete=False) as f:
+            try:
+                self._write_schema_data(f)
+                f.flush()
+                os.fdatasync(f.fileno())
+                f.close()
+            except Exception:
+                os.unlink(f.name)
+                raise
+            else:
+                os.rename(f.name, os.path.join(self._DIR, fingerprint))
+
+    def _write_schema_data(self, fileobj):
+        with zipfile.ZipFile(fileobj, 'w', zipfile.ZIP_DEFLATED) as schema:
             for key, value in self._dict.items():
                 if key in self.namespaces:
                     ns = value
@@ -519,11 +511,6 @@ def _write_schema(self, fingerprint):
                 json.dumps(self._generate_help(self._dict)).encode('utf-8')
             )
 
-        self._file.seek(0)
-        with self._open(fingerprint, 'wb') as f:
-            f.truncate(0)
-            f.write(self._file.read())
-
     def read_namespace_member(self, namespace, member):
         value = self._dict[namespace][member]
 
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to