Page MenuHomePhabricator

Avoid including a C file in crypto/aes.cpp
AbandonedPublic

Authored by Fabien on Jun 14 2019, 12:00.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

The ctaes lib doesn't come with a build system, but rather than
building it as a C file from the including project, ctaes.c is
included in its wrapper and built as C++.
This diffs changes this behavior by building the ctaes.c file and
linking it to the wrapper instead.

Test Plan
make check
ninja check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
ctaes_remove_c_include
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6338
Build 10723: Bitcoin ABC Buildbot (legacy)
Build 10722: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Jun 14 2019, 13:16

I suspect this might be pretty bad for performance. I also suspect you don't intend to become maintainer of ctaes.

This revision now requires changes to proceed.Jun 14 2019, 13:16
Fabien requested review of this revision.Jun 14 2019, 20:35

I ran the benchmark from D3341 and get the following results:

  • There is no obvious performance difference using Clang 6.0.0
  • There is a low performance improvement with GCC 7.4.0 (~5%)

The aes.cpp file is the wrapper for the ctaes library, but is not part of the library itself, so I supposed it was OK to modify it.

This diff was intended to be a prerequisite for detecting inclusion of C files in D3318, but if you prefer that I avoid taking ownership of this file I can simply add an exception in the linter.

deadalnix requested changes to this revision.Jun 14 2019, 23:38

You prevent any inlining here. This is why you get 5% perf regression. Don't do this.

This revision now requires changes to proceed.Jun 14 2019, 23:38

This is a 5% performance increase, not a regression. Anyway, I discard it.