EFeru / DbcParser

.NET CAN bus dbc file parser
MIT License
76 stars 28 forks source link

IsMultiplexed run time #37

Open taylorjdlee opened 1 year ago

taylorjdlee commented 1 year ago

https://github.com/EFeru/DbcParser/blob/59b4eb633e4fdf7f02858b2e27484be89287a570/DbcParserLib/Extensions.cs#L64

Due to using the any() method IsMultiplexed() can take sometime to run if there are a lot of signals present in the message. It would be great if we could have a simple boolean in the message that means we don't have to iterate across every signal to improve the runtime of this function.

But I also think it's down to the runtime of MultiplexingInfo https://github.com/EFeru/DbcParser/blob/59b4eb633e4fdf7f02858b2e27484be89287a570/DbcParserLib/Extensions.cs#L45

Pretty much I want IsMultiplexed() to be efficient as possible due to using this library to decode data at runtime. Maybe the message class can even have a boolean value stored that does this? Pretty much when we initialise a message when loading from the .dbc we run the IsMultiplexed() function once to generate this boolean value

Adhara3 commented 12 months ago

Hi @taylorjdlee ,

To be honest, this

Pretty much I want IsMultiplexed() to be efficient as possible due to using this library to decode data at runtime.

sounds weird. I do develop runtime systems and I want to share my experience on this. All static (i.e. that do not change during the "acquisition") informations should be used to prepare the system so that the minimum possible number of operations should be executed at runtime. So, if signal is multiplexed and will require further processing then you should set up your processing system the right way not ask you at every packet if that packet requires muxing processing or not. Put in another way, use the library to instantiate the right classes instead of having a single class that works for every signal and should then adapt at runtime to every possible combination.

Beside the performance reason, there is a conceptual reason to me: if you keep asking is signal multiplexed? (in code terms if(signal.IsMultiplexed()) then this implies that the answer may change which, for a given signal, is not true until you change your dbc.

So, ok, I will have a look at performances but please consider instantiate classes that know what to do upfront for best performance results.

Regards A

Uight commented 3 months ago

I pretty much agree with @Adhara3 on this but i guess it could be helpfull to have a IsMultiplexed Property instead of the ExtensionMethod. I wouldnt be using a method for something that cant change. Its pretty much just a nice thing as then you wouldnt have to store it in your own "wrapping" class for message.

Otherwise theres not much to optimize the code of IsMultiplexed(). However if you are using Sonarqube it suggests using Exists instead of Any. (https://sonarsource.github.io/rspec/#/rspec/S6605/csharp) But i doubt that would make a difference as the Signal list is always pretty small. Havents seen any can message with more than 30 signals.

Adhara3 commented 2 months ago

Will be part of the immutability refactoring and will I Claude the extended multiplexing