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

Moo conversion #140

Open
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@itcharlie

itcharlie commented May 23, 2015

Mouse to Moo conversion changes for issue #94. Thanks to Yanick for the orientation and help.

@tokuhirom

This comment has been minimized.

Member

tokuhirom commented May 26, 2015

There are some failing tests...

@itcharlie

This comment has been minimized.

itcharlie commented May 26, 2015

We are good now. Thanks @tokuhirom and @yanick

@syohex

This comment has been minimized.

Contributor

syohex commented Aug 4, 2015

Benchmark(Moo vs Mouse)

I benchmark Moo version and Mouse version. Benchmark scripts are in benchmark directory.

Environment

  • Intel Core i7-2620M CPU @ 2.70GHz
  • Memory 8GB
  • Ubuntu 15.04
  • Perl 5.22.0
  • Mouse 2.4.3
  • Moo 2.000002

cascade.pl

Moo

        Rate MTEx   TX
MTEx  3258/s   -- -90%
TX   31508/s 867%   --

Mouse

        Rate MTEx   TX
MTEx  3229/s   -- -90%
TX   32288/s 900%   --

data_section.pl

Moo

            Rate  file/1  file/2 vpath/1 vpath/2
file/1  104052/s      --    -39%    -41%    -43%
file/2  170836/s     64%      --     -3%     -7%
vpath/1 175812/s     69%      3%      --     -4%
vpath/2 183794/s     77%      8%      5%      --

Mouse

            Rate  file/1  file/2 vpath/1 vpath/2
file/1  106455/s      --    -42%    -42%    -44%
file/2  183991/s     73%      --     -0%     -4%
vpath/1 184721/s     74%      0%      --     -3%
vpath/2 190934/s     79%      4%      3%      --

demo-mt.pl

Moo

In this benchmark, Xslate is about 5.8 times faster than Text::MicroTemplate.

Mouse

In this benchmark, Xslate is about 6.0 times faster than Text::MicroTemplate.

demo-tt.pl

Moo

In this benchmark, Xslate is about 91.5 times faster than Template-Tookit.

Mouse

In this benchmark, Xslate is about 90.9 times faster than Template-Tookit.

expr.pl

Moo

          Rate     mt  s///g xslate
mt      3622/s     --    -9%   -86%
s///g   3999/s    10%     --   -84%
xslate 25121/s   594%   528%     --

Mouse

          Rate     mt  s///g xslate
mt      3556/s     --    -6%   -87%
s///g   3794/s     7%     --   -86%
xslate 26613/s   648%   602%     --

expr_eq.el

Moo

          Rate     mt  s///g xslate
mt      3490/s     --   -11%   -85%
s///g   3929/s    13%     --   -83%
xslate 22755/s   552%   479%     --

Mouse

          Rate     mt  s///g xslate
mt      3479/s     --   -13%   -88%
s///g   3999/s    15%     --   -86%
xslate 28444/s   718%   611%     --

foo.pl

Moo

         Rate     mt     ht xslate
mt     1581/s     --   -44%   -83%
ht     2844/s    80%     --   -69%
xslate 9050/s   472%   218%     --p

Mouse

         Rate     mt     ht xslate
mt     1555/s     --   -45%   -83%
ht     2844/s    83%     --   -69%
xslate 9135/s   488%   221%     --

include.pl

Moo

          Rate     TT     MT     HT Xslate
TT       153/s     --   -93%   -95%   -99%
MT      2283/s  1395%     --   -32%   -82%
HT      3350/s  2093%    47%     --   -74%
Xslate 12922/s  8361%   466%   286%     --

Mouse

          Rate     TT     MT     HT Xslate
TT       155/s     --   -93%   -95%   -99%
MT      2349/s  1419%     --   -29%   -82%
HT      3319/s  2046%    41%     --   -75%
Xslate 13274/s  8484%   465%   300%     --

interpolate.pl

Moo

              Rate        TMT      s///g     xslate    sprintf xslate/raw
TMT         2791/s         --       -34%       -75%       -92%       -94%
s///g       4225/s        51%         --       -62%       -88%       -90%
xslate     11061/s       296%       162%         --       -67%       -75%
sprintf    33810/s      1112%       700%       206%         --       -22%
xslate/raw 43530/s      1460%       930%       294%        29%         --

Mouse

              Rate        TMT      s///g     xslate    sprintf xslate/raw
TMT         2791/s         --       -35%       -76%       -92%       -94%
s///g       4266/s        53%         --       -64%       -88%       -91%
xslate     11821/s       324%       177%         --       -65%       -74%
sprintf    34133/s      1123%       700%       189%         --       -25%
xslate/raw 45510/s      1531%       967%       285%        33%         --

json.pl

Moo

          Rate xslate   json
xslate 18335/s     --    -1%
json   18442/s     1%     --

Mouse

          Rate   json xslate
json   17568/s     --    -1%
xslate 17772/s     1%     --

x-poor-env.pl

Moo

          Rate     TT Xslate     HT     MT
TT     43.1/s     --    -5%   -54%   -79%
Xslate 45.4/s     5%     --   -52%   -77%
HT     94.2/s   119%   108%     --   -53%
MT      201/s   366%   343%   113%     --

Mouse

         Rate     TT Xslate     HT     MT
TT     43.1/s     --    -2%   -57%   -78%
Xslate 44.2/s     3%     --   -56%   -78%
HT      101/s   134%   128%     --   -49%
MT      199/s   361%   350%    97%     --

x-rich-env.pl

Moo

          Rate     TT     MT    HTP Xslate
TT      72.4/s     --   -83%   -97%   -99%
MT       435/s   502%     --   -81%   -96%
HTP     2283/s  3054%   424%     --   -78%
Xslate 10288/s 14114%  2263%   351%     --

Mouse

          Rate     TT     MT    HTP Xslate
TT      72.4/s     --   -83%   -97%   -99%
MT       415/s   473%     --   -82%   -96%
HTP     2311/s  3093%   457%     --   -79%
Xslate 11164/s 15325%  2591%   383%     --

Finally

Almost case, Mouse version got good result than Moo version.

@zdm

This comment has been minimized.

zdm commented Aug 5, 2015

Any plans to release this code?

@GeJ

This comment has been minimized.

GeJ commented Aug 5, 2015

As syohex showed, Mouse performs better in most benchmarks.
Hopefully, the maintainers will stick with Mouse.

@karenetheridge

This comment has been minimized.

karenetheridge commented Dec 4, 2015

@itcharlie would it be possible for you to rebase this PR against the latest master (and possibly squash some of those merge commits down)? I think some of the performance differences can be improved. It would be a shame for this work not to be merged, as Moo is preferred over Mouse these days.

