Make unit converter use a factory to avoid looking up the ratios each conversion #93706

bdraco · 2023-05-28T14:21:01Z

Proposed change

With statistics we are doing the same conversion for each data point, and we currently have to do lookups every time to do the convert. Since its the same to/from unit over and over we can use a factory pattern to avoid all but the first set of lookups (unless its already in the LRU).

I added LRUs to avoid building the converters over and over since we also use this in sensor to convert units. It most cases we end up with ~20-30 tiny cached converter functions which uses less memory than most of our python modules.

After this change, all the unit conversion no longer show in all the statistics profiling because they are too small to be included. The flame on 1939 no longer has a sub-flame for the conversion:

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:

Documentation added/updated for www.home-assistant.io

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:

I have reviewed two other open pull requests in this repository.

With statistics we are doing the same conversion over and over and we have do do lookups every time to do the convert. Since its the same to/from unit over and over we can use a factory pattern to avoid all the lookups

home-assistant · 2023-05-28T14:21:07Z

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (recorder) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of recorder can trigger bot actions by commenting:

@home-assistant close Closes the pull request.
@home-assistant rename Awesome new title Renames the pull request.
@home-assistant reopen Reopen the pull request.
@home-assistant unassign recorder Removes the current integration label and assignees on the pull request, add the integration domain after the command.

bdraco · 2023-05-28T15:39:45Z

LRU stats after 1 minute

2023-05-28 15:55:08.674 CRITICAL (SyncWorker_16) [homeassistant.components.profiler] Cache stats for lru_cache <function BaseUnitConverter.converter_factory at 0x7fc6cf1b7e20> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=105, misses=7, maxsize=128, currsize=7)
2023-05-28 15:55:08.675 CRITICAL (SyncWorker_16) [homeassistant.components.profiler] Cache stats for lru_cache <function BaseUnitConverter.converter_factory_allow_none at 0x7fc6cf1b7f60> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=3, misses=1, maxsize=128, currsize=1)
2023-05-28 15:55:08.675 CRITICAL (SyncWorker_16) [homeassistant.components.profiler] Cache stats for lru_cache <function BaseUnitConverter.get_unit_ratio at 0x7fc6cef940e0> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=81, misses=8, maxsize=128, currsize=8)
2023-05-28 15:55:08.676 CRITICAL (SyncWorker_16) [homeassistant.components.profiler] Cache stats for lru_cache <function TemperatureConverter.converter_factory at 0x7fc6cef945e0> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=364, misses=5, maxsize=8, currsize=5)
2023-05-28 15:55:08.676 CRITICAL (SyncWorker_16) [homeassistant.components.profiler] Cache stats for lru_cache <function TemperatureConverter.converter_factory_allow_none at 0x7fc6cef94720> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=0, misses=0, maxsize=8, currsize=0)

homeassistant/util/unit_conversion.py

bdraco · 2023-05-28T15:57:26Z

LRU stats from another production system after 2 minutes

2023-05-28 15:56:41.949 CRITICAL (SyncWorker_6) [homeassistant.components.profiler] Cache stats for lru_cache <function BaseUnitConverter.converter_factory at 0x7f67fa4fbce0> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=807, misses=22, maxsize=128, currsize=22)
2023-05-28 15:56:41.950 CRITICAL (SyncWorker_6) [homeassistant.components.profiler] Cache stats for lru_cache <function BaseUnitConverter.converter_factory_allow_none at 0x7f67fa4fbe20> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=1, misses=1, maxsize=128, currsize=1)
2023-05-28 15:56:41.950 CRITICAL (SyncWorker_6) [homeassistant.components.profiler] Cache stats for lru_cache <function BaseUnitConverter.get_unit_ratio at 0x7f67fa4fbf60> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=79, misses=24, maxsize=128, currsize=24)
2023-05-28 15:56:41.951 CRITICAL (SyncWorker_6) [homeassistant.components.profiler] Cache stats for lru_cache <function TemperatureConverter.converter_factory at 0x7f67fa2dc4a0> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=621, misses=4, maxsize=8, currsize=4)
2023-05-28 15:56:41.951 CRITICAL (SyncWorker_6) [homeassistant.components.profiler] Cache stats for lru_cache <function TemperatureConverter.converter_factory_allow_none at 0x7f67fa2dc5e0> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=335, misses=1, maxsize=8, currsize=1)

bdraco · 2023-05-28T16:13:41Z

Converter implementation for use outside of stats doesn't show any measurable performance regression

bdraco · 2023-05-28T17:02:34Z

Tests for gdacs are failing because they aren't using pytest.approx for the float conversion

We could do two operations to reduce the floating point conversion loss but that might be a bit overkill to preserve decimal places beyond the actual precision

bdraco · 2023-05-28T21:23:26Z

refactored to retain the exact same floating point conversion behavior

bdraco · 2023-05-28T22:00:45Z

still effectively the same after refactor to make it more dry and avoid the double conversion float loss

bdraco · 2023-05-28T22:01:10Z

stats profile looks great as well. also gone from there.

bdraco · 2023-05-28T22:13:03Z

lru stats look excellent

