Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subscribe to device registry changes from entities #93601

Merged
merged 4 commits into from May 31, 2023

Conversation

emontnemery
Copy link
Contributor

Proposed change

Subscribe to device registry changes from entities which don't have their own name

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@emontnemery emontnemery force-pushed the entity_listen_device_updates branch from 61b5684 to 7f99b75 Compare May 30, 2023 16:05
@emontnemery emontnemery marked this pull request as ready for review May 30, 2023 17:51
Comment on lines 332 to 334
if not self._attr_name:
return True
return False
Copy link
Member

@bdraco bdraco May 30, 2023

Choose a reason for hiding this comment

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

Suggested change
if not self._attr_name:
return True
return False
return not self._attr_name

Comment on lines 342 to 344
if not self.entity_description.name:
return True
return False
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not self.entity_description.name:
return True
return False
return not self.entity_description.name

Comment on lines 346 to 349
if not self.name:
return True

return False
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not self.name:
return True
return False
return not self.name

@bdraco
Copy link
Member

bdraco commented May 30, 2023

testing performance now

Comment on lines 351 to 359
def _name_translation_key(self) -> str | None:
"""Return translation key for entity name."""
if self.translation_key is None:
return None
assert self.platform
return (
f"component.{self.platform.platform_name}.entity.{self.platform.domain}"
f".{self.translation_key}.name"
)
Copy link
Member

Choose a reason for hiding this comment

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

Can this change? should we cache it?

@bdraco
Copy link
Member

bdraco commented May 30, 2023

startup performance looks good

testing run time now
add_to_platform_finish

the entity registry subscribes are so much higher on the list.

I cleaned that up in #93823

bdraco added a commit that referenced this pull request May 30, 2023
noticed in #93601 that
the cost of creating the function in the closure was a bit expensive
since we do it once per entity
@bdraco bdraco mentioned this pull request May 30, 2023
20 tasks
@bdraco
Copy link
Member

bdraco commented May 30, 2023

Screenshot 2023-05-30 at 2 21 16 PM

This dict comp is unexpectedly expensive in

Screenshot 2023-05-30 at 2 21 43 PM

but its not from this PR .. looks like it came with #90185

edit: 1st attempt to make it faster #93824

bdraco added a commit that referenced this pull request May 30, 2023
While reviewing #93601 it was noticed this was slow at startup
#93601 (comment)

This is a first pass attempt to improve the performance
@bdraco
Copy link
Member

bdraco commented May 30, 2023

Runtime performance looks good 👍

balloob pushed a commit that referenced this pull request May 31, 2023
* Use ReadOnlyDict for entity registry options

While reviewing #93601 it was noticed this was slow at startup
#93601 (comment)

This is a first pass attempt to improve the performance

* fix tests
balloob pushed a commit that referenced this pull request May 31, 2023
noticed in #93601 that
the cost of creating the function in the closure was a bit expensive
since we do it once per entity
@emontnemery emontnemery requested a review from bdraco May 31, 2023 06:04
Copy link
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

LGTM

@emontnemery emontnemery merged commit 59c6220 into dev May 31, 2023
52 checks passed
@emontnemery emontnemery deleted the entity_listen_device_updates branch May 31, 2023 09:01
@github-actions github-actions bot locked and limited conversation to collaborators Jun 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants