On 15/07/16 12:53, David Kupka wrote:
Hello!

After Honza introduced thin client that builds plugins and commands
dynamically from schema client became much slower. This is only logical,
instead of importing a module client now must fetch the schema from
server, parse it and instantiate the commands using the data.

First step to speed it up was addition of schema cache to client. That
removed the RTT and download time of fetching schema every time.

Now the most time consuming task became displaying help for lists of
topics and command and displaying individual topics. This is simply
because of the need to instantiate all the commands to find the
relations between topics and commands.

All the necessary bits for server commands and topics are already in the
schema cache so we can skip this part and generate help from it, right?
Not so fast!

There are client plugins with commands and topics. So we can generate
basic bits (list of all topics, list of all commands, list of commands
for each topic) from schema and store it in cache. Then we need to go
through all client plugins and get similar bits for client plugins. Then
we can merge and print.

Still the client response is not as fast as before and I this it even
can't be. Also first time you display particular topic or list takes
longer because it must be freshly generated and stored in cache for next
use. And this is what the attached patches do.

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

Reimplemented so there is no need to distinguish client plugins and remote plugins. The main idea of this approach is to avoid creating instances of the commands just to get the information about topic, name and summary needed for displaying help. Instead class properties are used to access the information directly in schema.

--
David Kupka
From 8eefe955f7e961e1a4e55b823772383243c029a5 Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Tue, 19 Jul 2016 09:34:35 +0200
Subject: [PATCH 1/6] schema: Read data from cache only once

Keep data when read from storage to avoid unnecessary reads.

https://fedorahosted.org/freeipa/ticket/6048
---
 ipaclient/remote_plugins/schema.py | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index cd1d5d607978899254325f634ccec91d2c92f59b..09f041592f04e65544cf65647f28f395a2f9e4a7 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -306,9 +306,17 @@ class _SchemaNameSpace(collections.Mapping):
     def __init__(self, schema, name):
         self.name = name
         self._schema = schema
+        self._dict = {}
 
     def __getitem__(self, key):
-        return self._schema.read_namespace_member(self.name, key)
+        try:
+            return self._dict[key]
+        except KeyError:
+            self._dict[key] = self._schema.read_namespace_member(
+                self.name,
+                key
+            )
+            return self._dict[key]
 
     def __iter__(self):
         for key in self._schema.iter_namespace(self.name):
-- 
2.7.4

From fe155db233456b2d6d917a010177eb382a3de58f Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Wed, 20 Jul 2016 09:54:45 +0200
Subject: [PATCH 2/6] schema: Speed up cache verification

Check schema only once for each api instance.

https://fedorahosted.org/freeipa/ticket/6048
---
 ipaclient/remote_plugins/schema.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index 09f041592f04e65544cf65647f28f395a2f9e4a7..92430950f96be266ebaa2f238de226e1c02b8e6c 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -403,6 +403,9 @@ class Schema(object):
         return (fp, exp)
 
     def _ensure_cached(self):
+        if getattr(self, '_schema_cached', False):
+            return
+
         no_info = False
         try:
             # pylint: disable=access-member-before-definition
@@ -422,13 +425,9 @@ class Schema(object):
                     logger.warning('Failed to load server properties: {}'
                                    ''.format(e))
 
-        force_check = ((not getattr(self, '_schema_checked', False)) and
-                       self._api.env.force_schema_check)
-
-        if (force_check or
+        if (self._api.env.force_schema_check or
                 no_info or exp < time.time() or not Schema._in_cache(fp)):
             (fp, exp) = self._get_schema()
-            self._schema_checked = True
             _ensure_dir_created(SERVERS_DIR)
             try:
                 with self._open_server_info(self._api.env.server, 'w') as sc:
@@ -436,6 +435,7 @@ class Schema(object):
             except Exception as e:
                 logger.warning('Failed to store server properties: {}'
                                ''.format(e))
+        self._schema_cached = True
 
         if not self._dict:
             self._dict['fingerprint'] = fp
-- 
2.7.4

From 884685435ad836a112db3a8a863416fd0c67d354 Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Tue, 19 Jul 2016 09:22:57 +0200
Subject: [PATCH 3/6] schema: Generate and store bits for help

Store information needed for help on single place to avoid extensive
number of read operations.

https://fedorahosted.org/freeipa/ticket/6048
---
 ipaclient/remote_plugins/schema.py | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index 92430950f96be266ebaa2f238de226e1c02b8e6c..ceba41a84ce4aef48b206e1d0bdc580c06c0c3cb 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -370,6 +370,7 @@ class Schema(object):
         self._api = api
         self._client = client
         self._dict = {}
+        self._help = None
 
     def _open_server_info(self, hostname, mode):
         encoded_hostname = DNSName(hostname).ToASCII()
@@ -451,6 +452,27 @@ class Schema(object):
         self._ensure_cached()
         return self._dict[key]
 
+    def _generate_help(self, schema):
+        halp = {}
+
+        for namespace in ('commands', 'topics'):
+            halp[namespace] = {}
+
+            for member_schema in schema[namespace]:
+                member_full_name = member_schema['full_name']
+
+                topic = halp[namespace].setdefault(member_full_name, {})
+                topic['name'] = member_schema['name']
+                if 'doc' not in member_schema:
+                    topic['summary'] = u'<%s>' % member_full_name
+                else:
+                    topic['summary'] = (
+                        member_schema['doc'].split('\n\n', 1)[0].strip())
+                topic['topic'] = member_schema.get('topic_topic')
+                topic['NO_CLI'] = 'cli' in member_schema.get('exclude', [])
+
+        return halp
+
     def _open_archive(self, mode, fp=None):
         if not fp:
             fp = self['fingerprint']
@@ -474,6 +496,7 @@ class Schema(object):
                         member['full_name']
                     )
                     zf.writestr(path, json.dumps(member))
+            zf.writestr('help', json.dumps(self._generate_help(schema)))
 
     def _read(self, path):
         with self._open_archive('r') as zf:
@@ -491,6 +514,12 @@ class Schema(object):
                 if r:
                     yield r.groups('name')[0]
 
+    def get_help(self, namespace, member):
+        if self._help is None:
+            self._help = self._read('help')
+
+        return self._help[namespace][member]
+
 
 def get_package(api, client):
     try:
-- 
2.7.4

From 3b32ab0aac928885f3c3c826d766b59a1efbe9d8 Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Wed, 20 Jul 2016 09:52:16 +0200
Subject: [PATCH 4/6] schema: Introduce format versioning for api schema cache

https://fedorahosted.org/freeipa/ticket/6048
---
 ipaclient/remote_plugins/schema.py | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index ceba41a84ce4aef48b206e1d0bdc580c06c0c3cb..20d7cf7175373f9e2a87ceee273a9ad8694c238a 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -25,6 +25,8 @@ from ipapython.dn import DN
 from ipapython.dnsutil import DNSName
 from ipapython.ipa_log_manager import log_mgr
 
+FORMAT = '1'
+
 if six.PY3:
     unicode = str
 
@@ -355,7 +357,6 @@ class Schema(object):
     ns_member_pattern_template = '^{}/(?P<name>.+)$'
     ns_member_path_template = '{}/{}'
     namespaces = {'classes', 'commands', 'topics'}
-    schema_info_path = 'schema'
 
     @classmethod
     def _list(cls):
@@ -363,8 +364,17 @@ class Schema(object):
             yield os.path.splitext(os.path.basename(f))[0]
 
     @classmethod
-    def _in_cache(cls, fingeprint):
-        return os.path.exists(cls.schema_path_template.format(fingeprint))
+    def _valid(cls, fp):
+        try:
+            arch_path = cls.schema_path_template.format(fp)
+            with _LockedZipFile(arch_path, 'r') as zf:
+                fmt = json.loads(zf.read('format'))
+        except EnvironmentError:
+            return False
+        except KeyError:
+            fmt = '0'
+
+        return fmt == FORMAT
 
     def __init__(self, api, client):
         self._api = api
@@ -382,7 +392,7 @@ class Schema(object):
         if not client.isconnected():
             client.connect(verbose=False)
 
-        fps = [unicode(f) for f in Schema._list()]
+        fps = [unicode(f) for f in self._list() if self._valid(f)]
         kwargs = {u'version': u'2.170'}
         if fps:
             kwargs[u'known_fingerprints'] = fps
@@ -427,7 +437,7 @@ class Schema(object):
                                    ''.format(e))
 
         if (self._api.env.force_schema_check or
-                no_info or exp < time.time() or not Schema._in_cache(fp)):
+                no_info or exp < time.time() or not self._valid(fp)):
             (fp, exp) = self._get_schema()
             _ensure_dir_created(SERVERS_DIR)
             try:
@@ -440,8 +450,7 @@ class Schema(object):
 
         if not self._dict:
             self._dict['fingerprint'] = fp
-            schema_info = self._read(self.schema_info_path)
-            self._dict['version'] = schema_info['version']
+            self._dict['version'] = self._read('version')
             for ns in self.namespaces:
                 self._dict[ns] = _SchemaNameSpace(self, ns)
 
@@ -482,12 +491,11 @@ class Schema(object):
     def _store(self, fingerprint, schema={}):
         _ensure_dir_created(SCHEMA_DIR)
 
-        schema_info = dict(version=schema['version'],
-                           fingerprint=schema['fingerprint'])
-
         with self._open_archive('w', fingerprint) as zf:
-            # store schema information
-            zf.writestr(self.schema_info_path, json.dumps(schema_info))
+            # store schema cache information
+            zf.writestr('version', json.dumps(schema['version']))
+            zf.writestr('fingerprint', json.dumps(schema['fingerprint']))
+            zf.writestr('format', json.dumps(FORMAT))
             # store namespaces
             for namespace in self.namespaces:
                 for member in schema[namespace]:
-- 
2.7.4

From 73953d68aae7d31c679095640080fea12e1c7e0c Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Wed, 20 Jul 2016 13:23:33 +0200
Subject: [PATCH 5/6] frontend: Change summary, topic and NO_CLI to class
 properties

Avoid need to instantiate all commands just to get informations for
displaying help.

https://fedorahosted.org/freeipa/ticket/6048
---
 ipaclient/frontend.py                 | 24 +++++++++++++++++-------
 ipaclient/plugins/automount.py        |  9 ++++++---
 ipaclient/plugins/otptoken_yubikey.py | 11 +++++++----
 ipaclient/remote_plugins/schema.py    | 33 ++++++++++++++++++++++++++++-----
 ipalib/frontend.py                    | 10 ++++++----
 ipalib/plugable.py                    | 17 ++++++++++-------
 6 files changed, 74 insertions(+), 30 deletions(-)

diff --git a/ipaclient/frontend.py b/ipaclient/frontend.py
index 1525c88b3dfeadccd8115cb4b6ba149caef22103..bb81f47be8bf14cfa41735d483d6fd303f808978 100644
--- a/ipaclient/frontend.py
+++ b/ipaclient/frontend.py
@@ -2,9 +2,11 @@
 # Copyright (C) 2016  FreeIPA Contributors see COPYING for license
 #
 
+from ipalib import api
 from ipalib.frontend import Command, Method
 from ipalib.parameters import Str
 from ipalib.text import _
+from ipalib.util import classproperty
 
 
 class ClientCommand(Command):
@@ -111,20 +113,28 @@ class CommandOverride(Command):
     def __init__(self, api):
         super(CommandOverride, self).__init__(api)
 
-        next_class = api.get_plugin_next(type(self))
+        next_class = self.__get_next()
         self.next = next_class(api)
 
+    @classmethod
+    def __get_next(cls):
+        return api.get_plugin_next(cls)
+
     @property
     def doc(self):
         return self.next.doc
 
-    @property
-    def NO_CLI(self):
-        return self.next.NO_CLI
+    @classmethod
+    def __NO_CLI_getter(cls):
+        return cls.__get_next().NO_CLI
 
-    @property
-    def topic(self):
-        return self.next.topic
+    NO_CLI = classproperty(__NO_CLI_getter)
+
+    @classmethod
+    def __topic_getter(cls):
+        return cls.__get_next().topic
+
+    topic = classproperty(__topic_getter)
 
     @property
     def forwarded_name(self):
diff --git a/ipaclient/plugins/automount.py b/ipaclient/plugins/automount.py
index 8405f9f4fe283d9c068d51e10717fb1396fa44bf..86b61b2cc4172130e9ae66dd9f195b284425533b 100644
--- a/ipaclient/plugins/automount.py
+++ b/ipaclient/plugins/automount.py
@@ -27,6 +27,7 @@ from ipalib import api, errors
 from ipalib import Flag, Str
 from ipalib.frontend import Command, Method, Object
 from ipalib.plugable import Registry
+from ipalib.util import classproperty
 from ipalib import _
 from ipapython.dn import DN
 
@@ -52,9 +53,11 @@ class _fake_automountlocation_show(Method):
 
 @register(override=True, no_fail=True)
 class automountlocation_tofiles(MethodOverride):
