switch from MooX::Types::MooseLike to Type::Tiny #1120

SysPete · 2016-01-29T16:25:36Z

Dumbbench perf script shows around 4% improvement in speed and I guess things will be even better for more complex routes.

SysPete · 2016-01-29T16:37:13Z

At the moment all types are exported so would be good to restrict imports in consumers.

SysPete · 2016-01-30T11:43:54Z

lib/Dancer2/Core/App.pm

@@ -36,7 +36,7 @@ sub supported_engines { [ qw<logger serializer session template> ] }

 has _factory => (
    is      => 'ro',
-    isa     => Object['Dancer2::Core::Factory'],


Type::Standard's Object type throws an exception when used like this since Object doesn't check isa.

What do you suggest instead?

This has been changed to InstanceOf['Dancer2::Core::Factory']

SysPete · 2016-01-30T14:56:03Z

There is still room for improvement since some of the Dancer2 types are not inlined but I'm happy with this PR as it stands right now.

What I don't like: Dancer2::Core::Types exports all types which is needed for backwards compatibility especially for session/plugin/... code outside core. Perhaps this could be addressed later with a deprecation warning and eventual switch to exporting nothing by default.

Once switched to Type::Tiny we can start to take advantage of coercions defined within Core::Types rather than having coercion defined in attribute of consuming class. Something to consider for later.

SysPete · 2016-01-30T17:30:40Z

perf.pl improvements I've seen are approx -1.8% for Type::Tiny (no XS) and -2.7% for Type::Tiny::XS

SysPete · 2016-02-01T18:07:48Z

Today I ran 20 runs of perf tests against all three on a different system from previous tests and am still seeing improvements though not so large: -1.11% for Type::Tiny (no XS) and -2.3% for Type::Tiny::XS. So definitely looks like a worthy change on a performance basis.

SysPete · 2016-02-01T18:11:53Z

Full details of most recent perf tests...

MooX::Types::MooseLike	Type::Tiny PP	Type::Tiny::XS	_
0.72398	0.7343	0.70167
0.7187	0.73416	0.69776
0.7199	0.70478	0.69788
0.72433	0.70282	0.70248
0.71206	0.71577	0.73264
0.73116	0.70075	0.7299
0.75646	0.71776	0.73168
0.71591	0.69227	0.695183
0.71188	0.73426	0.7086
0.71848	0.73423	0.69454
0.71926	0.7034	0.69914
0.7573	0.73233	0.69656
0.71776	0.69669	0.71317
0.70974	0.69479	0.69755
0.72264	0.72189	0.71073
0.72024	0.72551	0.72262
0.71847	0.69432	0.69107
0.71449	0.72344	0.69518
0.715289	0.7099	0.6933
0.70787	0.70228	0.69158
0.72179595	0.7137825	0.70516165	average
	1.11%	2.30%	improvement

shumphrey · 2016-02-08T12:15:53Z

This looks good. For my own sake, all my types are done with Type::Tiny family, so it would be good for D2 to be the same.

bigpresh · 2016-02-08T19:04:51Z

All looks pretty sane to me! I'm a little concerned that some more helpful exception messages, referring to the type that was required, are now more generic - for e.g. from the tests:

-    qr{\Q/.*/\E is not a HashRef},
+    qr{\Q/.*/\E.+did not pass type constraint},

However, it shouldn't be too difficult to find out what the constraint is. Some other stuff is clearer, though, and overall it looks cleaner and nicer, and your benchmarks show it to be faster too, which is always nice, so I'm in favour (especially if it also then allows the type-based captures discussed elsewhere!).

SysPete · 2016-02-08T20:22:14Z

@bigpresh Type::Tiny exception messages are often overly informative - check this out:

perl -e 'use Types::Standard qw/Int/;Int->("qq")'
Value "qq" did not pass type constraint "Int" at -e line 1
    "Int" is a subtype of "Num"
    "Num" is a subtype of "LaxNum"
    Value "qq" did not pass type constraint "LaxNum"
    "LaxNum" is defined as: (defined($_) && !ref($_) && Scalar::Util::looks_like_number($_))

so compared to MooX::Types::MooseLike exceptions you're really not losing anything.

SysPete · 2016-02-08T20:28:06Z

and there is more as referenced in an earlier comment...

isa => Object['Dancer2::Core::Factory'] with MooX::Types::MooseLike silently actually gives you:

isa => Object

whereas Type::Tiny will throw an exception in this case. More wins.

racke · 2016-02-09T07:00:01Z

@SysPete already convinced me about Type::Tiny in our Interchange projects, so 👍

SysPete · 2016-02-09T13:30:59Z

@bigpresh I've updated tests now so expected regexp includes the type name.

bigpresh · 2016-02-09T13:40:04Z

Ah, nice one, thanks for the responses - great to see that the errors are as helpful, or even more so, than they were before!

xsawyerx · 2016-03-28T20:31:54Z

@SysPete Great work! (as usual...) 👍 from me.

@bigpresh, the errors we will see for the type checks are for us, not for the user, and they should:

Be rare. They'll happen when we screw up, not the user. Our code should provide cleaner errors when a user does something wrong.
Be more meaningful to us.

SysPete · 2016-04-02T10:35:54Z

Looks like we have +1 * 2 - anyone have any objections? I'd really like to see this one merged (needed for #1127 for starters).

cromedome · 2016-04-02T13:11:27Z

+1. Do it :)

On Apr 2, 2016, at 5:35 AM, Peter Mottram notifications@github.com wrote:

Looks like we have +1 * 2 - anyone have any objections? I'd really like to see this one merged (needed for #1127 for starters).

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub

xsawyerx · 2016-04-03T09:41:43Z

Extra modules are mentioned in Dancer2::Manual::Migration and Dancer2::Manual. Could you add Types::Tiny::XS too?

SysPete · 2016-04-03T10:47:40Z

On 03/04/16 11:41, Sawyer X wrote:

Extra modules are mentioned in |Dancer2::Manual::Migration| and
|Dancer2::Manual|. Could you add |Types::Tiny::XS| too?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1120 (comment)

Done.

xsawyerx · 2016-04-03T12:08:03Z

Bring it on!

was using MooX::Types::MooseLike::Base directly.

- inline ReadableFilePath and WritableFilePath using quote_sub - use Enum to simplify Dancer2Method and Dancer2HTTPMethod - use InstanceOf for abbreviated class types for core dancer objects instead of blessed+ref

xsawyerx · 2016-05-26T10:21:57Z

Merged, thanks! 👍

[ BUG FIXES ] * GH #1165, #1167: Copy is_behind_proxy attribute into new request on forward. (Russell Jenkins) [ ENHANCEMENTS ] * GH #1120: Move from MooX::Types::MooseLike to Type::Tiny for performance. (Peter Mottram) * GH #1145, #1164: Replace Class::Load with Module::Runtime (Sawyer X) * GH #1159, #1163: Make template keyword global. (Sawyer X, Russell Jenkins) [ DOCUMENTATION ] * GH #1158: List both static and shared modules in Apache's deploy instructions. (Varadinsky)

@SysPete

[ BUG FIXES ] * Fix memory leak in plugins. (Sawyer X) * GH #1180, #1220: Revert (most of) GH #1120. Change back to using MooX::Types::MooseLike until issues around Type::Tiny are resolved. Peter (@SysPete) Mottram * GH #1192: Decode body|query|request_parameters (Peter Mottram) * GH #1224: Plugins defined with :PluginKeyword attribute are now exported. (Yanick Champoux) * GH #1226: Plugins can now call the DSL of the app via $self->dsl (Sawyer X) [ ENHANCEMENTS ] * PR #1223: Add YAML::XS to Recommends (Peter Mottram) * PR #1117: If installed, use HTTP::XSCookies and all cookie operations will be faster (Peter Mottram) * PR #1228: Allow register_plugin() to pass @_ properly (Sawyer X) * PR #1231: Plugins can now call the syntax of plugins they loaded (Sawyer X) [ DOCUMENTATION ] * PR #1151: Note that config is immutable after first read (Peter Mottram) * PR #1222: Update list of files generated by `dancer2 -a`, make name of sample app consistent (Daniel Perrett)

