Speedup tests by caching HomeServerConfig instances

[progval]

These two lines:

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

are called many times with the exact same value for config.

As the test suite is CPU-bound and non-negligeably time is spent in parse_config_dict, this saves ~5% on the overall runtime of the Trial test suite (tested with both -j2 and -j12 on a 12t CPU).

This is sadly rather limited, as the cache cannot be shared between processes (it contains at least jinja2.Template and RLock objects which aren't pickleable), and Trial tends to run close tests in different processes.

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[progval]

Signed-off-by: Valentin Lorentz <progval plus matrixsignoff at progval dot net>

[DMRobertson]

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

[DMRobertson]

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

[progval]

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.

[erikjohnston]

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?

[progval]

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.

[erikjohnston]

Ah yup, you're totally right.

[DMRobertson]

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.

[progval]

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

[progval]

On second thought, it's a little silly to add this complexity for only 5%. I started working on this because with profiling on, this was a 50% improvement, but 5% doesn't seem worth it.

[erikjohnston]

@progval I'm having a quick look at speeding up of tests, and it looks like (with a bunch of other changes), this PR would save ~20% test time when running with an in-memory SQLite DB