-    @property
-    def NO_CLI(self):
-        return self.api.Command.automountlocation_show.NO_CLI
+    @classmethod
+    def __NO_CLI_getter(cls):
+        return api.Command.automountlocation_show.NO_CLI
+
+    NO_CLI = classproperty(__NO_CLI_getter)
 
     def output_for_cli(self, textui, result, *keys, **options):
         maps = result['result']['maps']
diff --git a/ipaclient/plugins/otptoken_yubikey.py b/ipaclient/plugins/otptoken_yubikey.py
index 5e0d994628ab997853a80d1f1118ba8ada9993d9..97281101f9c9f821af45df7ea830de94955ea13a 100644
--- a/ipaclient/plugins/otptoken_yubikey.py
+++ b/ipaclient/plugins/otptoken_yubikey.py
@@ -23,10 +23,11 @@ import six
 import usb.core
 import yubico
 
-from ipalib import _, IntEnum
+from ipalib import _, api, IntEnum
 from ipalib.errors import NotFound
 from ipalib.frontend import Command, Method, Object
 from ipalib.plugable import Registry
+from ipalib.util import classproperty
 
 if six.PY3:
     unicode = str
@@ -74,9 +75,11 @@ class otptoken_add_yubikey(Command):
     )
     has_output_params = takes_options
 
-    @property
-    def NO_CLI(self):
-        return self.api.Command.otptoken_add.NO_CLI
+    @classmethod
+    def __NO_CLI_getter(cls):
+        return api.Command.otptoken_add.NO_CLI
+
+    NO_CLI = classproperty(__NO_CLI_getter)
 
     def get_args(self):
         for arg in self.api.Command.otptoken_add.args():
diff --git a/ipaclient/remote_plugins/schema.py b/ipaclient/remote_plugins/schema.py
index 20d7cf7175373f9e2a87ceee273a9ad8694c238a..8bc70e08ca44661aae5d6f06b793e518450657da 100644
--- a/ipaclient/remote_plugins/schema.py
+++ b/ipaclient/remote_plugins/schema.py
@@ -95,9 +95,10 @@ class _SchemaPlugin(object):
     bases = None
     schema_key = None
 
-    def __init__(self, full_name):
+    def __init__(self, schema, full_name):
         self.name, _slash, self.version = full_name.partition('/')
         self.full_name = full_name
+        self._schema = schema
         self.__class = None
 
     def _create_default_from(self, api, name, keys):
@@ -218,6 +219,29 @@ class _SchemaCommandPlugin(_SchemaPlugin):
     bases = (_SchemaCommand,)
     schema_key = 'commands'
 
+    @property
+    def doc(self):
+        schema = self._schema[self.schema_key][self.full_name]
+        if 'doc' in schema:
+            return schema['doc']
+        else:
+            return None
+
+    @property
+    def summary(self):
+        halp = self._schema.get_help(self.schema_key, self.full_name)
+        return halp['summary']
+
+    @property
+    def topic(self):
+        halp = self._schema.get_help(self.schema_key, self.full_name)
+        return str(halp['topic']).partition('/')[0]
+
+    @property
+    def NO_CLI(self):
+        halp = self._schema.get_help(self.schema_key, self.full_name)
+        return halp['NO_CLI']
+
     def _create_output(self, api, schema):
         if schema.get('multivalue', False):
             type_type = (tuple, list)
@@ -554,10 +578,9 @@ def get_package(api, client):
     module = types.ModuleType(module_name)
     module.__file__ = os.path.join(package_dir, 'plugins.py')
     module.register = plugable.Registry()
-    for key, plugin_cls in (('commands', _SchemaCommandPlugin),
-                            ('classes', _SchemaObjectPlugin)):
-        for full_name in schema[key]:
-            plugin = plugin_cls(full_name)
+    for plugin_cls in (_SchemaCommandPlugin, _SchemaObjectPlugin):
+        for full_name in schema[plugin_cls.schema_key]:
+            plugin = plugin_cls(schema, full_name)
             plugin = module.register()(plugin)
     sys.modules[module_name] = module
 
diff --git a/ipalib/frontend.py b/ipalib/frontend.py
index cb00841f21bd5a3e3b4095dcd17a8816ca24400f..455b222d4d7fcbb65b43c4d8e1ffbbaf3e131d22 100644
--- a/ipalib/frontend.py
+++ b/ipalib/frontend.py
@@ -38,7 +38,7 @@ from ipalib.errors import (ZeroArgumentError, MaxArgumentError, OverlapError,
     ValidationError, ConversionError)
 from ipalib import errors, messages
 from ipalib.request import context, context_frame
-from ipalib.util import json_serialize
+from ipalib.util import classproperty, json_serialize
 
 if six.PY3:
     unicode = str
@@ -426,9 +426,11 @@ class Command(HasParam):
 
     api_version = API_VERSION
 
-    @property
-    def topic(self):
-        return type(self).__module__.rpartition('.')[2]
+    @classmethod
+    def __topic_getter(cls):
+        return cls.__module__.rpartition('.')[2]
+
+    topic = classproperty(__topic_getter)
 
     @property
     def forwarded_name(self):
diff --git a/ipalib/plugable.py b/ipalib/plugable.py
index 26fbeaa26d7986f2e184f0974ef396bd323d6bf5..073ad05d7aee5e83cae5c6e20bac8f9439505198 100644
--- a/ipalib/plugable.py
+++ b/ipalib/plugable.py
@@ -155,19 +155,22 @@ class Plugin(ReadOnly):
 
     bases = classproperty(__bases_getter)
 
-    @property
-    def doc(self):
-        return type(self).__doc__
+    @classmethod
+    def __doc_getter(cls):
+        return cls.__doc__
 
-    @property
-    def summary(self):
-        doc = self.doc
+    doc = classproperty(__doc_getter)
+
+    @classmethod
+    def __summary_getter(cls):
+        doc = cls.doc
         if not _(doc).msg:
-            cls = type(self)
             return u'<%s.%s>' % (cls.__module__, cls.__name__)
         else:
             return unicode(doc).split('\n\n', 1)[0].strip()
 
+    summary = classproperty(__summary_getter)
+
     @property
     def api(self):
         """
-- 
2.7.4

From 05bfe8c1fabdb6910332ba6335aa313d91b97915 Mon Sep 17 00:00:00 2001
From: David Kupka <dku...@redhat.com>
Date: Wed, 20 Jul 2016 13:24:03 +0200
Subject: [PATCH 6/6] help: Do not create instances to get information about
 commands and topics

https://fedorahosted.org/freeipa/ticket/6048
---
 ipalib/cli.py      | 15 ++++++++-------
 ipalib/plugable.py |  7 +++++--
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/ipalib/cli.py b/ipalib/cli.py
index 1faf8285c47d950d5fd311154b6936c8371d746c..d89a5320853ecf575c7ba710b2db2e62e1003141 100644
--- a/ipalib/cli.py
+++ b/ipalib/cli.py
@@ -727,8 +727,8 @@ class help(frontend.Local):
         self._builtins = []
 
         # build help topics
-        for c in self.api.Command():
-            if c is not self.api.Command[c.name]:
+        for c in self.api.Command:
+            if c is not self.api.Command.get_plugin(c.name):
                 continue
             if c.NO_CLI:
                 continue
@@ -793,13 +793,14 @@ class help(frontend.Local):
             self.print_commands(name, outfile)
         elif name == "commands":
             mcl = 0
-            for cmd in self.Command():
-                if cmd is not self.Command[cmd.name]:
+            for cmd_plugin in self.Command:
+                if cmd_plugin is not self.Command.get_plugin(cmd_plugin.name):
                     continue
-                if cmd.NO_CLI:
+                if cmd_plugin.NO_CLI:
                     continue
-                mcl = max(mcl, len(cmd.name))
-                writer('%s  %s' % (to_cli(cmd.name).ljust(mcl), cmd.summary))
+                mcl = max(mcl, len(cmd_plugin.name))
+                writer('%s  %s' % (to_cli(cmd_plugin.name).ljust(mcl),
+                                   cmd_plugin.summary))
         else:
             raise HelpError(topic=name)
 
diff --git a/ipalib/plugable.py b/ipalib/plugable.py
index 073ad05d7aee5e83cae5c6e20bac8f9439505198..af35f5bae27a17230267d5b10b5fdc4f784a4760 100644
--- a/ipalib/plugable.py
+++ b/ipalib/plugable.py
@@ -318,9 +318,12 @@ class APINameSpace(collections.Mapping):
         self.__enumerate()
         return iter(self.__plugins)
 
+    def get_plugin(self, key):
+        self.__enumerate()
+        return self.__plugins_by_key[key]
+
     def __getitem__(self, key):
-        self.__enumerate()
-        plugin = self.__plugins_by_key[key]
+        plugin = self.get_plugin(key)
         return self.__api._get(plugin)
 
     def __call__(self):
-- 
2.7.4

-- 
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