From cef6425bcac678157f58e9eafabb7e63c5831d18 Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Sun, 7 Aug 2022 15:49:54 -0400 Subject: [PATCH] Disable QTC inside the library by default (fixes #714) This results in measurable performance improvements to packaged binary libqpdf distributions. QTC remains available for library users and is still selectively enabled in CI. --- CMakeLists.txt | 13 ++++++++++++- ChangeLog | 8 ++++++++ TODO | 7 +------ build-scripts/test-sanitizers | 3 ++- include/qpdf/QTC.hh | 16 +++++++++++++++- libqpdf/QTC.cc | 2 +- manual/installation.rst | 12 ++++++++++++ manual/release-notes.rst | 14 ++++++++++++++ run-qtest | 7 ++++++- 9 files changed, 71 insertions(+), 11 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3e50f0b5..2581fd7f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -42,6 +42,9 @@ CMAKE_DEPENDENT_OPTION( CMAKE_DEPENDENT_OPTION( GENERATE_AUTO_JOB "Automatically regenerate job files" OFF "NOT MAINTAINER_MODE" ON) +CMAKE_DEPENDENT_OPTION( + ENABLE_QTC "Enable QTC test coverage" OFF + "NOT MAINTAINER_MODE" ON) CMAKE_DEPENDENT_OPTION( SHOW_FAILED_TEST_OUTPUT "Show qtest output on failure" OFF "NOT CI_MODE" ON) @@ -110,8 +113,15 @@ endif() add_compile_definitions($<$:POINTERHOLDER_TRANSITION=4>) +if(ENABLE_QTC) + set(ENABLE_QTC_ARG) +else() + add_compile_definitions(QPDF_DISABLE_QTC=1) + set(ENABLE_QTC_ARG --disable-tc) +endif() + enable_testing() -set(RUN_QTEST perl ${qpdf_SOURCE_DIR}/run-qtest) +set(RUN_QTEST perl ${qpdf_SOURCE_DIR}/run-qtest ${ENABLE_QTC_ARG}) if(WIN32) find_program(COPY_COMMAND NAMES cp copy) @@ -335,6 +345,7 @@ message(STATUS " build shared libraries: ${BUILD_SHARED_LIBS}") message(STATUS " build static libraries: ${BUILD_STATIC_LIBS}") message(STATUS " build manual: ${BUILD_DOC}") message(STATUS " compiler warnings are errors: ${WERROR}") +message(STATUS " QTC test coverage: ${ENABLE_QTC}") message(STATUS " system: ${CPACK_SYSTEM_NAME}") message(STATUS "") message(STATUS "*** Options Summary ***") diff --git a/ChangeLog b/ChangeLog index bc479f17..b861ddda 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2022-08-07 Jay Berkenbilt + + * Add new build configuration option ENABLE_QTC, which is off by + default when not running in MAINTAINER_MODE. When this is off, + QTC coverage calls sprinkled throughout the qpdf source code are + compiled out for increased performance. See "Build Options" in the + manual for a discussion. Fixes #714. + 2022-08-06 Jay Berkenbilt * Added by m-holger: QPDF::getObject() method as a simpler form of diff --git a/TODO b/TODO index fc007c98..1b452805 100644 --- a/TODO +++ b/TODO @@ -4,14 +4,9 @@ Next Before Release: -* Improve performance around QTC - * Make it possible to compile it out and deal with it in the tests - * Make sure at least some CI test is run with coverage enabled - * Cache environment variables - * Remove coverage cases for things that are heavily exercised or are - in critical paths * Make ./performance_check usable by other people by having published files to use for testing. + * https://opensource.adobe.com/dc-acrobat-sdk-docs/standards/pdfstandards/pdf/PDF32000_2008.pdf * Evaluate issues tagged with `next` * Stay on top of https://github.com/pikepdf/pikepdf/pull/315 diff --git a/build-scripts/test-sanitizers b/build-scripts/test-sanitizers index c3c314f9..75ac8af0 100755 --- a/build-scripts/test-sanitizers +++ b/build-scripts/test-sanitizers @@ -10,7 +10,8 @@ env CFLAGS="-fsanitize=address -fsanitize=undefined" \ CC=clang CXX=clang++ \ cmake -S . -B build \ -DCI_MODE=1 -DBUILD_SHARED_LIBS=0 -DCMAKE_BUILD_TYPE=Debug \ - -DREQUIRE_CRYPTO_OPENSSL=1 -DREQUIRE_CRYPTO_GNUTLS=1 + -DREQUIRE_CRYPTO_OPENSSL=1 -DREQUIRE_CRYPTO_GNUTLS=1 \ + -DENABLE_QTC=1 cmake --build build -j$(nproc) -- -k cd build # libtests automatically runs with all crypto providers. diff --git a/include/qpdf/QTC.hh b/include/qpdf/QTC.hh index 1fa55901..70115981 100644 --- a/include/qpdf/QTC.hh +++ b/include/qpdf/QTC.hh @@ -24,10 +24,24 @@ #include +// Defining QPDF_DISABLE_QTC will effectively compile out any QTC::TC +// calls in any code that includes this file, but QTC will still be +// built into the library. That way, it is possible to build and +// package qpdf with QPDF_DISABLE_QTC while still making QTC::TC +// available to end users. + namespace QTC { QPDF_DLL - void TC(char const* const scope, char const* const ccase, int n = 0); + void TC_real(char const* const scope, char const* const ccase, int n = 0); + + inline void + TC(char const* const scope, char const* const ccase, int n = 0) + { +#ifndef QPDF_DISABLE_QTC + TC_real(scope, ccase, n); +#endif // QPDF_DISABLE_QTC + } }; // namespace QTC #endif // QTC_HH diff --git a/libqpdf/QTC.cc b/libqpdf/QTC.cc index 21d240ba..1ca79c05 100644 --- a/libqpdf/QTC.cc +++ b/libqpdf/QTC.cc @@ -13,7 +13,7 @@ tc_active(char const* const scope) } void -QTC::TC(char const* const scope, char const* const ccase, int n) +QTC::TC_real(char const* const scope, char const* const ccase, int n) { static std::map active; auto is_active = active.find(scope); diff --git a/manual/installation.rst b/manual/installation.rst index e02380ee..08c49765 100644 --- a/manual/installation.rst +++ b/manual/installation.rst @@ -257,6 +257,16 @@ CHECK_SIZES that ensures an exact match between classes in ``sizes.cc`` and classes in the library's public API. This option requires Python 3. +ENABLE_QTC + This is off by default, except in maintainer mode. When off, + ``QTC::TC`` calls are compiled out by having ``QTC::TC`` be an empty + inline function. The underlying ``QTC::TC`` remains in the library, + so it is possible to build and package the qpdf library with + ``ENABLE_QTC`` turned off while still allowing developer code to use + ``QTC::TC`` if desired. If you are modifying qpdf code, it's a good + idea to have this on for more robust automated testing. Otherwise, + there's no reason to have it on. + GENERATE_AUTO_JOB Some qpdf source files are automatically generated from :file:`job.yml` and the CLI documentation. If you are adding new @@ -297,6 +307,8 @@ MAINTAINER_MODE - ``CHECK_SIZES`` + - ``ENABLE_QTC`` + - ``GENERATE_AUTO_JOB`` - ``WERROR`` diff --git a/manual/release-notes.rst b/manual/release-notes.rst index ebbfd4f5..ab2c1d8e 100644 --- a/manual/release-notes.rst +++ b/manual/release-notes.rst @@ -7,6 +7,12 @@ For a detailed list of changes, please see the file :file:`ChangeLog` in the source distribution. 11.0.0 + - Performance improvements + + - Many performance enhancements have been added. In developer + performance benchmarks, gains on the order of 20% have been + observed. + - Replacement of ``PointerHolder`` with ``std::shared_ptr`` - The qpdf-specific ``PointerHolder`` smart pointer implementation @@ -231,6 +237,14 @@ For a detailed list of changes, please see the file - The qpdf source code is now formatted automatically with ``clang-format``. See :ref:`code-formatting` for information. + - Test coverage with ``QTC`` is enabled during development but + compiled out of distributed qpdf binaries by default. This + results in a significant performance improvement, especially on + Windows. ``QTC::TC`` is still available in the library and is + still usable by end user code even though calls to it made + internally by the library are turned off. Internally, there is + some additional caching to reduce the overhead of repeatedly + reading environment variables at runtime. 10.6.3: March 8, 2022 - Announcement of upcoming change: diff --git a/run-qtest b/run-qtest index 1160589d..481052e9 100755 --- a/run-qtest +++ b/run-qtest @@ -13,6 +13,7 @@ my $code = undef; my @bin = (); my $color = undef; my $show_on_failure = 0; +my $disable_tc = 0; my @tc = (); if ($^O =~ m/^MSWin32|msys$/) @@ -51,6 +52,10 @@ while (@ARGV) usage() unless @ARGV; $show_on_failure = cmake_bool(shift(@ARGV)); } + elsif ($arg eq '--disable-tc') + { + $disable_tc = 1; + } elsif ($arg eq '--tc') { usage() unless @ARGV; @@ -94,7 +99,7 @@ push(@cmd, "-datadir", "$code/qtest", "-junit-suffix", basename($code)); -if (scalar(@tc)) +if (scalar(@tc) && (! $disable_tc)) { my @tc_srcs = map { File::Spec->abs2rel(abs_path($_))