Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Speedup tests by caching HomeServerConfig instances #15284

Merged
merged 9 commits into from
Apr 18, 2023
Merged

Speedup tests by caching HomeServerConfig instances #15284

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
743af2e
Speedup tests by caching HomeServerConfig instances
progval Mar 18, 2023
22865fa
Help mypy figure the types out
progval Mar 19, 2023
a4027d4
Fix changelog id
progval Mar 19, 2023
64e9d5d
Re-run linters
progval Mar 19, 2023
02bcd46
Fix py <3.7 support
progval Mar 28, 2023
11f78e8
Simplify typing
progval Mar 28, 2023
dd086ad
Merge branch 'develop' into setup-speedup
progval Mar 28, 2023
4e589d3
Remove unused imports
progval Mar 28, 2023
813dc8a
Merge branch 'develop' into setup-speedup
erikjohnston Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/15284.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Speedup tests by caching HomeServerConfig instances.
76 changes: 74 additions & 2 deletions tests/unittest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import gc
import hashlib
import hmac
import json
import logging
import secrets
import time
Expand All @@ -28,12 +29,15 @@
Generic,
Iterable,
List,
Literal,
progval marked this conversation as resolved.
Show resolved Hide resolved
NoReturn,
Optional,
Tuple,
Type,
TypeVar,
Union,
cast,
overload,
)
from unittest.mock import Mock, patch

Expand All @@ -53,6 +57,7 @@
from synapse import events
from synapse.api.constants import EventTypes
from synapse.api.room_versions import KNOWN_ROOM_VERSIONS, RoomVersion
from synapse.config._base import Config, RootConfig
from synapse.config.homeserver import HomeServerConfig
from synapse.config.server import DEFAULT_ROOM_VERSION
from synapse.crypto.event_signing import add_hashes_and_signatures
Expand Down Expand Up @@ -124,6 +129,74 @@ def new(*args: P.args, **kwargs: P.kwargs) -> R:
return _around


@overload
def deepcopy_config(config: RootConfig, root: Literal[True]) -> RootConfig:
...


@overload
def deepcopy_config(config: Config, root: Literal[False]) -> Config:
...


def deepcopy_config(
config: Union[Config, RootConfig], root: bool
) -> Union[RootConfig, Config]:
new_config: Union[Config, RootConfig]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I don't fully understand the need for this function, I think what you probably want to do here is

C = TypeVar("C", Config, RootConfig)
# or if that doesn't work, C = TypeVar("C", bound=Union[Config, RootConfig])
def deepcopy_config(config: C) -> C: ...

and replace if root with if isinstance(config, RootConfig). That should help avoid the casts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like mypy can't figure it out:

_TConfig = TypeVar("_TConfig", Config, RootConfig)


def deepcopy_config(config: _TConfig) -> _TConfig:
    new_config: _TConfig

    if isinstance(config, RootConfig):
        reveal_type(config.__class__)
        new_config = config.__class__(config.config_files)
    else:
        new_config = config.__class__(config.root)

prints:

$ poetry run mypy tests/unittest.py
tests/unittest.py:138: note: Revealed type is "Type[tests.unittest.<subclass of "Config" and "RootConfig">]"
tests/unittest.py:138: note: Revealed type is "Type[synapse.config._base.RootConfig]"
tests/unittest.py:139: error: Argument 1 to <subclass of "Config" and "RootConfig"> has incompatible type "List[str]"; expected "Optional[RootConfig]"  [arg-type]
Found 1 error in 1 file (checked 1 source file)

I'll tell it to ignore the issue


if root:
typed_rootconfig = cast(RootConfig, config)
new_config = typed_rootconfig.__class__(typed_rootconfig.config_files)
else:
typed_config = cast(Config, config)
new_config = typed_config.__class__(typed_config.root)

for attr_name in config.__dict__:
if attr_name.startswith("__") or attr_name == "root":
continue
attr = getattr(config, attr_name)
if isinstance(attr, Config):
new_attr = deepcopy_config(attr, root=False)
else:
new_attr = attr

setattr(new_config, attr_name, new_attr)

return new_config


_make_homeserver_config_obj_cache: Dict[str, Union[RootConfig, Config]] = {}


def make_homeserver_config_obj(config: Dict[str, Any]) -> RootConfig:
"""Creates a :class:`HomeServerConfig` instance with the given configuration dict.

This is equivalent to::

config_obj = HomeServerConfig()
config_obj.parse_config_dict(config, "", "")

but it keeps a cache of `HomeServerConfig` instances and deepcopies them as needed,
to avoid validating the whole configuration every time.
Comment on lines +165 to +166
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow why we're doing the deepcopying. Is it to allow tests to mutate config at test-time?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, put differently: what sort of validation are we avoiding here?

Copy link
Contributor Author

@progval progval Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it to allow tests to mutate config at test-time?

Yes, many of them do.

Or, put differently: what sort of validation are we avoiding here?

jsonschema and compiling Jinja templates. Profiling interfers heavily with the runtimes there, so I cannot easily get exact timings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if its easier to return the config wrapped in a class that stores any modifications on itself, e.g.:

class WrappedConfig:
    def __init__(self, config):
        self.config = config

    def __getattr__(self, name):
        return getattr(self.config, name)

that way you avoid having to try and write a deep copy mechanism, while ensuring the underlying config object doesn't change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would only work when setting attributes directly to the root attribute. Otherwise, root.foo.bar = "abc" would add an attribute to the object root.foo, but we still need to somehow get rid of it when resetting root.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yup, you're totally right.

"""
cache_key = json.dumps(config)

if cache_key in _make_homeserver_config_obj_cache:
# Cache hit: reuse the existing instance
config_obj = _make_homeserver_config_obj_cache[cache_key]
else:
# Cache miss; create the actual instance
config_obj = HomeServerConfig()
config_obj.parse_config_dict(config, "", "")

# Add to the cache
_make_homeserver_config_obj_cache[cache_key] = config_obj

assert isinstance(config_obj, RootConfig)

return deepcopy_config(config_obj, root=True)


class TestCase(unittest.TestCase):
"""A subclass of twisted.trial's TestCase which looks for 'loglevel'
attributes on both itself and its individual test methods, to override the
Expand Down Expand Up @@ -518,8 +591,7 @@ def setup_test_homeserver(self, *args: Any, **kwargs: Any) -> HomeServer:
config = kwargs["config"]

# Parse the config from a config dict into a HomeServerConfig
config_obj = HomeServerConfig()
config_obj.parse_config_dict(config, "", "")
config_obj = make_homeserver_config_obj(config)
kwargs["config"] = config_obj

async def run_bg_updates() -> None:
Expand Down