open-simulation-platform / libcosim

OSP C++ co-simulation library
https://open-simulation-platform.github.io/libcosim
Mozilla Public License 2.0
55 stars 10 forks source link

Adding a macro to remove fmi logging code in the callback function. #679

Closed davidhjp01 closed 2 years ago

davidhjp01 commented 2 years ago

The current fmi logging callback function is slowing down the simulation (even if it is not printing anything) for high frequency sampling applications. For example, the logging function uses 16% of cpu times in a particular simulation (without proxyfmu):

aa

This update provides an option to remove the logging code using the macro option (via conan).

markaren commented 2 years ago

Before merging, should we discuss how this can be used in practice? What if users of the demo app or cli want to disable logging?

Would it be better to use a runtime option, so that everyone can make use of the feature?

davidhjp01 commented 2 years ago

I made an update to pass an argument to cosim::default_model_uri_resolver and cosim::fmi::importer::create(), so that the logging can be disabled during runtime.

davidhjp01 commented 2 years ago

LIBCOSIM_NO_FMI_LOGGING is now read from the environment variable.

kyllingstad commented 2 years ago

Disabling logging by passing a no-op logger function looks like a good solution.

However, as a general rule, I don't think libraries should take their settings from environment variables. We don't know which applications libcosim will be embedded in, and this could potentially cause user environment variables to break third-party applications. The library should be under full control of the application that uses it, which means that this setting must be controllable via the library API—if it's really needed, that is. (See my question at the end.)

So then the question is where to put it. In the FMI subsystem (cosim::fmi), the natural place seems to be in cosim::fmi::fmu::instantiate_slave(), for example something like this:

virtual std::shared_ptr<slave_instance> instantiate_slave(
    std::string_view instanceName,
    bool disable_logging = false) = 0;

This could be easily implemented in terms of @davidhjp01's no-op logger function.

But our current applications typically don't use this API directly, they use the orchestration layer (in cosim/orchestration.hpp). And there, we don't make a difference between FMU slaves and other kinds of slaves (e.g. proxyfmu). Does it make sense to have a dedicated disable_logging parameter added to cosim::model::instantiate()? I'm not sure.

There are other ways to deal with this, though. For example, (and I think I would prefer this,) it could be limited to the FMI subsystem by a modification to cosim::fmu_file_uri_sub_resolver, e.g. as a resolver-level parameter passed to the constructor:

class fmu_file_uri_sub_resolver : public model_uri_sub_resolver
{
public:
    fmu_file_uri_sub_resolver(disable_logging = false);
    ...

From there, it could be transferred on to each particular cosim::fmu_model instance, and then on to cosim::fmi::fmu::instantiate_slave() from within cosim::fmu_model::instantiate.

But I still don't feel like we've really discussed the necessity of all this. @markaren only asked the question, he didn't say it was 100% necessary. Do anyone else have opinions?

davidhjp01 commented 2 years ago

Anyone has further opinion on this? I am fine with either approach but do think the runtime option would be more flexible to use.

davidhjp01 commented 2 years ago

I reverted the commits back to use the macro for disabling logging. If no one has other opinions I will merge this version when @kyllingstad accepts.

davidhjp01 commented 2 years ago

I will merge this branch soon if no-one has further comments.