Skip to content
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

replace moose with class accessor in Logger::Entry #1042

Merged
merged 2 commits into from
Apr 7, 2022

Conversation

hannaeko
Copy link
Member

@hannaeko hannaeko commented Mar 3, 2022

Purpose

Remove Moose from Logger::Entry for better performance. The logger entries are objects that are instantiated a large number of time and which properties are accessed a lot during tests execution. In large batches having too much overhead in the methods involved can lead to performance issue. (I don't have the exact numbers anymore, those are things I tested last September but didn't get the chance to backport in mainline Zonemaster).

Context

Changes

Replace Moose with Class::Accessor in Logger::Entry

How to test this PR

  • Tests can be executed as usual: zonemaster-cli --level info zonemaster.net --show-module --show-testcase
  • JSON generation is not impacted: zonemaster-cli --level info zonemaster.net --json

@hannaeko hannaeko added this to the v2022.1 milestone Mar 3, 2022
@hannaeko hannaeko requested review from mattias-p, matsduf, a user and tgreenx March 3, 2022 11:40
mattias-p
mattias-p previously approved these changes Mar 3, 2022
Copy link
Member

@mattias-p mattias-p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. But I have a question.

lib/Zonemaster/Engine/Logger/Entry.pm Show resolved Hide resolved
@matsduf
Copy link
Contributor

matsduf commented Mar 25, 2022

@blacksponge, I am not a Moose fan, but if we replace Moose here with Class::Accessor should we then replace it with Class::Accessor everywhere? What do we win and what do we loose?

@hannaeko
Copy link
Member Author

hannaeko commented Apr 6, 2022

What do we win and what do we loose?

We remove some complex abstraction and it might improve performance in some case but we lose the abstraction / type checking and need to do that by hand if we still want it, meaning more boilerplate code.
Maybe moving away from Moose is a good idea, but I don't think it need to be a priority for the modules for which we will not see to substantial gain.

Copy link
Contributor

@matsduf matsduf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with the change. I approve.

@hannaeko hannaeko merged commit 796d6ef into zonemaster:develop Apr 7, 2022
@marc-vanderwal marc-vanderwal added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ReleaseTested Status: The PR has been successfully tested in release testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants