Skip to content

Drop need to include ptree in ConfigurableParam.h#11825

Merged
ktf merged 1 commit into
AliceO2Group:devfrom
ktf:fix-macos-builds
Dec 7, 2023
Merged

Drop need to include ptree in ConfigurableParam.h#11825
ktf merged 1 commit into
AliceO2Group:devfrom
ktf:fix-macos-builds

Conversation

@ktf
Copy link
Copy Markdown
Member

@ktf ktf commented Aug 28, 2023 •
edited
Loading

Drop need to include ptree in ConfigurableParam.h

@ktf ktf requested review from a team, davidrohr, sawenzel, shahor02 and wiechula as code owners August 28, 2023 15:06
@ktf
Copy link
Copy Markdown
Member Author

ktf commented Aug 28, 2023

@sawenzel this is an attemp to silence the warning we have on macOS from root including ptree, possibly causing the macro tests to fail. See also:

https://ali-ci.cern.ch/alice-build-logs/AliceO2Group/AliceO2/11821/d5bccca520d335ce203097719ff4afe79c81281e/build_AliceO2_O2_o2_macOS/fullLog.txt

If it works, would you have any objection to this?

@ktf ktf force-pushed the fix-macos-builds branch from e192333 to ea957d9 Compare August 28, 2023 15:23
@ktf ktf requested a review from benedikt-voelkel as a code owner August 28, 2023 15:23
@alibuild
Copy link
Copy Markdown
Collaborator

alibuild commented Aug 28, 2023 •
edited
Loading

Error while checking build/O2/fullCI for ea957d9 at 2023-08-30 06:11:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
/sw/SOURCES/O2/11825-slc8_x86-64/0/Common/Utils/include/CommonUtils/ConfigurableParam.h:207:7: error: there are no arguments to 'assert' that depend on a template parameter, so a declaration of 'assert' must be available [-fpermissive]
/sw/SOURCES/O2/11825-slc8_x86-64/0/Common/Utils/include/CommonUtils/ConfigurableParam.h:207:7: error: there are no arguments to 'assert' that depend on a template parameter, so a declaration of 'assert' must be available [-fpermissive]
/sw/SOURCES/O2/11825-slc8_x86-64/0/Common/Utils/include/CommonUtils/ConfigurableParam.h:207:7: error: there are no arguments to 'assert' that depend on a template parameter, so a declaration of 'assert' must be available [-fpermissive]
/sw/SOURCES/O2/11825-slc8_x86-64/0/Common/Utils/include/CommonUtils/ConfigurableParam.h:207:7: error: there are no arguments to 'assert' that depend on a template parameter, so a declaration of 'assert' must be available [-fpermissive]
/sw/SOURCES/O2/11825-slc8_x86-64/0/Common/Utils/include/CommonUtils/ConfigurableParam.h:207:7: error: there are no arguments to 'assert' that depend on a template parameter, so a declaration of 'assert' must be available [-fpermissive]
ninja: build stopped: subcommand failed.

Full log here.

@ktf ktf force-pushed the fix-macos-builds branch from ea957d9 to 88c9e10 Compare August 30, 2023 13:42
@ktf ktf requested a review from a team as a code owner August 30, 2023 13:42
@alibuild
Copy link
Copy Markdown
Collaborator

alibuild commented Aug 30, 2023

Error while checking build/O2/fullCI for 88c9e10 at 2023-08-30 16:44:

## sw/BUILD/O2-latest/log
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
c++: error: unrecognized command-line option '--rtlib=compiler-rt'
/sw/SOURCES/O2/11825-slc8_x86-64/0/Common/Utils/include/CommonUtils/ConfigurableParam.h:207:13: error: 'assert' was not declared in this scope; did you mean 'mpl_::assert'?
ninja: build stopped: subcommand failed.

Full log here.

@ktf ktf force-pushed the fix-macos-builds branch from 88c9e10 to 27ef06e Compare August 30, 2023 17:51
@ktf ktf requested a review from a team as a code owner August 30, 2023 17:51
@alibuild
Copy link
Copy Markdown
Collaborator

alibuild commented Aug 30, 2023 •
edited
Loading

Error while checking build/O2/fullCI for 27ef06e at 2023-10-13 21:21:

No log files found

Full log here.

@sawenzel
Copy link
Copy Markdown
Collaborator

sawenzel commented Aug 30, 2023

This is somewhat new to me. Why is it problematic now? What did we change on MacOS that made this problem appear?

sawenzel
sawenzel reviewed Aug 30, 2023
View reviewed changes
Comment thread Common/Utils/include/CommonUtils/ConfigurableParam.h Outdated
sawenzel
sawenzel reviewed Aug 30, 2023
View reviewed changes
Comment thread Common/Utils/include/CommonUtils/ConfigurableParam.h
@sawenzel
Copy link
Copy Markdown
Collaborator

sawenzel commented Aug 31, 2023

I quickly did some checks. On my Mac, all macros are tested fine. The log that you mentioned (https://ali-ci.cern.ch/alice-build-logs/AliceO2Group/AliceO2/11821/d5bccca520d335ce203097719ff4afe79c81281e/build_AliceO2_O2_o2_macOS/fullLog.txt
)

shows an "illegal instruction"

/Volumes/build/build/alice-ci-workdir/o2/sw/BUILD/243d0785d2a3116e0733c7e5d90122167377d5b0/O2/test-root-macro.sh: line 36: 59587 Illegal instruction: 4  env ROOT_INCLUDE_PATH=${INCPATH} LD_LIBRARY_PATH=${LIBPATH}

when testing ROOT macros. It is not evident that this is related to warnings from ptrees. So I would prefer to go one step back and actually be sure that there is a problem.

@ktf
Copy link
Copy Markdown
Member Author

ktf commented Sep 4, 2023

I am not sure what is the problem, for sure we have a bunch of spurious messages about ptree right before the crash. I do not know if they are related, so I wanted to remove them to exclude they are the problem.

@ktf ktf force-pushed the fix-macos-builds branch from 27ef06e to 091f9d1 Compare October 23, 2023 12:35
@ktf
Copy link
Copy Markdown
Member Author

ktf commented Oct 23, 2023

For the record, I have updated this to fix the two macros which were failing and split off the unrelated cosmetic changes.

@ktf
Copy link
Copy Markdown
Member Author

ktf commented Oct 30, 2023

Any objections to this? The remaining formatting issues seems to be unrelated to my changes.

@ktf ktf force-pushed the fix-macos-builds branch from 091f9d1 to f75ba9f Compare November 24, 2023 14:12
@ktf
Copy link
Copy Markdown
Member Author

ktf commented Nov 24, 2023

@sawenzel as discussed. The various read* methods are now part of ConfigurableParamReaders.

@ktf ktf force-pushed the fix-macos-builds branch from f75ba9f to de8c022 Compare November 24, 2023 17:18
@AliceO2Group AliceO2Group deleted a comment from alibuild Nov 25, 2023
@AliceO2Group AliceO2Group deleted a comment from alibuild Nov 25, 2023
@ktf
Copy link
Copy Markdown
Member Author

ktf commented Nov 29, 2023

@sawenzel As promised, find below the benchmarking to justify the change. As you can see, my patch saves roughly 400s when compiling (i.e. ~4% of the total parsing + codegen). It's probably a lowerbound, since cling needs to parse those headers as well and it's not being accounted here. Admittedly, since last time I did a benchmark, there is now a unit test in the ANS encoding which is expensive and which we should probably fix in a separate PR. The remaining usage of boost_ptree is probably in actual cxx files getting a value from the FairMQ Properties (also to be fixed at some point). We should also understand why we have so many std::regex, but again, for a different PR.

No changes:

3817 files processed. Summary stats:

Compilation (6284 times):
  Parsing (frontend):         6690.1 s
  Codegen & opts (backend):   1080.2 s

Templates that took longest to instantiate:

 41899 ms: boost::unit_test::data::ds_detail::make_test_case_gen<CTFTestcase, b... (12 times, avg 3491 ms)
 41894 ms: boost::unit_test::data::ds_detail::test_case_gen<CTFTestcase, boost:... (12 times, avg 3491 ms)
 41883 ms: boost::unit_test::data::ds_detail::test_case_gen<CTFTestcase, boost:... (12 times, avg 3490 ms)
 41882 ms: boost::unit_test::data::for_each_sample<boost::unit_test::data::mono... (12 times, avg 3490 ms)
 41869 ms: boost::unit_test::data::invoke_action<boost::unit_test::data::ds_det... (12 times, avg 3489 ms)
 41868 ms: boost::unit_test::data::ds_detail::test_case_gen<CTFTestcase, boost:... (12 times, avg 3489 ms)
 41690 ms: CTFTestcase::test_method<const o2::ctf::ANSHeader &> (12 times, avg 3474 ms)
 41590 ms: CTFTestcase::_impl<o2::ctf::ANSHeader> (12 times, avg 3465 ms)
 36826 ms: boost::property_tree::basic_ptree<std::string, std::string>::subs (1479 times, avg 24 ms)
 34396 ms: boost::property_tree::basic_ptree<std::string, std::string>::get_chi... (1558 times, avg 22 ms)
 32687 ms: boost::property_tree::basic_ptree<std::string, std::string>::walk_path (1474 times, avg 22 ms)
 32252 ms: fmt::vsprintf<char> (307 times, avg 105 ms)
 32090 ms: fmt::detail::vprintf<char, fmt::basic_printf_context<char>> (308 times, avg 104 ms)
 31126 ms: std::__function::__func<(lambda at /Users/ktf/src/sw/SOURCES/O2/66ef... (1422 times, avg 21 ms)
 30206 ms: boost::property_tree::basic_ptree<std::string, std::string>::put<std... (1142 times, avg 26 ms)
 30134 ms: boost::property_tree::basic_ptree<std::string, std::string>::put<std... (1141 times, avg 26 ms)
 29013 ms: boost::property_tree::basic_ptree<std::string, std::string>::get_opt... (1144 times, avg 25 ms)
 28119 ms: std::function<std::function<void (o2::framework::ProcessingContext &... (474 times, avg 59 ms)
 28062 ms: std::__function::__value_func<std::function<void (o2::framework::Pro... (474 times, avg 59 ms)
 27933 ms: std::__function::__value_func<std::function<void (o2::framework::Pro... (474 times, avg 58 ms)
 27022 ms: boost::multi_index::multi_index_container<std::pair<const std::strin... (1479 times, avg 18 ms)
 26151 ms: boost::property_tree::basic_ptree<std::string, std::string>::put_child (1147 times, avg 22 ms)
 25958 ms: boost::property_tree::basic_ptree<std::string, std::string>::iterator (1468 times, avg 17 ms)
 25574 ms: boost::property_tree::basic_ptree<std::string, std::string>::basic_p... (2621 times, avg 9 ms)
 23770 ms: boost::multi_index::multi_index_container<std::pair<const std::strin... (2621 times, avg 9 ms)
 22232 ms: std::vector<std::unique_ptr<fair::mq::Message>>::reserve (1181 times, avg 18 ms)
 20707 ms: std::basic_regex<char>::basic_regex (121 times, avg 171 ms)
 20685 ms: std::basic_regex<char>::__init<const char *> (121 times, avg 170 ms)
 20664 ms: std::basic_regex<char>::__parse<const char *> (121 times, avg 170 ms)
 20207 ms: boost::multi_index::multi_index_container<std::pair<const std::strin... (1469 times, avg 13 ms)

With my patch:

3818 files processed (as it should, there is one extra cxx in my patch). Summary stats:

**** Time summary:
Compilation (6299 times):
  Parsing (frontend):         6405.6 s
  Codegen & opts (backend):   1069.3 s

Templates which took the longest to instantiate:

 37477 ms: boost::unit_test::data::ds_detail::make_test_case_gen<CTFTestcase, b... (12 times, avg 3123 ms)
 37472 ms: boost::unit_test::data::ds_detail::test_case_gen<CTFTestcase, boost:... (12 times, avg 3122 ms)
 37465 ms: boost::unit_test::data::ds_detail::test_case_gen<CTFTestcase, boost:... (12 times, avg 3122 ms)
 37464 ms: boost::unit_test::data::for_each_sample<boost::unit_test::data::mono... (12 times, avg 3122 ms)
 37442 ms: boost::unit_test::data::invoke_action<boost::unit_test::data::ds_det... (12 times, avg 3120 ms)
 37442 ms: boost::unit_test::data::ds_detail::test_case_gen<CTFTestcase, boost:... (12 times, avg 3120 ms)
 37241 ms: CTFTestcase::test_method<const o2::ctf::ANSHeader &> (12 times, avg 3103 ms)
 37132 ms: CTFTestcase::_impl<o2::ctf::ANSHeader> (12 times, avg 3094 ms)
 30824 ms: fmt::vsprintf<char> (307 times, avg 100 ms)
 30747 ms: fmt::detail::vprintf<char, fmt::basic_printf_context<char>> (308 times, avg 99 ms)
 30713 ms: std::__function::__func<(lambda at /Users/ktf/src/sw/SOURCES/O2/66ef... (1422 times, avg 21 ms)
 27749 ms: std::function<std::function<void (o2::framework::ProcessingContext &... (474 times, avg 58 ms)
 27681 ms: std::__function::__value_func<std::function<void (o2::framework::Pro... (474 times, avg 58 ms)
 27567 ms: std::__function::__value_func<std::function<void (o2::framework::Pro... (474 times, avg 58 ms)
 25158 ms: boost::property_tree::basic_ptree<std::string, std::string>::iterator (926 times, avg 27 ms)
 24261 ms: boost::property_tree::basic_ptree<std::string, std::string>::subs (937 times, avg 25 ms)
 21356 ms: std::vector<std::unique_ptr<fair::mq::Message>>::reserve (1181 times, avg 18 ms)
 21273 ms: std::basic_regex<char>::basic_regex (121 times, avg 175 ms)
 21234 ms: std::basic_regex<char>::__init<const char *> (121 times, avg 175 ms)
 21202 ms: std::basic_regex<char>::__parse<const char *> (121 times, avg 175 ms)
 19300 ms: std::basic_regex<char>::__parse_ecma_exp<const char *> (121 times, avg 159 ms)
 19250 ms: std::basic_regex<char>::__parse_alternative<const char *> (121 times, avg 159 ms)
 19247 ms: std::basic_regex<char>::__parse_term<const char *> (121 times, avg 159 ms)
 18572 ms: std::vector<std::unique_ptr<fair::mq::Message>>::__swap_out_circular... (1186 times, avg 15 ms)
 18126 ms: boost::multi_index::multi_index_container<std::pair<const std::strin... (937 times, avg 19 ms)
 17074 ms: std::__function::__func<(lambda at /Users/ktf/src/sw/SOURCES/O2/66ef... (1422 times, avg 12 ms)
 16633 ms: boost::property_tree::basic_ptree<std::string, std::string>::basic_p... (972 times, avg 17 ms)
 16487 ms: std::unordered_map<std::string, fair::mq::Transport>::unordered_map (1191 times, avg 13 ms)
 16070 ms: o2::framework::CallbackService::set<o2::framework::CallbackService::... (383 times, avg 41 ms)
 16031 ms: o2::framework::CallbackRegistry<o2::framework::CallbackService::Id, ... (383 times, avg 41 ms)

@ktf
Copy link
Copy Markdown
Member Author

ktf commented Nov 29, 2023

For the record, as usual the benchmarking was done with https://github.com/aras-p/ClangBuildAnalyzer.

Actual methodology:

pushd sw/BUILD/O2-latest/O2
<modify CMakeCache.txt to include -ftime-trace as CXXFLAGS>
find . -name "*.cxx.*" -delete
ninja install
ClangBuildAnalyzer --all . compile.db
ClangBuildAnalyzer --analyze compile.db
popd

@ktf ktf merged commit 982ea7b into AliceO2Group:dev Dec 7, 2023
@ktf ktf deleted the fix-macos-builds branch December 7, 2023 09:01
@sawenzel sawenzel mentioned this pull request Dec 15, 2023
Merged
andreasmolander pushed a commit to andreasmolander/AliceO2 that referenced this pull request Apr 12, 2024
@ktf @andreasmolander
Drop need to include ptree in ConfigurableParam.h (AliceO2Group#11825)
79b4d5b
mwinn2 pushed a commit to mwinn2/AliceO2 that referenced this pull request Apr 25, 2024
@ktf @mwinn2
Drop need to include ptree in ConfigurableParam.h (AliceO2Group#11825)
399d764
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sawenzel sawenzel sawenzel left review comments

@davidrohr davidrohr Awaiting requested review from davidrohr davidrohr is a code owner

@wiechula wiechula Awaiting requested review from wiechula wiechula is a code owner

@shahor02 shahor02 Awaiting requested review from shahor02 shahor02 is a code owner

@benedikt-voelkel benedikt-voelkel Awaiting requested review from benedikt-voelkel

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ktf @alibuild @sawenzel