Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/QMCDrivers/WalkerLogBuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,9 @@ class WalkerLogBuffer

public:
WalkerLogBuffer();
WalkerLogBuffer(const WalkerLogBuffer& other) = default;
WalkerLogBuffer(WalkerLogBuffer&& other) = default;
WalkerLogBuffer& operator=(WalkerLogBuffer&& other) = default;

/// current number of rows in the data buffer
inline size_t nrows() { return buffer.size(0); }
Expand Down
15 changes: 12 additions & 3 deletions src/QMCDrivers/WalkerLogCollector.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,22 @@ class WalkerLogCollector
Array<WLog::PsiVal, 2> Gtmp;
/// tmp storage for walker wavefunciton laplacians
Array<WLog::PsiVal, 1> Ltmp;
/// state data set by WalkerLogManager
const WalkerLogState& state_;
/** Hopefully This is just a bundle of constructor arguments.
* It was a reference, so perhaps, the intention was dynamic
* manipulation of a group of objects state from afar.
* Since it hadn't yet been used this way its just a private member
* now. _reviewers_ do not allow it to be made a reference again.
*
* If you want to do a state transform write a function, document
* it, call it from a sensible and obvious scope.
*/
WalkerLogState state_;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC. WalkerLogManager owns and manipulates the state_ and WalkerLogState only reads it. In this way, per crowd WalkerLogState won't go out of sync. For that reason, keeping it a const ref seems fine to me. What is your motivation to make it a copy especially a non-const copy?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sent out review before my laptop died. Continue reading the added test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot move a ref.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make it at least a const?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So multiple threads spectate on a single memory location to stay "synchronized"? examining the source shows no evidence this anti feature is used and no crowd level resources state should be getting controlled in this manner. If this needs to be updated then a call can be made to change the behavior of the per crowd loggers.


public:
/// constructor. The state should be given by the manager.
WalkerLogCollector(const WalkerLogState& state);

WalkerLogCollector(WalkerLogCollector&& other) = default;
WalkerLogCollector& operator=(WalkerLogCollector&& other) = default;
/// resize buffers to zero rows at beginning of each MC block
void startBlock();

Expand Down
1 change: 0 additions & 1 deletion src/QMCDrivers/WalkerLogManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ WalkerLogManager::WalkerLogManager(WalkerLogInput& inp, bool allow_logs, std::st
registered_hdf = false;
}


std::unique_ptr<WalkerLogCollector> WalkerLogManager::makeCollector() const
{
if (state.verbose)
Expand Down
11 changes: 6 additions & 5 deletions src/QMCDrivers/WalkerLogManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,14 @@ struct WalkerLogInput;
* to the HDF file at the end of each MC block.
*
* Just prior to the write, this class examines the distribution of walker
* energies on its rank for each MC step in the MC block, identifies the
* minimum/maximum/median energy walkers and buffers their full data
* energies on its rank for each MC step in the MC block, identifies the
* minimum/maximum/median energy walkers and buffers their full data
* (including per-particle information) for the write.
* This data corresponds to walker information at specific quantiles of the
* This data corresponds to walker information at specific quantiles of the
* energy distribution.
* See WalkerLogManager::writeBuffers()
*
* Writing per-particle information for all walkers is optional, as is
* Writing per-particle information for all walkers is optional, as is
* writing data for the minimum/maximum/median energy walkers.
* Scalar "property" data for each walker is always written.
*/
Expand Down Expand Up @@ -104,7 +104,8 @@ class WalkerLogManager

public:
WalkerLogManager(WalkerLogInput& inp, bool allow_logs, std::string series_root, Communicate* comm = 0);

WalkerLogManager& operator=(WalkerLogManager&& other) = default;
WalkerLogManager(WalkerLogManager&& other) = default;
/// create a WalkerLogCollector
std::unique_ptr<WalkerLogCollector> makeCollector() const;

Expand Down
38 changes: 38 additions & 0 deletions src/QMCDrivers/tests/ValidWalkerLogInput.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
//////////////////////////////////////////////////////////////////////////////////////
// This file is distributed under the University of Illinois/NCSA Open Source License.
// See LICENSE file in top directory for details.
//
// Copyright (c) 2025 QMCPACK developers.
//
// File developed by: Peter Doak, doakpw@ornl.gov, Oak Ridge National Laboratory
//////////////////////////////////////////////////////////////////////////////////////

#ifndef QMCPLUSPLUS_VALIDWALKERLOGINPUT_H
#define QMCPLUSPLUS_VALIDWALKERLOGINPUT_H

#include <array>
#include <string_view>
namespace qmcplusplus::testing
{
class WalkerLogInputSections
{
public:
enum class valid
{
DEFAULT = 0
};

static std::string_view getXml(valid val) { return xml[static_cast<std::size_t>(val)]; }
auto begin() { return xml.begin(); }
auto end() { return xml.end(); }

private:
static constexpr std::array<std::string_view, 1> xml{
R"XML(
<walkerlogs/>
)XML"};
};

} // namespace qmcplusplus::testing

#endif
69 changes: 69 additions & 0 deletions src/QMCDrivers/tests/test_WalkerLogManager.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
//////////////////////////////////////////////////////////////////////////////////////
// This file is distributed under the University of Illinois/NCSA Open Source
// License. See LICENSE file in top directory for details.
//
// Copyright (c) 2025 QMCPACK developers.
//
// File developed by: Peter W. Doak, doakpw@ornl.gov, Oak Ridge National
// Laboratory
//////////////////////////////////////////////////////////////////////////////////////

#include "OhmmsData/Libxml2Doc.h"
#include "QMCDrivers/WalkerLogManager.h"
#include "catch.hpp"

#include "Message/Communicate.h"
#include <MockGoldWalkerElements.h>

#include "ValidWalkerLogInput.h"
#include "WalkerLogInput.h"

namespace qmcplusplus
{
class CollectorHolder
{
public:
void setWalkerLogCollector(UPtr<WalkerLogCollector>&& wlc) { walker_collector_ = std::move(wlc); }
void startBlock() { walker_collector_->startBlock(); }

private:
UPtr<WalkerLogCollector> walker_collector_;
};

struct LogAndStuff
{
WalkerLogManager wlm;
CollectorHolder ch;
};

TEST_CASE("WalkerLogManager::move", "[drivers]")
{
Communicate* comm = OHMMS::Controller;
RuntimeOptions run_time_options;

auto mgwe = testing::makeGoldWalkerElementsWithEEEI(comm, run_time_options);
using WLInput = testing::WalkerLogInputSections;
Libxml2Document doc;
bool okay = doc.parseFromString(WLInput::getXml(WLInput::valid::DEFAULT));
REQUIRE(okay);
xmlNodePtr node = doc.getRoot();
WalkerLogInput walker_log_input{node};
auto make_stuff = [comm](WalkerLogInput& walker_log_input) -> LogAndStuff {
WalkerLogManager wlm{walker_log_input, true, "root_name", comm};
CollectorHolder ch;
ch.setWalkerLogCollector(wlm.makeCollector());
return {std::move(wlm), std::move(ch)};
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This factory function shows the tight connection between the manager and collectors as it returns them as a whole structure.

We can write the code as

   auto make_stuff = [comm](WalkerLogInput& walker_log_input) -> LogAndStuff {
-    WalkerLogManager wlm{walker_log_input, true, "root_name", comm};
-    CollectorHolder ch;
-    ch.setWalkerLogCollector(wlm.makeCollector());
-    return {std::move(wlm), std::move(ch)};
+    LogAndStuff logger{{walker_log_input, true, "root_name", comm}, {}};
+    logger.ch.setWalkerLogCollector(logger.wlm.makeCollector());
+    return logger;
   };

In this way, the hassle of move constructors can be avoided and also reduce the number copy operations.

The intention of introducing move constructor and operator is to bring real benefit. It is not worth maintaining move constructor and operators rigorously in every class but only places we benefit. Hopefully everyone agrees with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above was my opinion. This is not a request for change.
We can keep the code as you changed for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a point in blocking this other than making the refactoring I need to do to implement effective testing for a requested feature harder? I think ease of testing refactoring trumps weasling out of providing sane threaded design and basic constructors.

I'm correcting a design issue that should have never gotten through review the reference used as a back door state update and fixing the lack of clear declaration of constructors.

Copy link
Contributor

@ye-luo ye-luo Jul 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably you missed my other comment, I'm not making a request for change in this area of code. The blocking reason is missing CMake changes in this PR. Your added code is not compiled and tested.

To be honest, the name state made me feel it may carry variable like step counters. If that was the case, I wanted it to be a const ref in collectors and let the manager to operate it.
But the source code WalkerLogState really shows it is just parsed input. So copy is fine.

auto l_and_s = make_stuff(walker_log_input);

// In the past this has resulted in a AddressSanitizer:
// stack-use-after-return
// due to access to a dead reference to
// a state object made back in moved from WalkerLogManager in
// the factory function. Seemed to work in release, caught by llvm
// asan.
l_and_s.ch.startBlock();
}


} // namespace qmcplusplus
Loading