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

Slow loading of ALIAS #1512

Open
RkcCorian opened this issue Nov 3, 2021 · 14 comments
Open

Slow loading of ALIAS #1512

RkcCorian opened this issue Nov 3, 2021 · 14 comments

Comments

@RkcCorian
Copy link

Hello everyone,

I've created a couple of devices with ~ 500 ALIAS. Regardless of whether the original DPs come from userdata or adapters, with the advantage of having devices with your definition defined in one place (abstraction). I access those centrally with my scripts and visualization (JARVIS).
Now to my point... when I create devices with JARVIS, which in turn access this ALIAS, I have noticed that the access to ALIAS is noticeably slower (data are definitely loaded more slowly, sometimes ~ 3 seconds slower) compare to the original DP entries (userdata or adapter).
I recognize the effekt mainly once JARVIS is loaded freshly within the browser.

Thanks in advance in case this is a bug here and you can fix that!!!!

@foxriver76
Copy link
Collaborator

foxriver76 commented Nov 4, 2021

On a quick 10,000 getStates test:
normal getState: 7874,02 actions per second
getStateAlias: 3533,57 actions per second

So yes getState with alias is about twice as slow as normal getState.

This is mainly due to two getObject calls which are only done on aliases. Where one call is for the alias obj itself (get the id or readId + transform function), I guess this one could be easily cached without consuming too much memory.

The second call is to the linked state object to get the min/max etc to calc for read function. Here a optimization could be to only read the targetObject if we actually need to perform a conversion (so read function is configured), however some (IMO unnecessary, but now its there and we cannot remove this feature) magic is done like converting specific strings, e.g. aus to false if the alias is a boolean. Thus, the approach could be to move the second getObject into the formatAlias function and only call it if needed.

Thoughts?

@AlCalzone
Copy link
Collaborator

only call it if needed

Sounds like a good approach generally :)

My first thought also went to caching, but then one must take care to invalidate it when necessary.

@foxriver76
Copy link
Collaborator

foxriver76 commented Nov 4, 2021

Looking further into it, it seems like all stuff needed from the second getObject call is cached anyways:

this.aliases[sourceObj._id].source = {};
this.aliases[sourceObj._id].source.min = sourceObj.common.min;
this.aliases[sourceObj._id].source.max = sourceObj.common.max;
this.aliases[sourceObj._id].source.type = sourceObj.common.type;
this.aliases[sourceObj._id].source.unit = sourceObj.common.unit;
}

Edit: would only work for adapters which have the aliases subscribed

@AlCalzone
Copy link
Collaborator

Wouldn't it make sense to move the caching into the controller (or even the database wrappers)?

@foxriver76
Copy link
Collaborator

This would require subscribing all objects which are linked to aliases on an additional place, but it may be cheaper overall in most cases. However, I guess a more centralized approach will also cause other issues? @Apollon77 may has more information about potential downsides.

@AlCalzone
Copy link
Collaborator

I was thinking about caching on demand (e.g. first alias use), but I'm not sure if that is possible with the way things are set up.

@Apollon77
Copy link
Collaborator

In fact please remember that basically the "whole "alias login is wrapped in the adapter ... so moving code would just be moving code from place A to B ... basically would not change anything.
And adding that stuff to the DB client (!!) classes is also not helping that mich - beside adding more complexity there.

And yes one getObject should already be cached ... can the cache "key" be used directly or do we always need to search the "list of all keys" to find correct target? or such?

We could also cache the other obejct (and yes only load if needed for conversions)

@foxriver76
Copy link
Collaborator

After #1917 is merged I will implement some optimizations and provide benchmarks.

@Apollon77
Copy link
Collaborator

One idea could be (as @AlCalzone's idea above) to use locally stored values of relevant alias-base-states as soon as a first getState is done. So basically on first getState do the "expensive" way but also

  • subscribe the object, keep a local version up to date for later getStates to access locally
  • subscribe the base state and keep value locally up to date and use this
    If needed we could establish a new "alias redis client" to just hold those subscriptions

In fact there is a minimum timing difference to getState because in rare case the value could be outdated - but in reality this should never be an issue unless setstate/getState constellations are used and value is expected to be correct - or what do you think?

With this also "Multi state alias" could work with good performance ;-)

@foxriver76
Copy link
Collaborator

foxriver76 commented Sep 2, 2022

I don’t get the alias redis client thing. Aliases are actually nothing in the db. Fully managed on adapter level, so what do you mean here.

Ah ok I think I get it, but I actually assume that we can just cache where the alias publish happens.

@Apollon77
Copy link
Collaborator

IF we say that the alias value is "good enough" to be calculated on the "last received published values" (instead of always getting the most current value directly from DB) then we could simply subscribe all alias "base states" and to separate that from system and user redis clients we could use an own redis client.

In fact the topic is the following for an "get" for an alias value: To be 100% accurate at any timepoint you need to get the object and then get the relevant state value in order to return the alias values. This means "two sequencial DB operations" to return the value (compared to 1 normally). So caching the object at lest should help ... with a small risk of not having the most up to date mapping when the object was changed ... also caching the relevant values could impreove even more (but maybe object is already enough)

@foxriver76
Copy link
Collaborator

foxriver76 commented Sep 22, 2022

I have now started to refactor alias code a bit because a little bit messy currently. I will refactor the private getAliasState in an own method in the next commit, then I have a good overview and will check for optimizations with the current 100 % accuracy approach. Maybe we can get enough performance without the cache.

0ff7f0c

@Apollon77
Copy link
Collaborator

benchmark FTW

@foxriver76
Copy link
Collaborator

@RkcCorian if you are still using this approach, can you say something if performance has increased with controller version 5?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

4 participants