4847 logger singleton 3.0 by kbenne · Pull Request #5110 · NatLabRockies/OpenStudio

kbenne · 2024-03-13T14:48:09Z

@jmarrec I think this should address the core issue that the logger is not global across the script engines. I think there remains a little bit of work.

I'm going to rename LoggerSingleton since it is not really a singleton (Logger is) and the name is confusing. Also, LoggerSingleton is not part of the public API so I think we can change it without causing harm.
I think we might be able to remove the define SHARED_OS_LIBS. What do you think?
Now that I believe we have the global issue worked out, do you have availability to go through the CLI's use of the logger and undo any workarounds that might be in place? I believe there are some if Windows conditions that attempted to work around the issue.

jmarrec · 2024-03-14T20:13:32Z

@kbenne can you do a quick walkthrough with some comments please?

jmarrec · 2024-03-14T20:14:18Z

ProjectMacros.cmake

    set_target_properties(${swig_target} PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_RUNTIME_OUTPUT_DIRECTORY}/ruby/")
    target_link_libraries(${swig_target} ${${PARENT_TARGET}_depends})
    target_include_directories(${swig_target} SYSTEM PRIVATE ${RUBY_INCLUDE_DIRS})
+    target_compile_definitions(${swig_target} PRIVATE SHARED_OS_LIBS)


SHARED_OS_LIBS needed because?

The UtilitiesAPI preprocessor logic will not apply the required declspec if SHARED_OS_LIBS is not defined. That said, I think we could do away with entire SHARED_OS_LIBS definition with a bit of work. I don't think there is ever a configuration where we aren't using the openstudio.dll.

jmarrec · 2024-03-14T20:14:48Z

python/engine/CMakeLists.txt

 target_link_libraries(
  pythonengine
  PRIVATE
+  openstudiolib


We need this just because of the logger? isn't it part of the minimal?

Yes we need this because of the Logger and yes Logger is part of the minimal utilities target, however minimal is a different compiled unit and since we want a single global Logger we cannot have it implmented in multiple compiled units. In fact, I think we can do away with the minimal target after this.

jmarrec · 2024-03-14T20:15:24Z

src/energyplus/EnergyPlus.i



-#define UTILITIES_API
+%include <utilities/UtilitiesAPI.hpp>


? We need to dllexport the swig stuff inside the swig lib now?

The swig generated code includes Logger.hpp which declares the Logger. We either need to link the Logger implementation into the swig generated code (like we have been doing with utilities minimal), or we need to use dllimport to signal that the implentation will come from a DLL. The problem with the former is that while linking to utilities minimal satisfies the linker, we had multiple implementations of the Logger floating around therefore it was not actually global. TLDR we need to declspec the Logger API everywhere that it is used.

jmarrec · 2024-03-14T20:16:02Z

src/utilities/core/Logger.hpp

+  friend class Logger;

 public:
  /// destructor, cleans up, writes xml file footers, etc


(what is this comment?)

Old cruft I guess. I think we can delete these lines.

jmarrec · 2024-03-14T20:17:12Z

src/utilities/core/Logger.hpp

 public:
  /// destructor, cleans up, writes xml file footers, etc
-  ~LoggerSingleton();
+  ~LoggerImpl();


Is the dtor really needed? it's empty. = default at least

Yeah. I agree.

jmarrec · 2024-03-14T20:18:56Z

I rebased onto develop after marking it "Ready for CI" so that CI tries to build it. Curious to see if the windows logger test is fixed

jmarrec · 2024-03-22T10:36:53Z

Windows failing logger test in "classic" CLI

1/1 Test #3731: OpenStudioCLI.Classic.test_logger_rb ...***Failed    0.87 sec
Namespace(classic=True, logger_file=WindowsPath('D:/git/OpenStudio/src/cli/test/logger_test.rb'), os_cli_path=WindowsPath('D:/git/OpenStudio/build/Products/openstudio.exe'))
Running: D:\git\OpenStudio\build\Products\openstudio.exe classic execute_ruby_script D:\git\OpenStudio\src\cli\test\logger_test.rb
0 LOGGER - STDOUT Warn
1 LOGGER - STDOUT Error
Traceback (most recent call last):
  File "D:/git/OpenStudio/src/cli/test/run_test_logger.py", line 50, in <module>
    raise IOError(f"Expected 3 lines, got {n}")
OSError: Expected 3 lines, got 2

https://github.com/NREL/OpenStudio/blob/800d735ae010994a9159c7e2fde8f72debb46e69/src/cli/test/logger_test.rb#L1-L15

So that OpenStudio::logFree(OpenStudio::Error, "test", "Error") is not printed to the stdout

https://github.com/NREL/OpenStudio/blob/800d735ae010994a9159c7e2fde8f72debb46e69/src/cli/main.cpp#L83-L90

Previously, we used the openstudio::Singleton template to create the singleton named Logger. This implementation suffered a bug described in #4847, where the singleton was not global across the ScriptEngine DLL boundary. This change is a non templated implementation of Logger. The design is identical to openstudio::Singleton<LoggerSingleton>, but does not suffer the non-global issue. ref #4847

UtilitiesAPI.hpp needs to be included by SWIG so that the correct declspecs are used for the global logger. Previously, UTILITIES_API was just #define OPENSTUDIO_API, therefore the dll export decorator was not used.

ref #4847

jmarrec · 2024-03-22T10:41:17Z

src/utilities/core/Logger.hpp

-/// Explicitly instantiate and export LoggerSingleton Singleton template instance
-/// so that the same instance is shared between the DLL's that link to Utilities.dll
-UTILITIES_TEMPLATE_EXT template class UTILITIES_API openstudio::Singleton<LoggerSingleton>;


FWIW, openstudio::Singleton should be removed everywhere. It was needed a long time ago, but the same can be achieved with a Meyers' Singleton like you did (static ref& instance())

jmarrec · 2024-03-22T10:53:09Z

src/utilities/core/Logger.hpp

-/// Explicitly instantiate and export LoggerSingleton Singleton template instance
-/// so that the same instance is shared between the DLL's that link to Utilities.dll
-UTILITIES_TEMPLATE_EXT template class UTILITIES_API openstudio::Singleton<LoggerSingleton>;
+  static LoggerImpl& instance() {


I find this file weird for two reasons:

Why not create a Logger_Impl.hpp to put the impl inside

put it in namespace detail

Rename class to Logger_Impl).

=> That's the Impl pattern we have all over openstudio

Why is this static method definition in the header and not the cpp?

I think I'd also like to follow Meyers' singleton pattern. Something feels a bit off here for me.

I think we need to keep the name of the thing that has the ::instance method as Logger otherwise we would break third party Measures. The question then becomes what do you call the thing that is returned by Logger::instance? I felt like LoggerSingleton was a really unfortunate name because it isn't a singleton (and the name at least confused me at first) so I renamed LoggerSingleton as LoggerImpl, which isn't great because it implies the Impl pattern that is used throughout OpenStudio, which it is not. I do think we could call it something else though, I just couldn't come up with a better name. I would be ok with renaming LoggerImplas Logger_Impl and moving it under namesapce detail. Note that it is still a public thing, but then again so are all of our other Impls.

I don't think there's a particular reason why the static method definition is in the header and not the cpp. That is how the original code was and it didn't bother me enough to change.

The Meyers' singleton had one unfortunate side effect that was messing with us in this case, and that is it makes it easy to end up with multiple definitions. The singleton that I used here (phoenix singleton?) will yield a linker error if you don't link the one (and only) library where the singleton is defined. This helped me work through the issue. I also believe that we could use Meyers' singleton and move the defintion into the cpp and accomplish the same thing.

You return a Logger, cf #5119

class Logger { public: static Logger& instance() { static Logger logger; return logger; } Logger(const Logger& other) = delete; Logger(Logger&& other) = delete; Logger& operator=(const Logger&) = delete; Logger& operator=(Logger&&) = delete; private: Logger(); }

I do follow the point about the Phoenix Singleton though.

in #5119 I moved the instance() definition to the cpp file already, do you think that's enough?

Yes, I do think that is enough. If the new test that I wrote is passing (on windows) then I am convinced.

ci-commercialbuildings · 2024-03-22T11:24:54Z

CI Results for c162c04:

Ubuntu 20.04:
- OpenStudio-3.8.0-beta+c162c0492a-Ubuntu-20.04-x86_64.deb
- OpenStudio-3.8.0-beta+c162c0492a-Ubuntu-20.04-x86_64.tar.gz
Ubuntu 22.04:
- OpenStudio-3.8.0-beta+c162c0492a-Ubuntu-22.04-x86_64.deb
- OpenStudio-3.8.0-beta+c162c0492a-Ubuntu-22.04-x86_64.tar.gz
Darwin:
- OpenStudio-3.8.0-beta+c162c0492a-Darwin-x86_64.dmg
- OpenStudio-3.8.0-beta+c162c0492a-Darwin-x86_64.tar.gz
Windows:
- OpenStudio-3.8.0-beta+c162c0492a-Windows.exe
- OpenStudio-3.8.0-beta+c162c0492a-Windows.tar.gz
Code Coverage:

jmarrec · 2024-03-22T11:34:06Z

src/utilities/core/Logger.cpp


 namespace openstudio {

+std::shared_ptr<LoggerImpl> Logger::obj = nullptr;


So that's the bit that actually makes it global right?

jmarrec · 2024-03-27T16:19:13Z

Supperseded by #5119

kbenne requested a review from jmarrec March 13, 2024 14:48

kbenne self-assigned this Mar 13, 2024

kbenne marked this pull request as draft March 14, 2024 17:07

jmarrec reviewed Mar 14, 2024

View reviewed changes

jmarrec added Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. component - Ruby bindings component - Python bindings labels Mar 14, 2024

jmarrec force-pushed the 4847_LoggerSingleton_3.0 branch from 5508f9f to 7f2989b Compare March 14, 2024 20:18

kbenne and others added 6 commits March 22, 2024 11:44

Remove now duplicate and conflicting Logger template definition

659d01c

Add test for Logger global instance

3984b2b

Make Logger::instance more global

aad08ee

UtilitiesAPI.hpp needs to be included by SWIG so that the correct declspecs are used for the global logger. Previously, UTILITIES_API was just #define OPENSTUDIO_API, therefore the dll export decorator was not used.

Rename LoggerSingleton to LoggerImpl

69c474b

ref #4847

Remove poor man's hack for #4847

c162c04

jmarrec force-pushed the 4847_LoggerSingleton_3.0 branch from 7f2989b to c162c04 Compare March 22, 2024 10:46

jmarrec reviewed Mar 22, 2024

View reviewed changes

jmarrec mentioned this pull request Mar 22, 2024

4847 logger singleton 3.1 #5119

Merged

19 tasks

jmarrec merged commit c162c04 into develop Mar 27, 2024

jmarrec deleted the 4847_LoggerSingleton_3.0 branch March 27, 2024 16:19


		namespace openstudio {

		std::shared_ptr<LoggerImpl> Logger::obj = nullptr;

Conversation

kbenne commented Mar 13, 2024

Uh oh!

jmarrec commented Mar 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbenne Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec commented Mar 14, 2024

Uh oh!

jmarrec commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ci-commercialbuildings commented Mar 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarrec commented Mar 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kbenne Mar 14, 2024 •

edited

Loading

jmarrec commented Mar 22, 2024 •

edited

Loading

jmarrec Mar 25, 2024 •

edited

Loading

jmarrec Mar 25, 2024 •

edited

Loading

ci-commercialbuildings commented Mar 22, 2024 •

edited

Loading