SysPete mentioned this pull request Jan 29, 2016

switch to Type::Tiny #1101

Closed

SysPete reviewed Jan 30, 2016
View reviewed changes

SysPete mentioned this pull request Feb 7, 2016

route parameters with type constraints #1127

Closed

xsawyerx added the Enhancement label Mar 28, 2016

SysPete mentioned this pull request Mar 29, 2016

Convert named parameters to named captures in routes #1126

Closed

SysPete added 7 commits May 10, 2016 18:08

switch from MooX::Types::MooseLike to Type::Tiny

248b858

update prereqs for Type::Tiny (with optional ::XS)

5022a62

Switch tp Dancer2::Core::Types in Response::Delayed

d41812b

was using MooX::Types::MooseLike::Base directly.

fixup test errors due to different exception message with Type::Tiny

9ca8c44

simplify and speedup some types

f623e5b

- inline ReadableFilePath and WritableFilePath using quote_sub - use Enum to simplify Dancer2Method and Dancer2HTTPMethod - use InstanceOf for abbreviated class types for core dancer objects instead of blessed+ref

add explicit dep on Sub::Quote

1d3317d

include type in expected regexp in types.t

8baa84d

add Type::Tiny::XS to Manual and Manual::Migration

88f9ef3

xsawyerx closed this May 26, 2016

SysPete deleted the feature/type_tiny branch May 26, 2016 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch from MooX::Types::MooseLike to Type::Tiny #1120

switch from MooX::Types::MooseLike to Type::Tiny #1120

SysPete commented Jan 29, 2016

SysPete commented Jan 29, 2016

SysPete Jan 30, 2016

xsawyerx Mar 28, 2016

SysPete Mar 29, 2016

SysPete commented Jan 30, 2016

SysPete commented Jan 30, 2016

SysPete commented Feb 1, 2016

SysPete commented Feb 1, 2016

shumphrey commented Feb 8, 2016

bigpresh commented Feb 8, 2016

SysPete commented Feb 8, 2016

SysPete commented Feb 8, 2016

racke commented Feb 9, 2016

SysPete commented Feb 9, 2016

bigpresh commented Feb 9, 2016

xsawyerx commented Mar 28, 2016

SysPete commented Apr 2, 2016

cromedome commented Apr 2, 2016

xsawyerx commented Apr 3, 2016

SysPete commented Apr 3, 2016

xsawyerx commented Apr 3, 2016

xsawyerx commented May 26, 2016

switch from MooX::Types::MooseLike to Type::Tiny #1120

switch from MooX::Types::MooseLike to Type::Tiny #1120

Conversation

SysPete commented Jan 29, 2016

SysPete commented Jan 29, 2016

SysPete Jan 30, 2016

Choose a reason for hiding this comment

xsawyerx Mar 28, 2016

Choose a reason for hiding this comment

SysPete Mar 29, 2016

Choose a reason for hiding this comment

SysPete commented Jan 30, 2016

SysPete commented Jan 30, 2016

SysPete commented Feb 1, 2016

SysPete commented Feb 1, 2016

shumphrey commented Feb 8, 2016

bigpresh commented Feb 8, 2016

SysPete commented Feb 8, 2016

SysPete commented Feb 8, 2016

racke commented Feb 9, 2016

SysPete commented Feb 9, 2016

bigpresh commented Feb 9, 2016

xsawyerx commented Mar 28, 2016

SysPete commented Apr 2, 2016

cromedome commented Apr 2, 2016

xsawyerx commented Apr 3, 2016

SysPete commented Apr 3, 2016

xsawyerx commented Apr 3, 2016

xsawyerx commented May 26, 2016