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

Improve startup times in Complement test runs against workers, particularly in CPU-constrained environments. #13127

Merged
merged 15 commits into from
Jun 30, 2022

Conversation

reivilibre
Copy link
Contributor

Fixes #13051.

Should be more or less a step-by-step review, but it may be clear enough from the overall diff also, I don't know.

@reivilibre reivilibre marked this pull request as ready for review June 29, 2022 09:04
@reivilibre reivilibre requested a review from a team as a code owner June 29, 2022 09:04
@richvdh richvdh self-assigned this Jun 29, 2022
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

looks pretty good to me! a couple of nits.

synapse/app/_base.py Show resolved Hide resolved
docker/conf-workers/synapse.supervisord.conf.j2 Outdated Show resolved Hide resolved
Comment on lines 86 to 87
if attr_name == "___install":
return self.___install
Copy link
Member

Choose a reason for hiding this comment

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

I think this is redundant? __getattr__ is only called when a normal attribute lookup fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the same, but I get

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 30, in __getattr__
AttributeError: 'NoneType' object has no attribute '___install'

if I don't have that there. :/

Copy link
Member

Choose a reason for hiding this comment

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

'NoneType' object has no attribute '___install'

.... that implies you're calling ___install on None ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, interesting, this isn't a problem if I only have one underscore as a prefix for the name. The docs don't seem to mention why this is, but oh well..

Copy link
Contributor Author

@reivilibre reivilibre Jun 29, 2022

Choose a reason for hiding this comment

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

'NoneType' object has no attribute '___install'

.... that implies you're calling ___install on None ?

Yes, because that's how __getattr__ is implemented. The underlying, not-yet-installed reactor (None) doesn't have an ___install method.
According to what you + docs are saying, it shouldn't call __getattr__ because the class itself has a ___install

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you want to try at home:

from typing import Any

class A:
    def __init__(self) -> None:
        self.___reactor_target: Any = None

    def ___install(self, new_reactor: Any) -> None:
        self.___reactor_target = new_reactor

    def __getattr__(self, attr_name: str) -> Any:
        return getattr(self.___reactor_target, attr_name)


class B:
    def __init__(self) -> None:
        self.___reactor_target: Any = None

    def _install(self, new_reactor: Any) -> None:
        self.___reactor_target = new_reactor

    def __getattr__(self, attr_name: str) -> Any:
        return getattr(self.___reactor_target, attr_name)

then:

a = A()
a.___install(42) # <--- fails
print(a.conjugate())

compare to

b = B()
b._install(42)
print(b.conjugate())

synapse/app/_complement_fork_starter.py Outdated Show resolved Hide resolved
synapse/app/_complement_fork_starter.py Outdated Show resolved Hide resolved
synapse/app/_complement_fork_starter.py Outdated Show resolved Hide resolved
synapse/app/_complement_fork_starter.py Outdated Show resolved Hide resolved
@richvdh richvdh removed their assignment Jun 29, 2022
@reivilibre reivilibre requested a review from richvdh June 29, 2022 15:13
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm otherwise

synapse/app/_base.py Outdated Show resolved Hide resolved
synapse/app/complement_fork_starter.py Outdated Show resolved Hide resolved
synapse/app/complement_fork_starter.py Outdated Show resolved Hide resolved
@reivilibre reivilibre enabled auto-merge (squash) June 30, 2022 11:14
@reivilibre reivilibre merged commit 9667bad into develop Jun 30, 2022
@reivilibre reivilibre deleted the rei/comworkfork3 branch June 30, 2022 11:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Launch Synapse processes using fork() in Complement with workers CI
2 participants