2023-05-28 22:12:32.744 CRITICAL (SyncWorker_5) [homeassistant.components.profiler] Cache stats for lru_cache <function BaseUnitConverter.converter_factory at 0x7f96c0287d80> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=3946, misses=27, maxsize=128, currsize=27)
2023-05-28 22:12:32.744 CRITICAL (SyncWorker_5) [homeassistant.components.profiler] Cache stats for lru_cache <function BaseUnitConverter.converter_factory_allow_none at 0x7f96c0287f60> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=17, misses=2, maxsize=128, currsize=2)
2023-05-28 22:12:32.745 CRITICAL (SyncWorker_5) [homeassistant.components.profiler] Cache stats for lru_cache <function BaseUnitConverter.get_unit_ratio at 0x7f96c00640e0> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=1287, misses=2, maxsize=128, currsize=2)
2023-05-28 22:12:32.745 CRITICAL (SyncWorker_5) [homeassistant.components.profiler] Cache stats for lru_cache <function TemperatureConverter.converter_factory at 0x7f96c00645e0> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=3006, misses=4, maxsize=8, currsize=4)
2023-05-28 22:12:32.746 CRITICAL (SyncWorker_5) [homeassistant.components.profiler] Cache stats for lru_cache <function TemperatureConverter.converter_factory_allow_none at 0x7f96c0064720> at /usr/src/homeassistant/homeassistant/util/unit_conversion.py: CacheInfo(hits=55, misses=1, maxsize=8, currsize=1)

bdraco · 2023-05-29T01:14:49Z

Test failure is unrelated

frenck

LGTM, thanks @bdraco 👍

../Frenck

bdraco · 2023-05-29T18:53:22Z

Thanks. This one will make quite a difference for some use cases where the display and stored units differ

bdraco added 5 commits May 28, 2023 09:04

Make a unit converter factory for statistics

27b9e9d

With statistics we are doing the same conversion over and over and we have do do lookups every time to do the convert. Since its the same to/from unit over and over we can use a factory pattern to avoid all the lookups

move more code

fbfb63b

move more code

be76b9e

move more code

9d0404e

reduce

6fbeb81

home-assistant bot added cla-signed core integration: recorder Quality Scale: internal labels May 28, 2023

bdraco added 8 commits May 28, 2023 09:26

fix

ea2ddfd

coverage

e0c8704

fix

ab5f6e5

fix

d13f47d

fix

a756be6

same unit test

03193b6

dry

e2ceea8

simplify

42522cc

bdraco commented May 28, 2023

View reviewed changes

homeassistant/util/unit_conversion.py Show resolved Hide resolved

bdraco added 2 commits May 28, 2023 10:51

dry

ae0f99c

dry

1c9b799

bdraco added 2 commits May 28, 2023 11:18

one was missing approx

2c66457

preserve compat

2d23838

bdraco changed the title ~~Make a unit converter use a factory~~ Make unit converter use a factory to avoid looking up the ratios each conversion May 28, 2023

bdraco added 2 commits May 28, 2023 16:18

reduce loss

d92d509

reduce loss

faad2d6

bdraco added 3 commits May 28, 2023 16:20

reduce loss

efdfd22

reduce loss

8fac81d

reduce loss

4669b75

bdraco added 7 commits May 28, 2023 16:26

remove one lru

9c97ca7

remove one lru

392c1be

adjust

c6a928c

adjust

992f94b

dry it up

dadeb22

adjust

9df98f4

reduce

1b387e8

bdraco marked this pull request as ready for review May 28, 2023 22:13

bdraco requested review from a team and epenet as code owners May 28, 2023 22:13

bdraco added 3 commits May 28, 2023 18:56

cleanup cover

e6db2ed

add missing coverage

1d4bf6b

Merge branch 'dev' into convert_factory

553a55d

bdraco added 2 commits May 28, 2023 21:59

Merge branch 'dev' into convert_factory

cf4b538

Merge branch 'dev' into convert_factory

6aa84be

frenck approved these changes May 29, 2023

View reviewed changes

frenck merged commit 2f1f32f into dev May 29, 2023
52 checks passed

frenck deleted the convert_factory branch May 29, 2023 18:50

github-actions bot locked and limited conversation to collaborators May 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make unit converter use a factory to avoid looking up the ratios each conversion #93706

Make unit converter use a factory to avoid looking up the ratios each conversion #93706

bdraco commented May 28, 2023 •

edited

home-assistant bot commented May 28, 2023

bdraco commented May 28, 2023 •

edited

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 29, 2023

frenck left a comment

bdraco commented May 29, 2023

Make unit converter use a factory to avoid looking up the ratios each conversion #93706

Make unit converter use a factory to avoid looking up the ratios each conversion #93706

Conversation

bdraco commented May 28, 2023 • edited

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented May 28, 2023

bdraco commented May 28, 2023 • edited

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 28, 2023

bdraco commented May 29, 2023

frenck left a comment

Choose a reason for hiding this comment

bdraco commented May 29, 2023

bdraco commented May 28, 2023 •

edited

bdraco commented May 28, 2023 •

edited