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

Cache entity properties that are never expected to change in the base class #95315

Merged
merged 1 commit into from
Sep 14, 2023

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Jun 26, 2023

  • Removed from breaking changes because PR was reverted

Breaking change

Cache entity properties that are never expected to change in the base class

  • attribution
  • device_class
  • translation_key

are now cached and only calculated once.

Proposed change

There is some risk that there are bugs in entities that may incorrectly set these values to None and than set the correct value after startup, or the value is set late because its not available at startup (or when state is first written) for some reason which would result in them being cached incorrectly. edit: update: Since we checked every integration that was overriding device_class this far less likely at this point.

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

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:

TODO:


Research:

@bdraco
Copy link
Member Author

bdraco commented Jun 26, 2023

I'm not sure this is actually going to work, but it should would be nice to calculate these once

bdraco added a commit that referenced this pull request Jun 27, 2023
@bdraco
Copy link
Member Author

bdraco commented Jun 27, 2023

This is a bit of a grind, but _async_write_ha_state keeps falling down in the profile so seems worth it

@bdraco
Copy link
Member Author

bdraco commented Jun 28, 2023

there are just too many things that change their UOM to do here.

This would need to be another PR since it will need a lot more exploration to make work

@bdraco
Copy link
Member Author

bdraco commented Jul 24, 2023

just need to adjust that last test. But its too late in the cycle to merge this so will do after beta

@bdraco
Copy link
Member Author

bdraco commented Aug 12, 2023

need to fix tests for this again.

Might be good to wait for https://docs.python.org/3.12/whatsnew/3.12.html#pep-698-override-decorator-for-static-typing as well

homeassistant/components/abode/binary_sensor.py Outdated Show resolved Hide resolved
homeassistant/components/dynalite/cover.py Outdated Show resolved Hide resolved
homeassistant/components/huawei_lte/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/hyperion/switch.py Outdated Show resolved Hide resolved
homeassistant/components/sma/sensor.py Outdated Show resolved Hide resolved
homeassistant/components/smartthings/cover.py Outdated Show resolved Hide resolved
@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

no change squash to see if the CI will try again

@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

that seems to have fixed it

@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

double checked to make sure we didn't get any changes from the squash git diff -w 2a1397e817413b9924d201fdd92e40323a9473ba..96e610e002368c9d92cf4a08d94e34adb93a4e98 only has upstream changes so all good

@@ -781,7 +790,7 @@ def __init__(
additional_info=[property_key_name] if property_key_name else None,
)

@property
@property # type: ignore[override]
Copy link
Member Author

Choose a reason for hiding this comment

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

These probably can be cached, but that is left up to the zwave_js code owners for a future PR as I didn't want to make a change here that might have side effects.

@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

thanks

will update all my production to give it a final test (running an older version of this now)

@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

Also going to run a py-spy and look at the flames to check for any unexpected side effects. Not expecting any. This PR has really come down in size and scope so I feel much more comfortable about it (also why I've been sitting on it)

@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

py-spy looks good.

Still deploying to more systems.

@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

all of primary production looks good.

def vincenty(
happened to hit in the profile.. all the math in python code 😮‍💨 Probably not called enough to matter or I would have seen it before. Just a suprise.

no bug here

@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

finishing up testing on the secondary systems ....

@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

pushed it to my box with 3 zwave js devices. all good

@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

All looks good. I'm out of ways to test this, so lets do it.

@bdraco bdraco merged commit 042776e into dev Sep 14, 2023
34 checks passed
@bdraco bdraco deleted the cached_prop_no_change branch September 14, 2023 22:48
@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

Thanks for all the 👀

@puddly
Copy link
Contributor

puddly commented Sep 14, 2023

Regarding the profiling hit: I suspect the convergence threshold can be increased significantly:

MAX_ITERATIONS = 200
CONVERGENCE_THRESHOLD = 1e-12

The threshold is for the difference in longitude for two points (on the auxiliary sphere) for successive approximations, so 1e-12 corresponds to a change of nearly a tenth of a mm, which I think may be a little excessive. We can probably bump it up to 1e-10 and still retain ~cm accuracy and save on the number of iterations. Or switch to a C library 😄.

@bdraco
Copy link
Member Author

bdraco commented Sep 14, 2023

Or switch to a C library 😄.

If I see it again in profiles with any frequency, I'll probably make a c lib.

emontnemery added a commit that referenced this pull request Sep 15, 2023
emontnemery added a commit that referenced this pull request Sep 15, 2023
…the base class" (#100422)

Revert "Cache entity properties that are never expected to change in the base class (#95315)"

This reverts commit 042776e.
@bdraco
Copy link
Member Author

bdraco commented Sep 15, 2023

Bummer, but we still ended up with a lot of value from all the PRs that moved things to be done in the constructor anyways

#95315 (comment)

@jbouwh
Copy link
Contributor

jbouwh commented Sep 15, 2023

Yes, but I suggest we start an architectural discussing proposing making some attributes frozen or cached. Overall it will be a breaking change. Erik mentioned helpers and may be some other integrations (like MQTT) are having the ability updating the device_class. Even when concluding that this is not very useful, changing the behavior makes it a breaking change which possible we should give some deprecation time, so (custom) integrations can implement overrides if needed.

@bdraco
Copy link
Member Author

bdraco commented Sep 15, 2023

The alternative solution, which has some more benefits, is to avoid writing state in the first place by not calling async_write_ha_state unless we know something has changed. I've been hacking away at that for a few months (I used to have 20,000-100,000 state writes per minute in some instances). That's down in the mid-5000s for most of my installs now. On a fresh install, it sits around 250, which is an excellent place. Ideally, we can identify and fix integrations that make heavy calls to async_write_ha_state and address them individually. It will take longer, but the net effect will be better than this PR.

@github-actions github-actions bot locked and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants