From 6c6a2b4a8178578f3549241ed7199eebdbba44c3 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Wed, 10 Jan 2024 13:26:05 +0000 Subject: [PATCH 1/4] graphql-ws: optimise the "resolve" routine * Partially addresses https://github.com/cylc/cylc-uiserver/issues/547 * The resolve routine is implmented recursively in the graphql-ws library. * Because the function is async this results in a large number of async tasks being created when the library is used with large, deeply nested schema. * Async tasks have an overhead, above that of regular function calls. * For the example in https://github.com/cylc/cylc-uiserver/issues/547 this resulted in over 10 seconds of overheads. --- changes.d/548.feat.md | 1 + cylc/uiserver/websockets/resolve.py | 66 +++++++++++++++++++++++++++++ cylc/uiserver/websockets/tornado.py | 4 +- 3 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 changes.d/548.feat.md create mode 100644 cylc/uiserver/websockets/resolve.py diff --git a/changes.d/548.feat.md b/changes.d/548.feat.md new file mode 100644 index 00000000..e1803848 --- /dev/null +++ b/changes.d/548.feat.md @@ -0,0 +1 @@ +Improve the performance of the GraphQL server. diff --git a/cylc/uiserver/websockets/resolve.py b/cylc/uiserver/websockets/resolve.py new file mode 100644 index 00000000..e53ed09d --- /dev/null +++ b/cylc/uiserver/websockets/resolve.py @@ -0,0 +1,66 @@ +# MIT License +# +# Copyright (c) 2017, Syrus Akbary +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. + +""" +This file contains an implementation of "resolve" derived from the one +found in the graphql-ws library with the above license. + +This is temporary code until the change makes its way upstream. +""" + +# NOTE: transient dependency from graphql-ws not +from promise import Promise + +from graphql_ws.base_async import is_awaitable + + +async def resolve( + data, + _container=None, + _key=None, +): + """ + Recursively wait on any awaitable children of a data element and resolve + any Promises. + """ + stack = [(data, _container, _key)] + + while stack: + _data, _container, _key = stack.pop() + + if is_awaitable(_data): + _data = await _data + if isinstance(_data, Promise): + _data = _data.value + if _container is not None: + _container[_key] = _data + if isinstance(_data, dict): + items = _data.items() + elif isinstance(_data, list): + items = enumerate(_data) + else: + items = None + if items is not None: + stack.extend([ + (child, _data, key) + for key, child in items + ]) diff --git a/cylc/uiserver/websockets/tornado.py b/cylc/uiserver/websockets/tornado.py index fcabac34..1bb1cbb5 100644 --- a/cylc/uiserver/websockets/tornado.py +++ b/cylc/uiserver/websockets/tornado.py @@ -15,7 +15,6 @@ from graphql.execution.middleware import MiddlewareManager from graphql_ws.base import ConnectionClosedException from graphql_ws.base_async import ( - resolve, BaseAsyncConnectionContext, BaseAsyncSubscriptionServer ) @@ -29,6 +28,9 @@ from typing import Union, Awaitable, Any, List, Tuple, Dict, Optional from cylc.uiserver.authorise import AuthorizationMiddleware +from cylc.uiserver.websockets.resolve import resolve + + setup_observable_extension() NO_MSG_DELAY = 1.0 From 1311fbb2267c66d9adf818e240c75c4671eb0671 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Wed, 10 Jan 2024 15:17:49 +0000 Subject: [PATCH 2/4] websockets: avoid duplicate resolve call * Due to inheritance, we were calling `resolve(execution_result.data)` twice. --- cylc/uiserver/websockets/tornado.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/cylc/uiserver/websockets/tornado.py b/cylc/uiserver/websockets/tornado.py index 1bb1cbb5..aaf2e285 100644 --- a/cylc/uiserver/websockets/tornado.py +++ b/cylc/uiserver/websockets/tornado.py @@ -13,7 +13,7 @@ from asyncio.queues import QueueEmpty from tornado.websocket import WebSocketClosedError from graphql.execution.middleware import MiddlewareManager -from graphql_ws.base import ConnectionClosedException +from graphql_ws.base import ConnectionClosedException, BaseSubscriptionServer from graphql_ws.base_async import ( BaseAsyncConnectionContext, BaseAsyncSubscriptionServer @@ -165,4 +165,14 @@ async def send_execution_result(self, connection_context, op_id, execution_resul await resolve(execution_result.data) request_context = connection_context.request_context await request_context['resolvers'].flow_delta_processed(request_context, op_id) - await super().send_execution_result(connection_context, op_id, execution_result) + else: + await resolve(execution_result.data) + + # NOTE: skip TornadoSubscriptionServer.send_execution_result because it + # calls "resolve" then invokes BaseSubscriptionServer.send_execution_result + await BaseSubscriptionServer.send_execution_result( + self, + connection_context, + op_id, + execution_result, + ) From 2b77badce6e19e8b25f6ab34a7809666b4b0e47f Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Mon, 29 Jan 2024 13:57:55 +0000 Subject: [PATCH 3/4] Apply suggestions from code review --- cylc/uiserver/websockets/resolve.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cylc/uiserver/websockets/resolve.py b/cylc/uiserver/websockets/resolve.py index e53ed09d..4c298669 100644 --- a/cylc/uiserver/websockets/resolve.py +++ b/cylc/uiserver/websockets/resolve.py @@ -27,7 +27,8 @@ This is temporary code until the change makes its way upstream. """ -# NOTE: transient dependency from graphql-ws not +# NOTE: transient dependency from graphql-ws purposefully not +# reflected in cylc-uiserver dependencies from promise import Promise from graphql_ws.base_async import is_awaitable @@ -39,7 +40,7 @@ async def resolve( _key=None, ): """ - Recursively wait on any awaitable children of a data element and resolve + Wait on any awaitable children of a data element and resolve any Promises. """ stack = [(data, _container, _key)] From 2ba41a467941b9a0a9873960483035fbf57bbcf6 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 30 Jan 2024 14:28:26 +0000 Subject: [PATCH 4/4] Update cylc/uiserver/websockets/resolve.py --- cylc/uiserver/websockets/resolve.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cylc/uiserver/websockets/resolve.py b/cylc/uiserver/websockets/resolve.py index 4c298669..71bb8f23 100644 --- a/cylc/uiserver/websockets/resolve.py +++ b/cylc/uiserver/websockets/resolve.py @@ -28,7 +28,7 @@ """ # NOTE: transient dependency from graphql-ws purposefully not -# reflected in cylc-uiserver dependencies +# reflected in cylc-uiserver dependencies from promise import Promise from graphql_ws.base_async import is_awaitable