Skip to content
Open
14 changes: 13 additions & 1 deletion bin/pre-commit/clang_tidy_check.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,19 @@ def main():
if not os.environ.get("TIDY"):
return 0

repo_root = Path(__file__).parent.parent
# Derive the repo root from git so this keeps working regardless of where
# under the tree this script lives. Fall back to the script location
# (bin/pre-commit/ is two levels below the repo root) if git is unavailable.
result = subprocess.run(
["git", "rev-parse", "--show-toplevel"],
capture_output=True,
text=True,
cwd=Path(__file__).parent,
)
if result.returncode == 0:
repo_root = Path(result.stdout.strip())
else:
repo_root = Path(__file__).parent.parent.parent
files = staged_files(repo_root)
if not files:
return 0
Expand Down
8 changes: 4 additions & 4 deletions include/xrpl/server/detail/Door.h
Original file line number Diff line number Diff line change
Expand Up @@ -90,11 +90,11 @@ class Door : public IOList::Work, public std::enable_shared_from_this<Door<Handl
acceptor_type acceptor_;
boost::asio::strand<boost::asio::io_context::executor_type> strand_;
bool ssl_{
port_.protocol.count("https") > 0 || port_.protocol.count("wss") > 0 ||
port_.protocol.count("wss2") > 0 || port_.protocol.count("peer") > 0};
port_.protocol.contains("https") || port_.protocol.contains("wss") ||
port_.protocol.contains("wss2") || port_.protocol.contains("peer")};
bool plain_{
port_.protocol.count("http") > 0 || port_.protocol.count("ws") > 0 ||
(port_.protocol.count("ws2") != 0u)};
port_.protocol.contains("http") || port_.protocol.contains("ws") ||
(port_.protocol.contains("ws2"))};
static constexpr std::chrono::milliseconds kInitialAcceptDelay{50};
static constexpr std::chrono::milliseconds kMaxAcceptDelay{2000};
std::chrono::milliseconds accept_delay_{kInitialAcceptDelay};
Expand Down
7 changes: 7 additions & 0 deletions include/xrpl/tx/wasm/HostFunc.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <xrpl/basics/Expected.h>
#include <xrpl/basics/Slice.h>
#include <xrpl/beast/insight/Event.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Asset.h>
Expand Down Expand Up @@ -121,6 +122,12 @@ struct HostFunctions
return j;
}

[[nodiscard]] virtual beast::insight::Event
executionTimeEvent(std::string_view name) const
{
return {};
}

[[nodiscard]] virtual bool
checkSelf() const
{
Expand Down
10 changes: 10 additions & 0 deletions include/xrpl/tx/wasm/HostFuncImpl.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include <xrpl/core/CollectorManager.h>
#include <xrpl/tx/ApplyContext.h>
#include <xrpl/tx/wasm/HostFunc.h>

Expand Down Expand Up @@ -77,6 +78,15 @@ class WasmHostFunctionsImpl : public HostFunctions
return rt_;
}

beast::insight::Event
executionTimeEvent(std::string_view name) const override
{
return ctx_.registry.get()
.getCollectorManager()
.group(std::string{name.data(), name.size()})
->makeEvent("finish_time");
}

