Page MenuHomePhabricator

Merge #14209/#17218: logging: Replace LogPrintf macro with regular function
ClosedPublic

Authored by nakihito on Jan 23 2020, 00:33.

Details

Summary

fae3fbd61a89c7a35bc0eda91b1df61371dc0936 logging: Replace LogPrint macros with regular functions (MarcoFalke)

Pull request description:

It is not possible to run the full test suite when configured with `--enable-lcov`, since logging is disabled currently so that "unnecessary branches are not analyzed". (See c8914b9dbbf6106dac3c62769f7ce3bacd8fbf9b)

Fix this instead by replacing the macros with functions.

Tree-SHA512: 101aa4f4a3ffcefc38faf70c9d3deb5fc63e0b11ca54a164d0463931c79eaf53ab0b0c6ae92a45355574e3b1d2c32233874a6b24293e7a09d188fc6698e212a5

Only LogPrintf() is replaced due to PR17218 reversing the change.

Backport of Core PR14209 and PR17218

Test Plan
make check
./bitcoind -printtoconsole=0 -debuglogfile=0

Verify no logging information is printed.

./bitcoind -printtoconsole=0

Verify logging information is printed to debug.log file.

./bitcoind

Verify logging information is printed to console.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR14209
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9222
Build 16389: Default Diff Build & Tests
Build 16388: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Jan 23 2020, 00:33

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those Bitcoin Core PRs have been inserted into the summary for reference.

Fabien requested changes to this revision.Jan 23 2020, 09:06

Can you make sure that this is not going to cause issues in the light of https://github.com/bitcoin/bitcoin/pull/17218 ?

This revision now requires changes to proceed.Jan 23 2020, 09:06
nakihito retitled this revision from Merge #14209: logging: Replace LogPrint macros with regular functions to Merge #14209/#17218: logging: Replace LogPrintf macro with regular function.
nakihito edited the summary of this revision. (Show Details)

In terms of behavior, there is no issue with changing both to functions. However, this change causes LogPrint() to slow down immensely:
LogPrint() as a Function:

BenchmarkLogPrint, 5, 10000000, 2.96021, 5.59626e-08, 7.15266e-08, 5.617e-08
BenchmarkLogPrint, 5, 10000000, 2.96133, 5.59568e-08, 6.75059e-08, 5.72569e-08

LogPrint() as a macro:

BenchmarkLogPrint, 5, 10000000, 0.717328, 1.43326e-08, 1.43745e-08, 1.43441e-08
BenchmarkLogPrint, 5, 10000000, 0.674096, 1.34514e-08, 1.35127e-08, 1.348e-08
#include <bench/bench.h>
#include <chainparams.h>
#include <logging.h>

static void BenchmarkLogPrint(benchmark::State& state) {
    SelectParams(CBaseChainParams::REGTEST);

    while (state.KeepRunning()) {
        LogPrint(BCLog::NET, "%s default port=%s", __func__, Params().GetDefaultPort());
    }
}

BENCHMARK(BenchmarkLogPrint, 10 * 1000 * 1000);

So I included the reversion from PR17218.

This revision is now accepted and ready to land.Feb 3 2020, 14:57