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

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

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