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

Discovered devices improvements #1047

Merged
merged 4 commits into from
Oct 3, 2022
Merged

Discovered devices improvements #1047

merged 4 commits into from
Oct 3, 2022

Conversation

dlech
Copy link
Collaborator

@dlech dlech commented Sep 30, 2022

  • Performance improvements for keeping track of discovered devices.
  • New property to get advertisement data along with discovered devices.
  • Adds rssi attribute to AdvertisemntData

Additional details in the commit messages.

dlech and others added 4 commits September 30, 2022 14:45
This changes all scanner backends so that they only create a BLEDevice
object once per scan. For example, in several backends, a new BLEDevice
was created for each discovered device each time the discovered_devices
property was called, which could be significant if many (~100) devices
were discovered.

This also caches the last advertisement data along with the BLEDevice
which is expected to be used as an alternative to BLEDevice.metadata
in the future.
This adds an `rssi` attribute to the AdvertisementData type.

This is done so we can eventually deprecate the `rssi` on the BLEDevice
object (#1025).

While we are touching this, also change AdvertisementData to a named
tuple so we don't have to initialize all of the values ourselves.
This reduces code duplication and helps ensure that all backends work
the same.
This adds a new discovered_devices_and_advertisement_data property
to BleakScanner that can be used to get the last advertisement data in
addition to the BLEDevice of the devices discovered during the scan
session.

A new argument is added to the BleakScanner.discover() class method to
allow this method to return the new property as well.
@dlech
Copy link
Collaborator Author

dlech commented Sep 30, 2022

Hi @bdraco, could you please take a look?

@bdraco
Copy link
Contributor

bdraco commented Sep 30, 2022

Should be able to take a look a bit later today.

👍

tx_power=tx_power,
rssi=props.get("RSSI", -127),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Returns:
A Bluetooth address as a string.
"""
return ":".join(device_path[-17:].split("_"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be faster to grab it and use .replace

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, this isn't used in any critical paths.


from ..exc import BleakError
from .device import BLEDevice


class AdvertisementData:
class AdvertisementData(NamedTuple):
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it would be faster as a dataclass?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope. Named tuple is faster

Copy link
Contributor

@bdraco bdraco Sep 30, 2022

Choose a reason for hiding this comment

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

Found this while digging around https://medium.com/@jacktator/dataclass-vs-namedtuple-vs-object-for-performance-optimization-in-python-691e234253b9

But will get better data when I run in through the profiler since we will get actual use case data vs theory

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Part of the reason for using NamedTuple was for the immutability, but if there is a way to speed it up, we can consider alternatives.

Copy link
Contributor

@bdraco bdraco Sep 30, 2022

Choose a reason for hiding this comment

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

Makes sense. I don't think it's going to be an issue. Will know for sure once I do the profiles on the production systems. Identifying this as an area to target for additional profiling and testing.

Copy link

Choose a reason for hiding this comment

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

Don't forget to add frozen=True to dataclasses when profiling 👍

@bdraco
Copy link
Contributor

bdraco commented Sep 30, 2022

This looks awesome. Cleans things up quite a bit 👍

I added some first impression thoughts above. Some need to be validated with testing

I'll do testing and profiling when other stuff calms down later tonight and I don't have as many distractions

@@ -127,6 +160,45 @@ def detection_callback(s, d):

self._callback = detection_callback

def create_or_update_device(
Copy link
Contributor

Choose a reason for hiding this comment

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

For profiling this later, assume this is hot path


from ..exc import BleakError
from .device import BLEDevice


class AdvertisementData:
class AdvertisementData(NamedTuple):
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it can be avoided but
Sep 30 23:52:42 homeassistant homeassistant[487]: TypeError: AdvertisementData.__new__() missing 1 required positional argument: 'rssi'

@bdraco
Copy link
Contributor

bdraco commented Oct 1, 2022

It looks like the parse_msg path is slightly more expensive.

New
parse_msg

Original
parse_msg_orig

@bdraco
Copy link
Contributor

bdraco commented Oct 1, 2022

I think this could be made more performant by storing the _advertisement_callbacks by adapter path and doing a dict lookup to find it by adapter path so it can avoid enumerating them and calling startswith for every callback.

Maybe slice device_path to get the adapter path and lookup self._advertisement_callbacks_by_adapter.get(device_path[SLICED])?

        for (callback, adapter_path) in self._advertisement_callbacks:
            # filter messages from other adapters
            if not device_path.startswith(adapter_path):
                continue

            # TODO: this should be deep copy, not shallow
            callback(device_path, device.copy())

@dlech
Copy link
Collaborator Author

dlech commented Oct 1, 2022

It looks like the parse_msg path is slightly more expensive.

How much of this difference could just be noise/variance between runs? For example _async_detection_callback increased 0.2% but that isn't even part of Bleak. And unpack_variants increased as well (0.04%) and it wasn't changed.

The part that was changed - create_or_update_device (0.24%) vs the previous (BleakDevice) __init__ (0.40%) seems to be an improvement though.

@bdraco
Copy link
Contributor

bdraco commented Oct 1, 2022

I'll do a few more. Its not an exact science since we can't control how many advertisements come in.

@bdraco
Copy link
Contributor

bdraco commented Oct 1, 2022

2nd run
run2

@bdraco
Copy link
Contributor

bdraco commented Oct 1, 2022

3rd run
run3

@bdraco
Copy link
Contributor

bdraco commented Oct 1, 2022

4th run
run4

@bdraco
Copy link
Contributor

bdraco commented Oct 1, 2022

Only thing that still stands out as easy to optimize is startswith since its call 80k/min

@dlech
Copy link
Collaborator Author

dlech commented Oct 1, 2022

Comparing relative values instead of absolute values, it looks like "before", _run_advertisement_callbacks was > 10x unpack_variants while "after", it is consistently < 10x unpack_variants, so overall improvement, no? (assuming the one "before" measurement is repeatable with similar results)

@bdraco
Copy link
Contributor

bdraco commented Oct 1, 2022

All of these are 1 min snapshots with % is run time on a single core of _parse_msg and descendants. _parse_msg is the 2nd most expensive branch running (unmarshalling is the first most expensive but only by ~25% more now -- it was taking almost all the cpu run time before the switch to dbus-fast and maxxing out the core -- that one is hard to find any more wins).

@bdraco
Copy link
Contributor

bdraco commented Oct 1, 2022

I'll revert and do another one to compare. The system is mostly idle otherwise but its a real world performance test so everything is affected by everything else

@bdraco
Copy link
Contributor

bdraco commented Oct 1, 2022

Original run 2

original_run2

@bdraco
Copy link
Contributor

bdraco commented Oct 2, 2022

Some good news about the hot unmarshalling path, it’s now much faster as it has an optional (auto fallback to pure python if not available) which makes this path now the hottest one in production.

I’ll see if I can get unpack variants exposed in dbus-fast since it was already added in a previous PR and add an optional (auto fallback to pure python) cython backend to shave that off a bit. #1055

@dlech dlech merged commit 0e439ee into develop Oct 3, 2022
@dlech dlech deleted the discovered-devices branch October 3, 2022 15:36
dlech added a commit that referenced this pull request Oct 4, 2022
@dlech dlech mentioned this pull request Oct 13, 2022
dlech added a commit that referenced this pull request Oct 19, 2022
In #1047, the call to getRssi() was broken by using the wrong object.

This fixes the mistake.

Fixes #1085.
dlech added a commit that referenced this pull request Oct 25, 2022
In #1047, the call to getRssi() was broken by using the wrong object.

This fixes the mistake.

Fixes #1085.
pmessan pushed a commit to workaroundgmbh/bleak that referenced this pull request Nov 28, 2022
In hbldh#1047, the call to getRssi() was broken by using the wrong object.

This fixes the mistake.

Fixes hbldh#1085.
Comment on lines +84 to +90
seen_devices: Dict[str, Tuple[BLEDevice, AdvertisementData]]
"""
Map of device identifier to BLEDevice and most recent advertisement data.

This map must be cleared when scanning starts.
"""

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the point of this class variable in addition to the instance variable defined below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since nothing is assigned to the name here, this is just a type hint rather than a class variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, my apologies 🤦 I suppose moved here to not clutter the __init__() method with docstring?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants