Paths

Table of Contentst

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

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

Details

Reviewers

deadalnix
Fabien
jasonbcox

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

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

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

nakihito created this revision.Jan 23 2020, 00:33

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

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 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.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9113: Diff 15751.Jan 23 2020, 00:51

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

Included PR17218.

nakihito planned changes to this revision.Jan 31 2020, 22:35

Build Bitcoin-ABC / Diffs / Diff Testing started.

nakihito edited the summary of this revision. (Show Details)Jan 31 2020, 22:37

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)

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9222: Diff 15934.Jan 31 2020, 22:42

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.

deadalnix accepted this revision.Feb 3 2020, 01:38

Fabien accepted this revision.Feb 3 2020, 14:57

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

Closed by commit rABC0f47175d0df5: Merge #14209/#17218: logging: Replace LogPrintf macro with regular function (authored by Wladimir J. van der Laan <laanwj@gmail.com>, committed by nakihito). · Explain WhyFeb 3 2020, 20:15

This revision was automatically updated to reflect the committed changes.

nakihito added a commit: rABC0f47175d0df5: Merge #14209/#17218: logging: Replace LogPrintf macro with regular function.

nakihito mentioned this in D5144: Add LogPrint Benchmark.Feb 3 2020, 23:55

Fabien mentioned this in D5321: [CMAKE] Add support for generating test coverage reports.Feb 24 2020, 21:42

nakihito added a commit: rSTAGING0f47175d0df5: Merge #14209/#17218: logging: Replace LogPrintf macro with regular function.Apr 7 2020, 22:47

Fabien mentioned this in rABC100c22d068f4: [CMAKE] Add support for generating test coverage reports.Apr 17 2020, 07:08

Fabien mentioned this in rSTAGING100c22d068f4: [CMAKE] Add support for generating test coverage reports.May 5 2020, 19:00