bool
checkSelf() const override
{
Expand Down
17 changes: 16 additions & 1 deletion src/libxrpl/tx/wasm/WasmVM.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <xrpl/tx/wasm/HostFuncWrapper.h> // IWYU pragma: keep
#include <xrpl/tx/wasm/ParamsHelper.h>

#include <chrono>
#include <cstdint>
#include <string>
#include <vector>
Expand Down Expand Up @@ -126,9 +127,16 @@ runEscrowWasm(
auto& vm = WasmEngine::instance();
// vm.initMaxPages(MAX_PAGES);

auto const start = std::chrono::steady_clock::now();

auto const ret =
vm.run(wasmCode, hfs, gasLimit, funcName, params, createWasmImport(hfs), hfs.getJournal());

hfs.executionTimeEvent("runEscrowWasm")
.notify(
std::chrono::duration_cast<std::chrono::milliseconds>(
std::chrono::steady_clock::now() - start));

if (!ret)
{
#ifdef DEBUG_OUTPUT
Expand All @@ -140,7 +148,7 @@ runEscrowWasm(
#ifdef DEBUG_OUTPUT
std::cout << ", ret: " << ret->result << ", gas spent: " << ret->cost << std::endl;
#endif
return EscrowResult{ret->result, ret->cost};
return EscrowResult{.result = ret->result, .cost = ret->cost};
}

NotTEC
Expand All @@ -154,9 +162,16 @@ preflightEscrowWasm(
auto& vm = WasmEngine::instance();
// vm.initMaxPages(MAX_PAGES);

auto const start = std::chrono::steady_clock::now();

auto const ret =
vm.check(wasmCode, hfs, funcName, params, createWasmImport(hfs), hfs.getJournal());

hfs.executionTimeEvent("preflightEscrowWasm")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was a surprise to me too, but the caller in EscrowCreate.cpp is a mock. So it doesn't have the real impl of executionTimeEvent.

        HostFunctions mock(ctx.j);
        auto const re = preflightEscrowWasm(code, mock, escrowFunctionName);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That does seem a bit intentional?

.notify(
std::chrono::duration_cast<std::chrono::milliseconds>(
std::chrono::steady_clock::now() - start));

return ret;
}

Expand Down
8 changes: 4 additions & 4 deletions src/libxrpl/tx/wasm/WasmiVM.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ InstanceWrapper::setGas(std::int64_t gas) const
ModulePtr
ModuleWrapper::init(StorePtr& s, Bytes const& wasmBin, beast::Journal j)
{
wasm_byte_vec_t const code{wasmBin.size(), (char*)(wasmBin.data())};
wasm_byte_vec_t const code{.size = wasmBin.size(), .data = (char*)(wasmBin.data())};
ModulePtr m = ModulePtr(wasm_module_new(s.get(), &code), &wasm_module_delete);
if (!m)
throw std::runtime_error("can't create module");
Expand Down Expand Up @@ -711,8 +711,8 @@ WasmiResult
WasmiEngine::call(FuncInfo const& f, std::vector<wasm_val_t>& in)
{
WasmiResult ret(NR);
wasm_val_vec_t const inv =
in.empty() ? wasm_val_vec_t WASM_EMPTY_VEC : wasm_val_vec_t{in.size(), in.data()};
wasm_val_vec_t const inv = in.empty() ? wasm_val_vec_t WASM_EMPTY_VEC
: wasm_val_vec_t{.size = in.size(), .data = in.data()};

#ifdef SHOW_CALL_TIME
auto const start = usecs();
Expand Down Expand Up @@ -960,7 +960,7 @@ wasm_trap_t*
WasmiEngine::newTrap(std::string const& txt)
{
static char empty[1] = {0};
wasm_message_t msg = {1, empty};
wasm_message_t msg = {.size = 1, .data = empty};

if (!txt.empty())
wasm_name_new(&msg, txt.size() + 1, txt.c_str()); // include 0
Expand Down
71 changes: 71 additions & 0 deletions src/test/app/TestHostFunctions.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
#include <test/jtx/Env.h>
#include <test/unit_test/SuiteJournal.h>

#include <xrpl/beast/insight/Event.h>
#include <xrpl/beast/insight/EventImpl.h>
#include <xrpl/ledger/AmendmentTable.h>
#include <xrpl/ledger/detail/ApplyViewBase.h>
#include <xrpl/ledger/helpers/NFTokenHelpers.h>
Expand All @@ -11,13 +13,72 @@

#include <boost/algorithm/hex.hpp>

#include <algorithm>
#include <cstddef>
#include <cstdint>
#include <iterator>
#include <string>
#include <string_view>
#include <vector>

namespace xrpl::test {

/**
* Lets a test assert that the WASM execution-timing path fired and inspect the
* recorded durations, without needing a StatsD sink or a full Collector.
*/
struct RecordingEventImpl : public beast::insight::EventImpl
{
std::size_t count = 0;
value_type last{};
value_type total{};
value_type min{value_type::max()};
value_type max{value_type::min()};
std::vector<value_type> samples;

~RecordingEventImpl() override
{
if (count > 0)
{
std::cout << "Mean (ms): " << meanMs() << "\n";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: Claude complained see this printed too many times for unrelated tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

As for the resolution of the metrics, the best I can do is fake the values as milliseconds. That is pass in microseconds. StatsD is set at millisecond resolution.

}
}

void
notify(value_type const& value) override
{
++count;
last = value;
total += value;
min = std::min(min, value);
max = std::max(max, value);
samples.push_back(value);
}

[[nodiscard]] double
meanMs() const
{
return count != 0 ? static_cast<double>(total.count()) / static_cast<double>(count) : 0.0;
}

[[nodiscard]] value_type
percentile(double p) const
{
if (samples.empty())
{
return value_type{};
}
auto sorted = samples;
std::ranges::sort(sorted);
auto rank = static_cast<std::size_t>((p / 100.0) * static_cast<double>(sorted.size()));
if (rank >= sorted.size())
{
rank = sorted.size() - 1;
}
return sorted[rank];
}
};

struct TestLedgerDataProvider : public HostFunctions
{
jtx::Env& env;
Expand Down Expand Up @@ -54,6 +115,7 @@ struct TestHostFunctions : public HostFunctions
Bytes data;
int clock_drift = 0;
void* rt = nullptr;
std::shared_ptr<RecordingEventImpl> execTimeEvent = std::make_shared<RecordingEventImpl>();

public:
TestHostFunctions(test::jtx::Env& env, int cd = 0)
Expand All @@ -76,6 +138,15 @@ struct TestHostFunctions : public HostFunctions
return rt;
}

// Return an Event backed by our recording impl so a test can assert that
// the WASM execution was timed. The name is ignored -- every call records
// into the same impl.
[[nodiscard]] beast::insight::Event
executionTimeEvent(std::string_view name) const override
{
return beast::insight::Event(execTimeEvent);
}

Expected<std::uint32_t, HostFunctionError>
getLedgerSqn() const override
{
Expand Down
50 changes: 50 additions & 0 deletions src/test/app/Wasm_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1544,11 +1544,61 @@ struct Wasm_test : public beast::unit_test::Suite
}
}

template <typename Functor>
void
perf(TestHostFunctions& hfs, size_t runs, Functor&& f)
{
for (auto i = size_t{}; i < runs; ++i)
{
auto result = f();
BEAST_EXPECT(result);
}
BEAST_EXPECT(hfs.execTimeEvent->count == runs);
}

void
perfEscrowFinish()
{
testcase("perf escrow finish");
using namespace test::jtx;

static constexpr auto kRuns = 1000;

auto const wasm = hexToBytes(kAllHostFunctionsWasmHex);

Env env{*this};
auto hfns = TestHostFunctions{env, 0};

perf(hfns, kRuns, [&] {
return runEscrowWasm(wasm, hfns, 1'000'000, escrowFunctionName, {}).has_value();
});
}

void
perfEscrowCreate()
{
testcase("perf escrow create");
using namespace test::jtx;

static constexpr auto kRuns = 1000;

auto const wasm = hexToBytes(kAllHostFunctionsWasmHex);

Env env{*this};
auto mock = HostFunctions{env.journal};
Comment thread
TimothyBanks marked this conversation as resolved.
Outdated
auto hfns = TestHostFunctions{env, 0};

perf(hfns, kRuns, [&] { return !preflightEscrowWasm(wasm, hfns, escrowFunctionName); });
}

void
run() override
{
using namespace test::jtx;

perfEscrowFinish();
perfEscrowCreate();

testGetDataHelperFunctions();
testWasmLib();
testBadWasm();
Expand Down
2 changes: 1 addition & 1 deletion src/xrpld/app/main/Application.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include <xrpld/app/ledger/OrderBookDBImpl.h>
#include <xrpld/app/ledger/TransactionMaster.h>
#include <xrpld/app/main/BasicApp.h>
#include <xrpld/app/main/CollectorManager.h>
#include <xrpld/app/main/GRPCServer.h>
#include <xrpld/app/main/LoadManager.h>
#include <xrpld/app/main/NodeIdentity.h>
Expand Down Expand Up @@ -57,6 +56,7 @@
#include <xrpl/beast/utility/PropertyStream.h>
#include <xrpl/beast/utility/instrumentation.h>
#include <xrpl/core/ClosureCounter.h>
#include <xrpl/core/CollectorManager.h>
#include <xrpl/core/HashRouter.h>
#include <xrpl/core/Job.h>
#include <xrpl/core/NetworkIDService.h>
Expand Down
3 changes: 2 additions & 1 deletion src/xrpld/app/main/CollectorManager.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <xrpld/app/main/CollectorManager.h>

#include <xrpl/core/CollectorManager.h>

#include <xrpl/basics/BasicConfig.h>
#include <xrpl/beast/insight/Collector.h>
Expand Down
2 changes: 1 addition & 1 deletion src/xrpld/rpc/ServerHandler.h
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
#pragma once

#include <xrpld/app/main/Application.h>
#include <xrpld/app/main/CollectorManager.h>
#include <xrpld/rpc/detail/WSInfoSub.h>

#include <xrpl/core/CollectorManager.h>
#include <xrpl/core/JobQueue.h>
#include <xrpl/json/Output.h>
#include <xrpl/server/Server.h>
Expand Down
Loading
Loading