@@ -212,12 +213,14 @@ sub getopt_spec { @Spec }
sub _build_getopt_spec {
my($self) = @_;

$DB::single = 1;

This comment has been minimized.

@karenetheridge

karenetheridge Dec 4, 2015

This line should be removed.

@yanick

This comment has been minimized.

yanick commented Dec 4, 2015

@karenetheridge @itcharlie I can do the rebase, if you want. Shouldn't be too hard.

@itcharlie

This comment has been minimized.

itcharlie commented Dec 4, 2015

Thanks Yanick,

@karenetheridge I don't think I will be able to sit down and work on this
right away. If Yanick can do this right away I suggest he work on this
instead of me.

best,

Charlie

Charlie Gonzalez
(E) itcharlie@gmail.com
Github Profile https://github.com/itcharlie

On Fri, Dec 4, 2015 at 2:08 PM, Yanick Champoux notifications@github.com
wrote:

@karenetheridge https://github.com/karenetheridge @itcharlie
https://github.com/itcharlie I can do the rebase, if you want.
Shouldn't be too hard.


Reply to this email directly or view it on GitHub
#140 (comment)
.

@genehack

This comment has been minimized.

genehack commented Dec 4, 2015

+1 for this changeset. Xslate is the only thing I routinely use that is dependent on Mouse and it would be extremely disappointing to see this work not accepted, especially given that the benchmark differences are pretty negligible.

@@ -212,12 +213,14 @@ sub getopt_spec { @Spec }
sub _build_getopt_spec {
my($self) = @_;

$DB::single = 1;

my @spec;
foreach my $attr($self->meta->get_all_attributes) {

This comment has been minimized.

@karenetheridge

karenetheridge Dec 4, 2015

aha, this would be a contributor to the slowdown -- this Moo class is being upgraded to Moose to get the list of attributes. (thanks @shadowcat_mst)

@yanick

This comment has been minimized.

yanick commented Dec 4, 2015

Rebase to master at https://github.com/yanick/p5-Text-Xslate/tree/moo_conversion

I also spotted that the tests are still using Mouse. I'm going to take care of that in the next few hours

@shadowcat-mst

This comment has been minimized.

shadowcat-mst commented Dec 4, 2015

Symbol.pm was a heavy user, solved via

sub clone {
    my $self = shift;
    return bless({ %$self },ref($self));
}

sub _dump_denote {
    my($self, $type, $parser) = @_;

    my $entity = exists $self->{$attr}
        ? $self->{$attr}
        : $parser->can('default_' . $type);

    require B;
    my $cvgv = B::svref_2object($entity)->GV;
    printf STDERR "%s: %s::%s (%s:%s)\n",
        $type,
        $cvgv->STASH->NAME, $cvgv->NAME,
        $self->id, $self->line,
    ;
}

I can only get a 1% benchmark difference because almost all the work in almost all the benchmarks is being done by ->render's XS code. Well, except for the poor-env test where Moo appears to be loading slightly faster but I'm not sure that's really a significant difference either.

The getopt stuff in Runner needs tweaking as well but that didn't show up at the top of my profile (MooX::Options would maybe solve this, or borrowing part of its implementation)

@shadowcat-mst

This comment has been minimized.

shadowcat-mst commented Dec 4, 2015

If somebody can propose a benchmark that shows a more-than-1% slowdown in a way I can actually proile, I wouldn't mind poking this with a stick. The Symbol thing was basically the only thing I managed to get a handle on from the benchmark scripts. It's friday evening though, so I may just be being stupid.

@yanick

This comment has been minimized.

yanick commented Dec 4, 2015

Switched types to be 'Types::Standard' in my branch, which should also speed up the Moo version.

@GeJ

This comment has been minimized.

GeJ commented Dec 4, 2015

-1 with this. Just because people can't stand to add one standalone module to their module list, they are willing to impose several new modules to others. @gfx already expressed that he was not willing to switch to Moo but would consider plain objects. Text::Xslate works fine with Mouse, and it doesn't need to be "fixed" with Moo.

Personally, I don't want to add Moo and its dependencies to my module set. Already did, and it cost me a week to revert because people knew better than me how I should handle warnings. Not anymore!

@shadowcat-mst

This comment has been minimized.

shadowcat-mst commented Dec 25, 2015

@GeJ If you don't want to write warning clean code, Moo 2.0 in March no longer fatalises warnings by default. If you'd like to learn how, strictures 2 now only fatalises warnings we can be reasonably sure won't cause forward compatibility issues.

Also, if you have a pre-2.0 Moo you can easily do -

use Moo;
use warnings NONFATAL => 'all';

or use Moo::Lax or ...

Plenty of options other than reversion and they're all basically a single perl -pi -e away :)

@GeJ

This comment has been minimized.

GeJ commented Dec 27, 2015

@shadowcat-mst I perfectly know that. That's the quick fix I implemented two years ago when I was asked to leave a Sunday family gathering by a pissed off boss who got harassed by an angry client who was asking why none of his orders were processed in a timely fashion.

When I went to the office on that day I realized that someone, somewhere thought that appending an undefined value to a string was such an offense to the programming Gods that it should warrant killing any piece of code that would attempt to do so. And that's my problem with Moo : I don't like such clear POLA violations and I can't trust people who commit them. And rather than doing a rollback, Moo users had to use a workaround for roughly a year.

Also, and aside any personal opinion I may have on Moo, the root of this PR is just for convenience. There is no technical merit in this. s/Mouse/Moo/g everywhere in the code is not enough, it is slower than the original. Looking back at the PR, it has been suggested to add even more extra modules to catch up, or worse : drop the Moo API and directly tap into the guts of the object to speed up things. Wow!

I will repeat myself : just because people don't want to add one standalone module, they are willing to impose their convoluted solution to others. Mouse works just fine. It may not have all the bells and whistles of Moose or Moo, but it is in my opinion the perfect example of a much simple solution that fits the needs of a given problem. If memory serves, when I removed every reference to Moo in my code, there were no Moo-only feature I used that couldn't be replaced by either a Mouse extension or half a dozen lines of code.

Finally, a careful reader might remember that a respected member of the Perl community qualified Mouse as "amazing code", adding "If you're going to use Mouse, use Mouse." So, can we please leave it like that and stop trying to fix a problem that doesn't exist.

Edit: added a few edits this and there to fix a few mistakes. Please forgive my possibly awful English as it is not my native language.

@zdm

This comment has been minimized.

zdm commented Dec 27, 2015

+1 for Moo. Because Moo is more widely used and benchmark shows, that is is not slower that Mouse (several percents are standard error).

@shadowcat-mst

This comment has been minimized.

shadowcat-mst commented Jan 4, 2016

And that's my problem with Moo : I don't like such clear POLA violations and I can't trust people who commit them.

It was already clearly documented. I'm sorry you missed it.

And rather than doing a rollback, Moo users had to use a workaround for roughly a year.

The first ever release of Moo fatalized warnings. The delay was to try and minimise the damage to downstream users who -liked- them and regarded them as an important part of their code's integrity. Your "rollback" would've been their "sudden backcompat breakage", and that wasn't acceptable to us.

@abraxxa

This comment has been minimized.

abraxxa commented Sep 7, 2017

Will this get merged and released?

@yanick yanick referenced this pull request Sep 7, 2017

Open

Moo conversion #178

@yanick

This comment has been minimized.

yanick commented Sep 7, 2017

I turned my own copy of this branch as a PR, considering it's freshly rebased and contain a few extra tweaks (as per the comments above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment