Speed up whitespace skipping in the lexer (#5490)
* Speed up whitespace skipping in the lexer lexer::skip_whitespace() called get() for every whitespace byte, and get() checks the (almost always false, once past the first character) next_unget flag on every call. skip_whitespace() now reads its first character with get() (needed to honor a pending unget() left over from finishing the previous token, e.g. scan_number() always ungets the character that terminated the number) and every further whitespace character with a new get_ignoring_pending_unget() variant that skips that branch, since nothing in the loop calls unget(). This is a narrower fix than the full contiguous-buffer bulk-skip suggested in the issue (scan a run of whitespace directly in the adapter's buffer and update position counters once per run). That approach depends on bulk-scan adapter infrastructure (supports_bulk_scan/bulk_data()/bulk_skip()) introduced by the open, unmerged parser-performance PR #5283, which this change intentionally does not depend on or replicate. Building new bulk-scan adapter infrastructure from scratch was judged out of scope/riskier than warranted here, so this change is limited to the safe, always-correct improvement of removing redundant per-character bookkeeping from the existing byte-at-a-time loop; full bulk-skipping is left as future work once #5283 (or equivalent adapter support) lands. Line/column/byte-offset bookkeeping is untouched and verified bit-for-bit identical before and after this change, including for pretty-printed (dump(4)) input with embedded newlines. Fixes #5412 Stacked on top of the PR for #5411 (branch issue-5411-lexer-skip-conversion). Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Fix codegen regression in skip_whitespace() from #5490 Benchmarking found the get()/get_ignoring_pending_unget() split in skip_whitespace() made long whitespace runs (e.g. indentation in pretty-printed JSON) 1.75x-3.2x SLOWER instead of faster, reproducible with both Apple Clang and GCC. Root cause: rewriting the loop from a plain do-while into an initial get() followed by a while-loop defeated the compiler's ability to keep the input adapter's read/end pointers in registers across iterations; both compilers instead reloaded them from memory on every character. The function split itself was not the problem (it still fully inlines); the loop's control-flow shape was. The fix keeps the same two-function structure but restores a do-while shape (guarded by an if for the "first char not whitespace" case), which lets both compilers hoist the pointers back into registers, matching or beating pre-#5490 performance. Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Share the position-counter bump between get() and get_ignoring_pending_unget() Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Extract current_is_whitespace() to deduplicate skip_whitespace()'s two whitespace checks Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Use a raw string literal for the multi-line error-position test input Signed-off-by: Niels Lohmann <mail@nlohmann.me> * Fix clang-tidy raw-string-literal finding and guard a new test against JSON_NOEXCEPTION The issue #5412 whitespace-skipping test added a check_error() helper that relies on catching json::parse_error to verify the exception message; under JSON_NOEXCEPTION, JSON_THROW aborts instead of throwing, which crashed ci_test_noexceptions (and cascaded into the other ci_cmake_options jobs). Guard the whole section with #if !defined(JSON_NOEXCEPTION), matching the existing pattern used by sibling tests in this file. Also switch one escaped string literal to a raw string literal to satisfy clang-tidy's modernize-raw-string-literal check. Signed-off-by: Niels Lohmann <mail@nlohmann.me> --------- Signed-off-by: Niels Lohmann <mail@nlohmann.me>
This commit is contained in:
1 parent
faa35cc647
commit
6d86cc0f6b
3 files changed
+199
-8
No files matched your search
@@ -1446,8 +1446,7 @@ scan_number_done:
|
|||||||
*/
|
*/
|
||||||
char_int_type get()
|
char_int_type get()
|
||||||
{
|
{
|
||||||
++position.chars_read_total;
|
advance_position();
|
||||||
++position.chars_read_current_line;
|
|
||||||
|
|
||||||
if (next_unget)
|
if (next_unget)
|
||||||
{
|
{
|
||||||
@@ -1459,6 +1458,23 @@ scan_number_done:
|
|||||||
current = ia.get_character();
|
current = ia.get_character();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
return track_after_read();
|
||||||
|
}
|
||||||
|
|
||||||
|
/// shared head of get() / get_ignoring_pending_unget(): bump the
|
||||||
|
/// per-character position counters (line-count-on-'\n' bookkeeping is
|
||||||
|
/// handled afterwards, in track_after_read(), once `current` is known)
|
||||||
|
void advance_position() noexcept
|
||||||
|
{
|
||||||
|
++position.chars_read_total;
|
||||||
|
++position.chars_read_current_line;
|
||||||
|
}
|
||||||
|
|
||||||
|
/// shared tail of get() / get_ignoring_pending_unget(): capture the
|
||||||
|
/// character for error messages (if needed) and update line/column
|
||||||
|
/// bookkeeping for the character now in `current`
|
||||||
|
char_int_type track_after_read()
|
||||||
|
{
|
||||||
// seekable adapters reconstruct the token lazily on error (see
|
// seekable adapters reconstruct the token lazily on error (see
|
||||||
// get_token_string), so the eager per-character copy is skipped
|
// get_token_string), so the eager per-character copy is skipped
|
||||||
capture_char(std::integral_constant<bool, lazy_token_string> {});
|
capture_char(std::integral_constant<bool, lazy_token_string> {});
|
||||||
@@ -1472,6 +1488,29 @@ scan_number_done:
|
|||||||
return current;
|
return current;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/*!
|
||||||
|
@brief like get(), but for call sites that can prove no unget() is pending
|
||||||
|
|
||||||
|
get() has to check the `next_unget` flag on every call, because a
|
||||||
|
previous token may have ended with unget() (e.g. scan_number() always
|
||||||
|
ungets the character that terminated the number, so the next call to
|
||||||
|
scan() can see it again). skip_whitespace() reads that first,
|
||||||
|
possibly-ungotten character via a plain get(), but every further
|
||||||
|
character it reads is guaranteed to be a fresh read: nothing between
|
||||||
|
those calls invokes unget(). This variant skips the (otherwise always
|
||||||
|
false) next_unget branch for those calls; it is not a general
|
||||||
|
replacement for get().
|
||||||
|
*/
|
||||||
|
char_int_type get_ignoring_pending_unget()
|
||||||
|
{
|
||||||
|
JSON_ASSERT(!next_unget);
|
||||||
|
|
||||||
|
advance_position();
|
||||||
|
current = ia.get_character();
|
||||||
|
|
||||||
|
return track_after_read();
|
||||||
|
}
|
||||||
|
|
||||||
/// seekable adapter: nothing to capture, the token is rebuilt on error
|
/// seekable adapter: nothing to capture, the token is rebuilt on error
|
||||||
void capture_char(std::true_type /*lazy*/) const noexcept {}
|
void capture_char(std::true_type /*lazy*/) const noexcept {}
|
||||||
|
|
||||||
@@ -1665,13 +1704,37 @@ scan_number_done:
|
|||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// whether `current` is one of the four JSON whitespace characters
|
||||||
|
bool current_is_whitespace() const noexcept
|
||||||
|
{
|
||||||
|
return current == ' ' || current == '\t' || current == '\n' || current == '\r';
|
||||||
|
}
|
||||||
|
|
||||||
void skip_whitespace()
|
void skip_whitespace()
|
||||||
{
|
{
|
||||||
|
// the first character may be a pending unget() left over from the
|
||||||
|
// previous token (see get_ignoring_pending_unget()); every
|
||||||
|
// subsequent character read by this loop is guaranteed fresh, since
|
||||||
|
// nothing below calls unget()
|
||||||
|
get();
|
||||||
|
|
||||||
|
if (!current_is_whitespace())
|
||||||
|
{
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
// this is written as an if-guarded do-while (rather than a plain
|
||||||
|
// while loop) because that shape is what lets both GCC and Clang
|
||||||
|
// keep the input adapter's read pointer in a register across
|
||||||
|
// iterations; the equivalent while-loop measurably defeated that
|
||||||
|
// optimization in testing, turning long whitespace runs (e.g. the
|
||||||
|
// indentation of pretty-printed JSON) from a register-only loop
|
||||||
|
// into one that reloads the pointer from memory every character
|
||||||
do
|
do
|
||||||
{
|
{
|
||||||
get();
|
get_ignoring_pending_unget();
|
||||||
}
|
}
|
||||||
while (current == ' ' || current == '\t' || current == '\n' || current == '\r');
|
while (current_is_whitespace());
|
||||||
}
|
}
|
||||||
|
|
||||||
token_type scan()
|
token_type scan()
|
||||||
|
|||||||
@@ -9229,8 +9229,7 @@ scan_number_done:
|
|||||||
*/
|
*/
|
||||||
char_int_type get()
|
char_int_type get()
|
||||||
{
|
{
|
||||||
++position.chars_read_total;
|
advance_position();
|
||||||
++position.chars_read_current_line;
|
|
||||||
|
|
||||||
if (next_unget)
|
if (next_unget)
|
||||||
{
|
{
|
||||||
@@ -9242,6 +9241,23 @@ scan_number_done:
|
|||||||
current = ia.get_character();
|
current = ia.get_character();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
return track_after_read();
|
||||||
|
}
|
||||||
|
|
||||||
|
/// shared head of get() / get_ignoring_pending_unget(): bump the
|
||||||
|
/// per-character position counters (line-count-on-'\n' bookkeeping is
|
||||||
|
/// handled afterwards, in track_after_read(), once `current` is known)
|
||||||
|
void advance_position() noexcept
|
||||||
|
{
|
||||||
|
++position.chars_read_total;
|
||||||
|
++position.chars_read_current_line;
|
||||||
|
}
|
||||||
|
|
||||||
|
/// shared tail of get() / get_ignoring_pending_unget(): capture the
|
||||||
|
/// character for error messages (if needed) and update line/column
|
||||||
|
/// bookkeeping for the character now in `current`
|
||||||
|
char_int_type track_after_read()
|
||||||
|
{
|
||||||
// seekable adapters reconstruct the token lazily on error (see
|
// seekable adapters reconstruct the token lazily on error (see
|
||||||
// get_token_string), so the eager per-character copy is skipped
|
// get_token_string), so the eager per-character copy is skipped
|
||||||
capture_char(std::integral_constant<bool, lazy_token_string> {});
|
capture_char(std::integral_constant<bool, lazy_token_string> {});
|
||||||
@@ -9255,6 +9271,29 @@ scan_number_done:
|
|||||||
return current;
|
return current;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/*!
|
||||||
|
@brief like get(), but for call sites that can prove no unget() is pending
|
||||||
|
|
||||||
|
get() has to check the `next_unget` flag on every call, because a
|
||||||
|
previous token may have ended with unget() (e.g. scan_number() always
|
||||||
|
ungets the character that terminated the number, so the next call to
|
||||||
|
scan() can see it again). skip_whitespace() reads that first,
|
||||||
|
possibly-ungotten character via a plain get(), but every further
|
||||||
|
character it reads is guaranteed to be a fresh read: nothing between
|
||||||
|
those calls invokes unget(). This variant skips the (otherwise always
|
||||||
|
false) next_unget branch for those calls; it is not a general
|
||||||
|
replacement for get().
|
||||||
|
*/
|
||||||
|
char_int_type get_ignoring_pending_unget()
|
||||||
|
{
|
||||||
|
JSON_ASSERT(!next_unget);
|
||||||
|
|
||||||
|
advance_position();
|
||||||
|
current = ia.get_character();
|
||||||
|
|
||||||
|
return track_after_read();
|
||||||
|
}
|
||||||
|
|
||||||
/// seekable adapter: nothing to capture, the token is rebuilt on error
|
/// seekable adapter: nothing to capture, the token is rebuilt on error
|
||||||
void capture_char(std::true_type /*lazy*/) const noexcept {}
|
void capture_char(std::true_type /*lazy*/) const noexcept {}
|
||||||
|
|
||||||
@@ -9448,13 +9487,37 @@ scan_number_done:
|
|||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// whether `current` is one of the four JSON whitespace characters
|
||||||
|
bool current_is_whitespace() const noexcept
|
||||||
|
{
|
||||||
|
return current == ' ' || current == '\t' || current == '\n' || current == '\r';
|
||||||
|
}
|
||||||
|
|
||||||
void skip_whitespace()
|
void skip_whitespace()
|
||||||
{
|
{
|
||||||
|
// the first character may be a pending unget() left over from the
|
||||||
|
// previous token (see get_ignoring_pending_unget()); every
|
||||||
|
// subsequent character read by this loop is guaranteed fresh, since
|
||||||
|
// nothing below calls unget()
|
||||||
|
get();
|
||||||
|
|
||||||
|
if (!current_is_whitespace())
|
||||||
|
{
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
// this is written as an if-guarded do-while (rather than a plain
|
||||||
|
// while loop) because that shape is what lets both GCC and Clang
|
||||||
|
// keep the input adapter's read pointer in a register across
|
||||||
|
// iterations; the equivalent while-loop measurably defeated that
|
||||||
|
// optimization in testing, turning long whitespace runs (e.g. the
|
||||||
|
// indentation of pretty-printed JSON) from a register-only loop
|
||||||
|
// into one that reloads the pointer from memory every character
|
||||||
do
|
do
|
||||||
{
|
{
|
||||||
get();
|
get_ignoring_pending_unget();
|
||||||
}
|
}
|
||||||
while (current == ' ' || current == '\t' || current == '\n' || current == '\r');
|
while (current_is_whitespace());
|
||||||
}
|
}
|
||||||
|
|
||||||
token_type scan()
|
token_type scan()
|
||||||
|
|||||||
@@ -1486,6 +1486,71 @@ TEST_CASE("parser class")
|
|||||||
CHECK(accept_helper("\"\\uD80C\\uFFFF\"") == false);
|
CHECK(accept_helper("\"\\uD80C\\uFFFF\"") == false);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#if !defined(JSON_NOEXCEPTION)
|
||||||
|
SECTION("issue #5412 - whitespace skipping bookkeeping (compact vs. pretty-printed)")
|
||||||
|
{
|
||||||
|
// lexer::skip_whitespace() reads its first character with get() (to
|
||||||
|
// honor a possibly pending unget() from the previous token) and every
|
||||||
|
// further whitespace character with get_ignoring_pending_unget() (a
|
||||||
|
// get() variant that skips the then-always-false next_unget check).
|
||||||
|
// This must not change the reported byte offset, line, or column of
|
||||||
|
// a syntax error, even when a long run of whitespace containing
|
||||||
|
// multiple newlines is skipped beforehand (as with pretty-printed
|
||||||
|
// input). The expected values below were captured from the
|
||||||
|
// unmodified do-while(get()) loop, so any regression that miscounts
|
||||||
|
// characters or newlines while skipping whitespace changes them.
|
||||||
|
const auto check_error = [](const std::string & input, std::size_t expected_byte,
|
||||||
|
const std::string & expected_what)
|
||||||
|
{
|
||||||
|
CAPTURE(input)
|
||||||
|
try
|
||||||
|
{
|
||||||
|
json _ = json::parse(input);
|
||||||
|
FAIL_CHECK("expected a parse_error, but parsing succeeded");
|
||||||
|
}
|
||||||
|
catch (const json::parse_error& e)
|
||||||
|
{
|
||||||
|
CHECK(e.byte == expected_byte);
|
||||||
|
CHECK(std::string(e.what()) == expected_what);
|
||||||
|
}
|
||||||
|
};
|
||||||
|
|
||||||
|
// a nested document, serialized both compactly and pretty-printed
|
||||||
|
// (dump(4)), each truncated right before the final closing '}' so
|
||||||
|
// that the parser hits EOF after skipping all of the (in the
|
||||||
|
// pretty-printed case, substantial) indentation whitespace
|
||||||
|
const json doc =
|
||||||
|
{
|
||||||
|
{"a", 1},
|
||||||
|
{"b", json::array({true, false, nullptr, "x"})},
|
||||||
|
{"c", json::object({{"d", 3.14}, {"e", json::array({1, 2, 3})}})}
|
||||||
|
};
|
||||||
|
|
||||||
|
const std::string compact = doc.dump();
|
||||||
|
const std::string pretty = doc.dump(4);
|
||||||
|
|
||||||
|
check_error(compact.substr(0, compact.size() - 1), 60,
|
||||||
|
"[json.exception.parse_error.101] parse error at line 1, column 60: syntax error while parsing object - unexpected end of input; expected '}'");
|
||||||
|
check_error(pretty.substr(0, pretty.size() - 1), 193,
|
||||||
|
"[json.exception.parse_error.101] parse error at line 17, column 1: syntax error while parsing object - unexpected end of input; expected '}'");
|
||||||
|
|
||||||
|
// an invalid token appearing after several indented, multi-line
|
||||||
|
// whitespace runs vs. the same document without any of that
|
||||||
|
// whitespace
|
||||||
|
check_error(R"({
|
||||||
|
"a": 1,
|
||||||
|
"b": [
|
||||||
|
true,
|
||||||
|
false
|
||||||
|
],
|
||||||
|
"c": @
|
||||||
|
})", 70,
|
||||||
|
"[json.exception.parse_error.101] parse error at line 7, column 10: syntax error while parsing value - invalid literal; last read: '\"c\": @'");
|
||||||
|
check_error(R"({"a":1,"b":[true,false],"c":@})", 29,
|
||||||
|
"[json.exception.parse_error.101] parse error at line 1, column 29: syntax error while parsing value - invalid literal; last read: '\"c\":@'");
|
||||||
|
}
|
||||||
|
#endif
|
||||||
|
|
||||||
SECTION("tests found by mutate++")
|
SECTION("tests found by mutate++")
|
||||||
{
|
{
|
||||||
// test case to make sure no comma precedes the first key
|
// test case to make sure no comma precedes the first key
|
||||||
|
|||||||
Reference in new issue
Block a user