Moo conversion #178
Moo conversion #178
Conversation
|
Would it be possible to rebase this down to one or a smaller handful of commits? |
|
Sure. Let me see what I can do. |
Moo is now included in one of the test files.
yanick
force-pushed
the
moo_conversion
branch
from
d335a03
to
135ab43
Compare
|
There we go. |
|
Please, don't touch the current Xslate with Moo. Fork some new Mooslate or such. Any (even small) speed drop is unacceptable. Moo--. |
|
@yanick I don't think your force push took effect on the server. I share the concerns about performance -- some benchmarking is essential to be sure that there isn't a change. I know Moo can be high-performance, but a few things need to be done to ensure that -- e.g. adding in XS accessors. |
| @@ -623,8 +623,8 @@ sub _compiler { | |||
| my $compiler = $self->{compiler}; | |||
|
|
|||
| if(!ref $compiler){ | |||
| require Mouse; | |||
| Mouse::load_class($compiler); | |||
| require Class::Load; | |||
There was a problem hiding this comment.
I would add Class::Load::XS to the list of optional dependencies -- e.g. via Dist::Zilla::Plugin::DynamicPrereqs require('Class::Load::XS') if not want_pp() and can_xs(); (or the equivalent unrolled code) in Makefile.PL.
|
Looks like there is some work to do to get appveyor to pass, as well. (missing prereq declarations?) It would also be nice to see this tested on travis. this is a good place to start as a template - https://github.com/p5sagit/JSON-MaybeXS/blob/master/.travis.yml |
What do you mean? Which server are we talking about? |
|
The github server - it doesn't appear that the squash took effect. |
|
#140 contains more discussions re benchmarking. As @shadowcat-mst said, we do not expect any performance degradation with Moo, so if there is any, it is something we can solve with the use of a profiler. |
Ah, no, it did. I just didn't squashed to a single commit. I kept the part of the history that I thought could be useful, as well as the distinction between my commits and Charlie's. |
|
ok. anyway, it's more important that we get this working with no performance degradation :) |
|
[performance] I'm conferencing as we speak, but as soon as I have some time I'll rerun the benchmarks and see what they reveal. |
|
Appveyor is passing, yay! \o/ |
* remove commented out default * removes trailing whitespaces * add Types::Standard as a dependency
yanick
force-pushed
the
moo_conversion
branch
from
a9d33d3
to
e7b3c6b
Compare
| @@ -23,6 +23,7 @@ my %args = ( | |||
| }, | |||
|
|
|||
| requires => { | |||
| 'Types::Standard' => '0', | |||
| 'Data::MessagePack' => '0.38', | |||
| 'Encode' => '2.26', | |||
| 'Mouse' => 'v2.5.0', | |||
There was a problem hiding this comment.
this prereq should also be changed (and also in cpanfile).
| @@ -3,7 +3,7 @@ use Moo 2.000001; | |||
|
|
|||
| use Text::Xslate::Util qw(p $DEBUG); | |||
|
|
|||
| use MooX::Types::MooseLike::Base qw(:all); | |||
| use MooX::Types::MooseLike::Base qw(:all); | |||
There was a problem hiding this comment.
Do we need this as well as Types::Standard?
|
Performance: so I looked at the benchmark folder, saw there is no direct way to compare different versions of the code, so I got lost in the yak forest and wrote this http://techblog.babyl.ca/entry/benchpress This being said, I ran benchmarks for |
|
more than two years later - what is the reason this hasn't been merged? |
|
Probably slipped between the cracks, or the 10% speed penalty is too high a price to pay. Might be interesting to replay the benchmarks now, as time and versions passed. |
Rebase of #140, with a few optimizations that was discussed in its comment threads.