Migrate away from Moose

[mattias-p]

Purpose

This PR replaces the last remaining uses of Moose in Engine.

Context

Fixes #1318.

Changes

• In most cases Moose is replaced with the more light-weight Class::Accessor. But in some cases Class::Accessor doesn't offer much in the way of replacement. In those cases the Moose constructs are replaced with plain Perl.
• The dependency on Moose is removed.

How to test this PR

Unit tests should pass.

[marc-vanderwal]

Looks good to me so far. I hope this change will improve the engine’s performance somewhat.

[mattias-p]

I ran the unit tests ten times each on b1d6f90 (current develop) 4e0a578 (current 1318-demoose) and found that the minimum running time decreased by about 10% and the average running time decreased by about 9%.

[mattias-p]

I ran the unit tests once each for the same commits and found that the memory usage decreased by about 18%. ("Maximum resident set size" as reported by GNU time)

[mattias-p]

Also the number of transitive dependencies decrease by about 28% from 67 to 48. (According to cpanm --scandeps --self-contained)

[marc-vanderwal]

It’s a lot of repetition, it’s true.

Out of curiosity, I tried looking for a solution that involved some metaprogramming to get rid of the repetition. The following code seems to work (well, at least, unit tests pass):

BEGIN {
    # This block uses metaprogramming techniques to define a number of almost
    # identical methods that are passthrough functions to methods in
    # Zonemaster::LDNS::Packet to be called on the `packet` member of
    # instances of this class.

    no strict 'refs';

    # Accessors taking an optional argument, defaulting to an empty list.
    for my $m (qw(timestamp querytime id opcode rcode edns_version)) {
        *{$m} = sub {
            my ( $self, $value ) = @_;
            return $self->packet->$m( $value // () );
        };
    }

    # Accessors that do not take any arguments
    for my $m (qw(type string data
                  aa ra tc do
                  question answer authority additional
                  edns_size edns_rcode edns_z edns_data has_edns)) {
        *{$m} = sub {
            my ( $self ) = @_;
            return $self->packet->$m;
        }
    }
}

I know that touching the symbol table like this is very akin to dark magic in Perl and may scare away newcomers, so I’m also fine with keeping the code as it is, even with the repetition.

[mattias-p]

If the repeated contents was in any way error prone I believe I'd prefer the DRY version. But in this case I feel the repeated version is boring in a good way whereas the DRY version is slightly exciting in a not-so-good way.

Since you went to the effort to come up with an alternative implementation I tried to come up with a third solution. How do you like this?

sub timestamp    { my ( $self, $time )    = @_; return $self->packet->timestamp( $time       // () ); }
sub querytime    { my ( $self, $value )   = @_; return $self->packet->querytime( $value      // () ); }
sub id           { my ( $self, $id )      = @_; return $self->packet->id( $id                // () ); }
sub opcode       { my ( $self, $string )  = @_; return $self->packet->opcode( $string        // () ); }
sub rcode        { my ( $self, $string )  = @_; return $self->packet->rcode( $string         // () ); }
sub edns_version { my ( $self, $version ) = @_; return $self->packet->edns_version( $version // () ); }

sub type       { my ( $self ) = @_; return $self->packet->type; }
sub string     { my ( $self ) = @_; return $self->packet->string; }
sub data       { my ( $self ) = @_; return $self->packet->data; }
sub aa         { my ( $self ) = @_; return $self->packet->aa; }
sub do         { my ( $self ) = @_; return $self->packet->do; }
sub ra         { my ( $self ) = @_; return $self->packet->ra; }
sub tc         { my ( $self ) = @_; return $self->packet->tc; }
sub question   { my ( $self ) = @_; return $self->packet->question; }
sub authority  { my ( $self ) = @_; return $self->packet->authority; }
sub answer     { my ( $self ) = @_; return $self->packet->answer; }
sub additional { my ( $self ) = @_; return $self->packet->additional; }
sub edns_size  { my ( $self ) = @_; return $self->packet->edns_size; }
sub edns_rcode { my ( $self ) = @_; return $self->packet->edns_rcode; }
sub edns_data  { my ( $self ) = @_; return $self->packet->edns_data; }
sub edns_z     { my ( $self ) = @_; return $self->packet->edns_z; }
sub has_edns   { my ( $self ) = @_; return $self->packet->has_edns; }

[marc-vanderwal]

Well, you know the saying in Perl: TIMTOWTDI. I like the way this code is indented, and it’s the way I’ve been doing simple accessors like these in my personal projects.

So yeah, you have a point: there is little gain to be had in maintainability to try to make it DRY. That’s why I said I was fine with the non-DRY version.

[matsduf]

I think the more compressed alternative that you @mattias-p propose above is better because it is easier to read, and I prefer that to the more magic solution that you @marc-vanderwal suggests. The explicit solution is easier to read and follow.

[mattias-p]

Apparently I forgot to push the commit I made for this, but I pushed it now.

[mattias-p]

@matsduf, @tgreenx, @marc-vanderwal please review again.

[matsduf]

@mattias-p, you write "Unit tests should pass". Do you think there is enough unit test coverage for this change? This change is hard to review. Make sure that all unit tests work is probably the best test. Could more unit tests be added to verify the correctness of the change?

I think this should be merged soon so that we have a chance to let pass through tests that we do.

I am prepared to approve.

[marc-vanderwal]

@mattias-p, you write "Unit tests should pass". Do you think there is enough unit test coverage for this change? This change is hard to review. Make sure that all unit tests work is probably the best test. Could more unit tests be added to verify the correctness of the change?

I checked out the PR’s branch and ran the test suite through Devel::Cover. According to the report for the Zonemaster::Engine::Packet module (originally HTML, had to print to PDF to be able to upload it to Github), there are a handful of untested methods:

• querytime;
• id;
• opcode;
• data;
• edns_data;
• edns_z.

The querytime and data methods are only exercised when the Redis cache is in use. The edns_data method is used in Nameserver11 and edns_z is used in Nameserver12, but the code paths don’t seem to be covered. The remaining methods, id and opcode, are entirely unused.

[mattias-p]

I added a unit test for the mentioned methods without test coverage. I considered removing the unused id and opcode methods instead of adding tests for them, but they are documented and I didn't want to break the API. The new test's aren't very interesting, but I did find a nicer way to integrate Test::NoWarnings and it's nice to have a file where you can just add more unit tests for the Engine Packet module.