Reduce boost usage #16

pull vasild wants to merge 4 commits into bitcoin-core:master from vasild:reduce-boost-usage changing 3 files +71 −51

vasild commented at 7:55 PM on December 6, 2019: contributor

A few changes that remove the usage of boost from some places in the code.
Remove unnecessary boost include cab9c51669

Remove boost usage from GetAnnotation()

Split GetAnnotation() to 3 functions that emit the result via an
output parameter and return true/false to designate whether it was
set or not.

5724a2c58a

vasild commented at 9:09 AM on December 11, 2019: contributor

The remaining boost usages would need bigger changes. Is fully removing boost to be attempted? The README contains "The boost dependency should be eliminated".
in src/mp/proxy.cpp:177 in e791cc5d15 outdated
```
 172 | @@ -170,7 +173,8 @@ void EventLoop::loop()
 173 |          m_cv.notify_all();
 174 |          if (m_num_clients == 0 && m_async_fns.empty()) {
 175 |              log() << "EventLoop::loop done, cancelling event listeners.";
 176 | -            m_task_set.reset();
 177 | +            using TaskSet = kj::TaskSet;
 178 | +            m_task_set.~TaskSet();
```
ryanofsky commented at 9:51 PM on December 16, 2019:

In commit "Change EventLoop::m_task_set to not use boost" (e791cc5d15d259ab768480c733f633175f39dcc6):

Calling the task set destructor explicitly here isn't really safe, because the destructor will get called a second time when the event loop object is destroyed, which would be undefined behavior (https://stackoverflow.com/questions/2771567/why-exactly-is-calling-the-destructor-for-the-second-time-undefined-behavior-in)

To prevent the destructor from being called twice while still getting rid of boost::optional, I think the simplest thing to do would probably be to replace boost::optional<kj::TaskSet> with std::unique_ptr<kj::TaskSet>.

Do you think you could update the PR with this change, or prevent the double destruction in another way?

vasild commented at 6:17 PM on December 17, 2019:

Yes, I will fix the double free. Good catch! Thanks!

vasild commented at 7:27 PM on January 1, 2020:

Changed to std::unique_ptr, this also reduced the amount of changes.
ryanofsky changes_requested
ryanofsky commented at 10:12 PM on December 16, 2019: collaborator

Thanks for the PR! I'd like to merge this if the m_task_set double-destroy problem mentioned below is addressed.

The remaining boost usages would need bigger changes. Is fully removing boost to be attempted? The README contains "The boost dependency should be eliminated".

Yes, I'd like to remove the boost dependency, and I see after this PR the remaining boost::optional usages are more complicated to get rid of. One way might be to implement a small custom Optional template class that only supports needed emplace, reset, operator bool, and operator* methods. I don't think this would be too difficult to do. One way might be to start from another optional implementation like https://github.com/abseil/abseil-cpp/blob/master/absl/types/optional.h and just remove all the code there that isn't needed.
vasild force-pushed on Jan 1, 2020
ryanofsky approved
ryanofsky commented at 8:29 PM on January 6, 2020: collaborator

Code review ACK 8349490601cbc94e9ac90e969c9c97f0f85f6319. Thanks for the change and followup!

ryanofsky commented at 8:50 PM on January 6, 2020: collaborator

I'm seeing a compiler error due to a (pre-existing) missing include exposed by this change:

[ 85%] Building CXX object CMakeFiles/mpgen.dir/src/mp/gen.cpp.o
libmultiprocess/src/mp/gen.cpp: In function ‘void Generate(kj::StringPtr, kj::StringPtr, kj::StringPtr, kj::ArrayPtr<const kj::StringPtr>)’:
libmultiprocess/src/mp/gen.cpp:148:77: warning: ‘capnp::ParsedSchema capnp::SchemaParser::parseDiskFile(kj::StringPtr, kj::StringPtr, kj::ArrayPtr<const kj::StringPtr>) const’ is deprecated [-Wdeprecated-declarations]
     auto file_schema = parser.parseDiskFile(src_file, src_file, import_paths);
                                                                             ^
In file included from libmultiprocess/src/mp/gen.cpp:8:0:
include/capnp/schema-parser.h:103:16: note: declared here
   ParsedSchema parseDiskFile(kj::StringPtr displayName, kj::StringPtr diskPath,
                ^~~~~~~~~~~~~
libmultiprocess/src/mp/gen.cpp:169:10: error: ‘transform’ is not a member of ‘std’
     std::transform(guard.begin(), guard.end(), guard.begin(), [](unsigned char c) {
          ^~~~~~~~~

Adding #include <algorithm> to src/mp/gen.cpp fixes it. I'll try to edit the PR and merge it today if github will allow editing. If not, I'll just wait a day and merge this tomorrow, following up with the fix separately if still needed.

Change Field::(param and result) to not use boost

Fall back to the KISS boolean flag denoting whether the variable is set
or not.

138ad6794a

Change EventLoop::m_task_set to not use boost

EventLoop::m_task_set was unnecessary defined as
boost::optional<kj::TaskSet> because it is always initialized.

It can, however, possibly be destroyed twice: in the EventLoop::loop()
method (conditional) and in the EventLoop destructor when it goes out of
scope (unconditional). So, use std::unique_ptr to handle this.

fb73b81b98

ryanofsky force-pushed on Jan 6, 2020
ryanofsky referenced this in commit abb3ae9ce9 on Jan 6, 2020
ryanofsky merged this on Jan 6, 2020
ryanofsky closed this on Jan 6, 2020
ryanofsky commented at 9:06 PM on January 6, 2020: collaborator

I was able to add the <algorithm> include and merge as abb3ae9ce9a0837e60013a1c413888859983b3d4. Thanks again!
vasild commented at 9:09 PM on January 6, 2020: contributor

Thanks! I was just about to fix it, but you were quicker :) I did not get that compilation error (clang 8.0.1), but I guess that is not so interesting now.

Next thing, time permitting, I will look to nuke the other boost usages.
vasild deleted the branch on Jan 6, 2020
bitcoin-core locked this on Jun 25, 2025

Contributors

vasild

ryanofsky

Linked (view graph)

#115 MSAN Error during connection shutdown #162 macos "Broken pipe" exceptions during tests #219 SIGABRT in ~ProxyClientBase with #29409 and rust client