-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cache test level config in logger::entry #1044
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement. Just a thought, do we really need to clone the data even once? Can't we just pass a ref around?
I designed the Profile module interface to avoid leaking references to internal mutable data structures. The idea is that it'll make our lives easier. |
Won't this break the ability to switch between profiles in Backend? Maybe reset_config() could be called from the "run" methods in Zonemaster::Engine::Test or something? |
@blacksponge, has switching been tested? |
As the profile is only set in the child process this is not breaking profile switching. To be sure I tried to run two consecutive tests with different profile and it was correctly executed. |
I will re-review when the conflict has been resolved. |
463636e
to
55960b8
Compare
I have rebased on top of develop, please re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve.
Purpose
Reduce the amount of time used cloning the test levels config.
Each call to
Zonemaster::Engine::Profile->effective->get
clone the result if it is not a scalar. For the test levels, this means that there will be a lot of calls to clone a fairly big hash. Here are the profiling info for a single test measured withPERL5OPT=-d:NYTProf ./zonemaster-cli --level info zonemaster.net --show-module --show-testcase
.Context
Ported from https://gitlab.rd.nic.fr/zonemaster/zonemaster-engine/-/blob/nightly-ronde/lib/Zonemaster/Engine/Logger/Entry.pm
Changes
How to test this PR
Testing a domain should work as usual:
./zonemaster-cli --level info zonemaster.net --show-module --show-testcase