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

feat: Load converters on-demand #8471

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Nerivec
Copy link
Contributor

@Nerivec Nerivec commented Dec 12, 2024

This is intended for post-2.0.0.
Will keep all conflict-prone changes to a minimum until ready to execute all scripts to update the entire repo.

Changes

Logic before
  • Load all definitions into memory on start
    • import all src/devices/* modules
    • process all definitions (process extensions, validate)
    • create global lookup (mapping zigbeeModel to array of definitions)
    • store definitions in global object
  • Resolve definition:
    • for already joined devices on start if needed
    • on joining device if needed
    • on interviewing device if needed
    • on message from device if needed
    • on re-interview

Definition resolving

  • find matching definitions in lookup by zigbeeModel
  • find matching definition
    • if no match, and allowing to generate, generate external definition
    • if single match, return match
    • if multiple match, try identify by fingerprint, else return first match by zigbeeModel
  • cache definition in Z2M Device
Logic after
  • Load pre-built index (mapping zigbeeModel to array of [module, definition index]) into memory on start (index is JSON built at compile time)
  • Resolve definition:
    • for already joined devices on start if needed
    • on interviewing device if needed, when got modelID
    • on message from device if needed
    • on re-interview

Definition resolving

  • find matching indexes in pre-built index by zigbeeModel
  • import matched src/devices/* modules (cached)
  • get matched definitions by index
  • find matching indexed definition
    • if no match, and allowing to generate, generate external definition
    • if single match, return match
    • if multiple match, try identify by fingerprint, else return first match by zigbeeModel
  • process matching definition (process extensions, validate)
  • cache definition in Z2M Device
  • [after "resolving for all devices"] purge cached imported definitions modules (happens on start, or after an external converter changes)

Other changes

  • Definition type changed to use lambda for non-basic properties (breaking for existing external converters)
DIFF
@@ -5,14 +5,17 @@ type DefinitionBase = {
     vendor: string;
     description: string;
     whiteLabel?: WhiteLabel[];
+    generated?: true;
+    externalConverterName?: string;
+};
+
+type DefinitionConfig = {
     endpoint?: (device: Zh.Device) => {[s: string]: number};
     configure?: Configure;
     options?: Option[];
     meta?: DefinitionMeta;
     onEvent?: OnEvent;
     ota?: true | Ota.ExtraMetas;
-    generated?: true;
-    externalConverterName?: string;
 };
 
 type DefinitionFeatures = {
@@ -21,8 +24,12 @@ type DefinitionFeatures = {
     exposes: DefinitionExposes;
 };
 
-export type Definition = DefinitionMatcher & DefinitionBase & DefinitionFeatures;
+export type ResolvedDefinition = DefinitionConfig & DefinitionFeatures;
+
+export type Definition = DefinitionMatcher & DefinitionBase & ResolvedDefinition;
+
+export type ResolvedDefinitionWithExtend = DefinitionConfig & (({extend: ModernExtend[]} & Partial<DefinitionFeatures>) | DefinitionFeatures);
+
+export type DefinitionWithExtend = DefinitionMatcher & DefinitionBase & ResolvedDefinitionWithExtend;
 
-export type DefinitionWithExtend = DefinitionMatcher &
-    DefinitionBase &
-    (({extend: ModernExtend[]} & Partial<DefinitionFeatures>) | DefinitionFeatures);
+export type IndexedDefinition = DefinitionMatcher & DefinitionBase & {resolve: () => ResolvedDefinitionWithExtend};
  • Export src/devices/* using named exports
  • Export some more APIs from ZHC
  • Rename some ZHC APIs to match new logic
    • Lookup now only used for external converters (skipped entirely if no external converter present)
  • Rename some symbols in generateDefinition.ts to avoid clash with globals.

TODO:

  • New tests
  • Execute scripts to update repo to new logic
  • Fix places where the scripts aren't able to "do everything"
  • [external] Review usage of resolveDefinition in Z2M (optimize as needed)
  • [external] Identify locations where cache purging would be beneficial (other than immediately after start).
  • [post] Remove no longer useful scripts

Benchmarks

This being a deep change in the architecture, some in-depth testing was required. Since this is mostly about memory, tests mostly focus on memory...
Tested using Zigbee2MQTT 2.0.0 (for both "Before" & "After").
I tried to minimize the impact of other systems while testing, but allow for some margin of error still...

VSCode Profiler

With brand new network (already established on coordinator, no device).
Cold start.

Item Before After Diff
CPU time total (non-idle) 1948 ms 1242 ms 64%

Heap snapshot once settled after start (limited to items above 1 MB in "Before"):

Item Before After Diff
global / 35.6 MB 13.6 MB 38% 🚀
Object 33.7 MB 21.9 MB 64%
(compiled code) 25.6 MB 18.8 MB 73%
(string) 18.1 MB 11.5 MB 63%
(closure) 17.1 MB 13.7 MB 80%
Array 16.1 MB 2.44 MB 15% 🚀
(array) 11.5 MB 5.36 MB 46% 🚀
system / Context 8.22 MB 4.73 MB 57%
Buffer 6.42 MB 6.43 MB -
Numeric 4.76 MB 0 - 🚀
(unknown) 3.35 MB 2.72 MB 81%
Light 2.73 MB 0 - 🚀
(concatenated string) 2.57 MB 903 KB 34% 🚀
Map 1.69 MB 815 KB 47% 🚀
Module 1.65 MB 1.31 MB 79%
(system) 1.54 MB 1.23 MB 79%
Enum 1.42 MB 1.5 KB - 🚀
--- --- --- ---
TOTAL 192 MB 105 MB 54%
Clinic.js

With brand new network (already established on coordinator).
Cold start and pairing of 1 device.
Red=Before
Blue=After

mem-before
mem-after

Even Loop use

even-loop-use-before

even-loop-use-after

Event Loop delay

event-loop-delay-before
event-loop-delay-after

Overall, I'd say we can expect memory usage to be around 60% of what it was before. This is reflected throughout the application's runtime, and should remain pretty stable, with small spikes only expected around the addition of new devices (i.e. need to load some converters).

@Nerivec
Copy link
Contributor Author

Nerivec commented Dec 12, 2024

@sjorge @kirovilya if you have some time, and can review the changes, mostly just the core logic changes in src/index.ts, the rest is utility. Would be good to have some feedback, tweak the approach as necessary.

src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
Copy link
Contributor

@sjorge sjorge left a comment

Choose a reason for hiding this comment

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

I only looked at index.ts, this code has always been a bit confusing at times IMHO.

I think this look good, I left a few nits feel free to ignore them but It could slightly improve readability I think.

Two questions/ideas did pop into my head while reading over this...

  1. Can definitions loaded from external providers somehow be marked on the Device, the frontend for example could then maybe somehow display that. This could be helpful when people forgot to remove one if changes get merged or just to know which devices are currently dependent on external converters.
  2. The logger change reminded again we have some quirks for certain vendors/devices. I wonder if we can somehow get these out of index.ts, perhaps there could be some sort of hook that can be used instead so we can keep these in their respective vendor files.

@Koenkk
Copy link
Owner

Koenkk commented Dec 13, 2024

@kirovilya could you also take a look at this?

@Nerivec Nerivec force-pushed the converters-load-on-demand branch from e76a1c9 to 7b11ecd Compare December 13, 2024 21:07
@Nerivec
Copy link
Contributor Author

Nerivec commented Dec 14, 2024

@sjorge Thanks for the feedback 👍

  1. The new external converters in 2.0.0 already have a prop that identifies them. It would need to be added to the published payload, then I guess it's just a matter of flipping a switch somewhere in frontend 😉
  2. Probably good indeed. Though not in this PR to keep it only about the base logic change (it will already be huge once scripts are applied).

@Nerivec Nerivec force-pushed the converters-load-on-demand branch from 543274d to 5eb45c2 Compare December 21, 2024 23:01
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.

3 participants