Cache resolved Fix file path to improve include() performance.
#327
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- includeBenchmarkBase1: 3.2375 +/- 0.0193 - includeBenchmarkBase1000: 3.5835 +/- 0.1132 - includeBenchmarkBase100000: 5.4308 +/- 0.1477 - includeBenchmarkSome1: 3.6988 +/- 0.0185 - includeBenchmarkSome1000: 3.9415 +/- 0.2000 - includeBenchmarkSome100000: 7.8708 +/- 0.1882 - includeBenchmarkMany1: 5.1312 +/- 0.0764 - includeBenchmarkMany1000: 5.5572 +/- 0.2899 - includeBenchmarkMany100000: 17.6438 +/- 0.0740
- includeBenchmarkBase1: 3.3080 +/- 0.0558 - includeBenchmarkBase1000: 3.4018 +/- 0.0212 - includeBenchmarkBase100000: 5.4032 +/- 0.0494 - includeBenchmarkSome1: 3.7932 +/- 0.1135 - includeBenchmarkSome1000: 3.8172 +/- 0.0428 - includeBenchmarkSome100000: 6.1055 +/- 0.1947 - includeBenchmarkMany1: 5.2958 +/- 0.2262 - includeBenchmarkMany1000: 5.3335 +/- 0.3680 - includeBenchmarkMany100000: 7.7365 +/- 0.1504
|
Coming Alma metadata update ETL in lobid-resources will use this branch. Will check tomorow. |
dr0i
approved these changes
Oct 30, 2023
|
Tested on basedump: before this PR average (since a month) was 14h 12m. Now it's 13h 45m. So a bit faster 👍 . |
dr0i
added a commit
to hbz/lobid-resources
that referenced
this pull request
Oct 30, 2023
dr0i
added a commit
to hbz/lobid-resources
that referenced
this pull request
Oct 30, 2023
dr0i
added a commit
to hbz/lobid-resources
that referenced
this pull request
Oct 30, 2023
dr0i
added a commit
to hbz/lobid-resources
that referenced
this pull request
Oct 30, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #287.
Improves the performance of
include()instructions by avoiding costly operations for every record (attempting to construct a URL, hitting the file system). Instead, caches the resolved path to the included Fix file upon first encounter.Benchmark results based on simplistic "integration tests"¹ [runtime in seconds: lower is better; N=4]:
Base1Base1000Base100000Some1Some1000Some100000Many1Many1000Many100000Still has a noticeable overhead, though, so there's room for further improvement [runtime in seconds: lower is better; N=4]:
Base100000Many100000¹ NOTE: I intend to remove the "integration tests" after the pull request has been merged; in case we should ever need them again, we can easily restore them.
However, it doesn't seem to affect real-world workloads as much (Limetrans transformation: approx. 2m records; the second workflow utilizes roughly twice as many
include()s as the first one) [runtime in hours/minutes/seconds: lower is better; N=1]:@TobiasNx / @dr0i: You might want to evaluate the impact on lobid-resources as well.