Skip to content

Commit e522abc

Browse files
horenmarhorenmar
committed
Avoid quadratic JSON array parse behaviour in catch_discover_tests
Using CMake's `string(JSON` to parse JSON array leads to quadratic running time in number of tests, see https://gitlab.kitware.com/cmake/cmake/-/work_items/27985 This leads to _terrible_ runtime for `catch_discover_tests` when called on binaries with lot of tests (1k+). To get reasonable runtimes, we have to avoid using `string(JSON` to parse out the individual test objects from the array with all tests. This commit replaces the sane approach of using real JSON parser with a set of terrible hacks, where we use CMake's string APIs to split the JSON array on what looks like object boundary (`}<ws>*,<ws>*{`), and then checking whether the resulting thing can be parsed as JSON object. If not, we append the next piece and check again. And again, and again, until we get a proper JSON object. This is all around a hilariously terrible idea, however: 1) It works in practice for all tested inputs. 2) It improves the time it takes to run `catch_discover_tests` on binary with 1k tests from 4.2s to 1.1s and 2k tests from 16s to 3.9s.
1 parent 60c8b87 commit e522abc

4 files changed

Lines changed: 372 additions & 15 deletions

File tree

‎extras/CatchAddTests.cmake‎

Lines changed: 150 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,142 @@
11
# Distributed under the OSI-approved BSD 3-Clause License. See accompanying
22
# file Copyright.txt or https://cmake.org/licensing for details.
33

4+
# Because natively using CMake's JSON processing for arrays leads to quadratic
5+
# running times, we do terrible hack and split JSON array by object boundary
6+
# + commas and try to reconstruct valid JSON objects. During this, we need
7+
# to replace CMake characters that could be in the test name/tags with
8+
# placeholder, so it doesn't affect CMake's processing of the strings/lists
9+
# we create during the parsing.
10+
#
11+
# We use 0x01, 0x02, 0x03, and 0x04 as placeholder bytes, as those cannot
12+
# exist in JSON unescaped.
13+
#
14+
# 0x01 <=> ';' (CMake list separator)
15+
# 0x02 == element boundary marker used while splitting the tests array
16+
# 0x03 <-> '[' (opens a CMake bracket-argument context)
17+
# 0x04 <-> ']' (closes a CMake bracket-argument context)
18+
#
19+
function(get_placeholder_bytes semicolon_var boundary_var open_bracket_var close_bracket_var)
20+
string(ASCII 1 semicolon)
21+
string(ASCII 2 boundary)
22+
string(ASCII 3 open_bracket)
23+
string(ASCII 4 close_bracket)
24+
set(${semicolon_var} "${semicolon}" PARENT_SCOPE)
25+
set(${boundary_var} "${boundary}" PARENT_SCOPE)
26+
set(${open_bracket_var} "${open_bracket}" PARENT_SCOPE)
27+
set(${close_bracket_var} "${close_bracket}" PARENT_SCOPE)
28+
endfunction()
29+
30+
31+
# Placeholder bytes in the listing would break our parsing hack, so
32+
# we check they don't exist. They shouldn't exist in valid JSON, but
33+
# the reporter might not be escaping them properly.
34+
function(validate_input_noescapes listing_var)
35+
get_placeholder_bytes(semicolon boundary open_bracket close_bracket)
36+
foreach(byte "${semicolon}" "${boundary}" "${open_bracket}" "${close_bracket}")
37+
string(FIND "${${listing_var}}" "${byte}" found)
38+
if(NOT found EQUAL -1)
39+
message(FATAL_ERROR
40+
"The test listing contains raw control byte (0x01-0x04) which should not "
41+
"be there. This means either bad escaping in JSON reporter, or corrupted file. "
42+
)
43+
endif()
44+
endforeach()
45+
endfunction()
46+
47+
48+
# Replaces relevant characters with their placeholders, see `get_placeholder_bytes`
49+
# Modifies argument `var` in place.
50+
function(magic_escape_chars var)
51+
get_placeholder_bytes(semicolon boundary open_bracket close_bracket)
52+
set(value "${${var}}")
53+
string(REPLACE ";" "${semicolon}" value "${value}")
54+
string(REPLACE "[" "${open_bracket}" value "${value}")
55+
string(REPLACE "]" "${close_bracket}" value "${value}")
56+
set(${var} "${value}" PARENT_SCOPE)
57+
endfunction()
58+
59+
60+
# Turns placeholders back into original characters, see `get_placeholder_bytes`
61+
# Modifies argument `var` in place.
62+
function(magic_unescape_chars var)
63+
get_placeholder_bytes(semicolon boundary open_bracket close_bracket)
64+
set(value "${${var}}")
65+
string(REPLACE "${semicolon}" ";" value "${value}")
66+
string(REPLACE "${open_bracket}" "[" value "${value}")
67+
string(REPLACE "${close_bracket}" "]" value "${value}")
68+
set(${var} "${value}" PARENT_SCOPE)
69+
endfunction()
70+
71+
72+
# Abuses knowledge of Catch2's JSON reporter output for listing tests to
73+
# split JSON array of the test listings into a CMake list of strings,
74+
# with each element being the JSON string of one array entry.
75+
#
76+
# This avoids the terrible quadratic running time of using CMake's JSON
77+
# support to parse the JSON reporter output "properly", where the whole
78+
# JSON array of tests is parsed again for every element. Instead, we can
79+
# use the CMake's API to only parse the individual test's objects, which
80+
# are usually small and only have to be reparsed fixed number of times
81+
# (once for test names, once for labels).
82+
#
83+
# We process the string representing the JSON array by splitting it on
84+
# `}<ws>*,<ws>*{` and then checking for each chunk whether it is a valid
85+
# JSON object representing Catch2's test. If not (e.g. because we split
86+
# on the presence of `}<ws>*,<ws>*{` inside a test name), then we append
87+
# the next chunk to the current one and check again. And again, until
88+
# we get back to a valid JSON.
89+
#
90+
# Note that to support passing the object strings back from the function,
91+
# they will still contain the placeholders from `get_placeholder_bytes`
92+
# and need to be unescaped before further processing (e.g. sending them
93+
# into CMake's JSON parsing API).
94+
function(split_json_array json_array_var out_var)
95+
# We have to pass the input by var name to avoid CMake processing
96+
# the input as an arg.
97+
set(json_in "${${json_array_var}}")
98+
99+
# Strip the array brackets at the start and end of the JSON array.
100+
# Must happen before we escape the other [] instances below from the
101+
# actual array data.
102+
string(REGEX REPLACE "^[ \t\r\n]*\\[" "" json_in "${json_in}")
103+
string(REGEX REPLACE "\\][ \t\r\n]*$" "" json_in "${json_in}")
104+
105+
magic_escape_chars(json_in)
106+
107+
# We need to keep the whitespace around comma around, so that if we
108+
# split inside the test object, we can reconstruct it losslessly.
109+
get_placeholder_bytes(_semicolon _boundary_marker _open_bracket _close_bracket)
110+
string(REGEX REPLACE "(}[ \t\r\n]*)[,]([ \t\r\n]*{)" "\\1${_boundary_marker}\\2" json_in "${json_in}")
111+
112+
# We escaped all list separators above, so now we can turn the JSON
113+
# string into a CMake list of fragments in single pass.
114+
string(REPLACE "${_boundary_marker}" ";" fragments "${json_in}")
115+
116+
# And now we have to reconstruct the actual JSON structure from fragments.
117+
set(array_elements "")
118+
set(accumulator "")
119+
foreach(next_fragment IN LISTS fragments)
120+
if(accumulator)
121+
set(accumulator "${accumulator},${next_fragment}")
122+
else()
123+
set(accumulator "${next_fragment}")
124+
endif()
125+
126+
# Because the fragments (might) contain invalid JSON characters due
127+
# to escaping, we have to unescape it before checking if we can parse it.
128+
set(maybe_json "${accumulator}")
129+
magic_unescape_chars(maybe_json)
130+
string(JSON unused ERROR_VARIABLE err GET "${maybe_json}" "name")
131+
if(err STREQUAL "NOTFOUND")
132+
list(APPEND array_elements "${accumulator}")
133+
set(accumulator "")
134+
endif()
135+
endforeach()
136+
137+
set(${out_var} "${array_elements}" PARENT_SCOPE)
138+
endfunction()
139+
4140
# TBD: Further possible optimization is that most arguments for per-test
5141
# `prepare_command` call are constant across one invocation of
6142
# `catch_discover_tests`, and thus need checking and escaping only
@@ -66,7 +202,6 @@ function(make_temp_file_path OUT_VARIABLE FALLBACK_PATH)
66202
endfunction()
67203

68204
function(catch_discover_tests_impl)
69-
70205
cmake_parse_arguments(
71206
""
72207
""
@@ -147,9 +282,10 @@ function(catch_discover_tests_impl)
147282
)
148283
endif()
149284

150-
# Read the JSON output back from the output file (and then get rid of the file)
285+
# Read the JSON output back from the output file and validate it.
151286
file(READ ${listing_output_path} listing_output)
152287
file(REMOVE ${listing_output_path})
288+
validate_input_noescapes(listing_output)
153289

154290
# Prepare reporter
155291
if(reporter)
@@ -205,36 +341,35 @@ function(catch_discover_tests_impl)
205341
message(FATAL_ERROR "Unsupported catch output version: '${version}'")
206342
endif()
207343

208-
# Speed-up reparsing by cutting away unneeded parts of JSON.
344+
# Extract just the JSON array with tests and then split them into
345+
# individual objects.
209346
string(JSON test_listing GET "${listing_output}" "listings" "tests")
210-
string(JSON num_tests LENGTH "${test_listing}")
347+
split_json_array(test_listing tests)
211348

212349
# Exit early if no tests are detected
213-
if(num_tests STREQUAL "0")
350+
if(NOT tests)
214351
file(WRITE "${_CTEST_FILE}" "")
215352
return()
216353
endif()
217354

218-
# CMake's foreach-RANGE is inclusive, so we have to subtract 1
219-
math(EXPR num_tests "${num_tests} - 1")
220-
221-
foreach(idx RANGE ${num_tests})
222-
string(LENGTH "${script}" script_len)
355+
# Each element in the tests is JSON-string representing one test object.
356+
# We have to parse it and then turn it into CTest script commands.
357+
foreach(single_test IN LISTS tests)
223358
# Because appending to the same string in CMake has quadratic runtime,
224359
# we flush the script into the file periodically to avoid the worst case.
360+
string(LENGTH "${script}" script_len)
225361
if (script_len GREATER _WriteToFileThreshold)
226362
file(APPEND "${_CTEST_FILE}" "${script}")
227363
set(script "")
228364
endif()
229365

230-
366+
# The elements are still escaped and contain JSON-invalid characters,
367+
# they have to be unescaped before parsing them as JSON.
368+
magic_unescape_chars(single_test)
231369
if(add_tags)
232-
string(JSON single_test GET "${test_listing}" ${idx})
233370
string(JSON test_tags GET "${single_test}" "tags")
234-
string(JSON plain_name GET "${single_test}" "name")
235-
else()
236-
string(JSON plain_name GET "${test_listing}" ${idx} "name")
237371
endif()
372+
string(JSON plain_name GET "${single_test}" "name")
238373

239374
# Escape characters in test case names that would be parsed by Catch2
240375
# Note that the \ escaping must happen FIRST! Do not change the order.

‎tests/CMakeLists.txt‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -632,6 +632,13 @@ if(CATCH_ENABLE_CMAKE_HELPER_TESTS)
632632
"-DCATCH_ADD_TESTS_SCRIPT=${CATCH_DIR}/extras/CatchAddTests.cmake"
633633
-P "${CMAKE_CURRENT_LIST_DIR}/TestScripts/DiscoverTests/TestPrepareCommand.cmake"
634634
)
635+
636+
add_test(NAME "CMakeHelper::DecomposeJsonArray"
637+
COMMAND
638+
"${CMAKE_COMMAND}"
639+
"-DCATCH_ADD_TESTS_SCRIPT=${CATCH_DIR}/extras/CatchAddTests.cmake"
640+
-P "${CMAKE_CURRENT_LIST_DIR}/TestScripts/DiscoverTests/TestDecomposeJsonArray.cmake"
641+
)
635642
endif()
636643

637644
foreach(reporterName # "Automake" - the simple .trs format does not support any kind of comments/metadata

0 commit comments

Comments
 (0)