Improve performance by removing dubious code #217
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
This commit removes code that has had a very long history and legacy. It survived over 10 years in the code base without much documentation of its purpose, either in code comments, documentation or commit messages. It’s likely that it might have been part of some debugging code paths that allow a programmer to retrieve access to objects touched by LDNS. It stores all of these objects within a few hashes. However, maintaining these hashes proves to be rather expensive. Disabling it speeds up simple runs of zonemaster-cli by roughly 3%, which is significant when Zonemaster is used to test millions of domains.
marc-vanderwal
added
the
V-Patch
marc-vanderwal
requested review from
mattias-p,
matsduf,
tgreenx and
MichaelTimbert
marc-vanderwal
force-pushed
the
improve-performance
branch
from
6d1e083 to
3ede58c
Compare
tgreenx
previously approved these changes
Jan 28, 2025
|
matsduf
reviewed
Jan 28, 2025
The unit test that this commit deletes tested the behavior of Zonemaster::LDNS under interpreter threads. But we never use interpreter threads in Zonemaster::Engine, so the test exercises a situation that will never occur in practice. We can therefore safely remove the file.
marc-vanderwal
force-pushed
the
improve-performance
branch
from
3ede58c to
5eaa9b7
Compare
|
The failing unit test run by Travis (which runs them with |
tgreenx
approved these changes
Jan 29, 2025
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.
Purpose
This PR improves the performance in Zonemaster::LDNS by removing undocumented code of dubious purpose.
The code in question has had a very long history and legacy. It survived over 10 years in the code base without much documentation of its purpose, either in code comments, documentation or commit messages. It is enabled only if the system Perl is compiled with support for interpreted threads, which introduces a difference in behavior that is system dependent.
The code might likely have been part of some debugging code paths so that a programmer could retrieve access to objects touched by LDNS. It stores all of these objects within a few hashes.
However, maintaining these hashes proves to be rather expensive. Disabling it speeds up simple runs of zonemaster-cli by roughly 3%, which is significant when Zonemaster is used to test millions of domains.
Context
Attempting to improve performance.
Changes
How to test this PR
Unit tests should pass.