Skip to content

[4/4] Stop persisting current_loc (+2% parse throughput) - #919

Merged
jkeiser merged 1 commit into
masterfrom
jkeiser/move-current-loc
Jun 12, 2020
Merged

jkeiser merged 1 commit into
masterfrom
jkeiser/move-current-loc

Conversation

@jkeiser

@jkeiser jkeiser commented Jun 7, 2020

Copy link
Copy Markdown
Member

This change makes it more likely the compiler will enregister the next tape write location (a very hot variable) by making it temporary to stage 2 (i.e. in structural_parser) instead of persisting it in dom_parser_implementation.

This is the last of a series of 4 PRs which simplifies a lot of stage 2 and results in a performance improvement at the end.

Performance

It looks like an overall average throughput improvement close to 2%. The improvements in instruction count and throughput are both pretty much universal. canada.json loses 6% or so overall, but other number parsing things gain substantially. Looking at the numbers, everything has a slightly larger number of cache references, and canada.json is large enough that almost all of them are cache misses, accounting for the difference. The increased number of cache references absolutely does not accord with my theory of why the patch is faster. I'll experiment with that, but I consider this loss acceptable (and recoverable as we delve into streaming in earnest).

File Old Cyc New Cyc +% Old Ins New Ins +%
mesh 275 257 7% 877 850 3%
marine_ik 273 258 5% 821 801 3%
random 146 142 3% 496 490 1%
tree-pretty 102 99 3% 346 341 2%
instruments 101 98 3% 352 348 1%
twitter 95 93 3% 307 299 2%
numbers 231 225 3% 725 713 2%
github_events 86 84 3% 278 273 2%
twitter_api_response 100 97 3% 318 310 3%
apache_builds 94 91 2% 319 316 1%
twitter_api_compact_response 124 121 2% 389 379 3%
mesh.pretty 166 162 2% 533 521 2%
citm_catalog 84 82 2% 299 302 -1%
update-center 117 116 1% 352 347 1%
twitter_timeline 134 132 1% 411 399 3%
twitterescaped 195 192 1% 560 556 1%
gsoc-2018 70 70 1% 178 177 1%
google_maps_api_response 111 111 1% 365 365 0%
google_maps_api_compact_response 206 206 0% 682 683 0%
repeat 113 114 -1% 364 364 0%
canada 263 279 -6% 916 903 1%

PR Notes:

  • This change doesn't have its awesome effect on master (it makes it worse). It doesn't get really good until [3/4] Remove unneeded structural_iterator variables #918--presumably because removing several variables from structural_iterator reduced reduced the number of decisions the compiler had to make about registers.
  • number_writer: After moving current_loc to the structural_parser, the number_writer couldn't get to it anymore.
    • I couldn't change the dom_parser_implementation &parser to structural_parser &parser, because number_writer gets passed to parse_large_integer and slow_parse_float, and that would deenregister the whole thing.
    • So I changed current_loc from uint32_t to uint64_t * and had number_writer store that. That single pointer is enough to write values to the right place and update the pointer.
    • Unfortunately, that wasn't enough: any attempt to pass next_tape_loc by reference or pointer caused it to de-enregister (reasonably enough). So I pass it by value, let it write the number out, and then bump next_tape_loc after calling either slow_parse_float or parse_large_integer.
  • I also changed number_writer to tape_writer and had structural_parser use it everywhere, so the write logic is largely localized.

@jkeiser
jkeiser force-pushed the jkeiser/move-current-loc branch 2 times, most recently from 1473439 to caf3879 Compare June 8, 2020 01:04
@jkeiser jkeiser changed the title Stop persisting current_loc (+2% parse throughput) [4/4] Stop persisting current_loc (+2% parse throughput) Jun 8, 2020
@jkeiser
jkeiser changed the base branch from master to jkeiser/remove-iterator-variables June 8, 2020 01:42
@jkeiser
jkeiser force-pushed the jkeiser/move-current-loc branch from caf3879 to e701896 Compare June 8, 2020 01:44
@jkeiser
jkeiser requested a review from lemire June 8, 2020 02:50
@jkeiser
jkeiser force-pushed the jkeiser/move-current-loc branch from e701896 to ba7aa09 Compare June 8, 2020 03:49
@jkeiser
jkeiser force-pushed the jkeiser/remove-iterator-variables branch from 7dfc8eb to d178e08 Compare June 8, 2020 22:22
@jkeiser
jkeiser force-pushed the jkeiser/move-current-loc branch from ba7aa09 to 755f2a4 Compare June 8, 2020 22:23
Comment thread src/generic/stage2/numberparsing.h
@lemire

lemire commented Jun 11, 2020

Copy link
Copy Markdown
Member

@jkeiser Let us discuss the unlikely inside an unlikely issue.

(Otherwise, this looks good to me.)

@jkeiser
jkeiser force-pushed the jkeiser/move-current-loc branch from 755f2a4 to bbd61eb Compare June 12, 2020 16:18
Base automatically changed from jkeiser/remove-iterator-variables to master June 12, 2020 16:52
@jkeiser
jkeiser merged commit 1febf2e into master Jun 12, 2020
@jkeiser
jkeiser deleted the jkeiser/move-current-loc branch June 12, 2020 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants