Nested helpers

[sri]

Bringing this up on the mailing-list in this early stage is a bit unusual, but the core team is currently divided on this feature. So maybe some feedback from the community would help us decide about its future.

    https://github.com/kraih/mojo/compare/nested_helpers

This branch contains a proposal for nested helpers, the idea is to allow you to organize your helpers into different namespaces.

  helper 'foo.bar' => sub {...};

The dot serves as namespace separator, and "foo" becomes a helper that returns a proxy object on which you can call all the nested helpers.

  $c->foo->bar(...)

These nested helpers work exactly like normal helpers with the only limitation that they will not be imported into templates, since begin/end blocks wouldn't work with them, but they can be called like <%= $c->foo->bar %>.

What do you think?

--

sebastian

[Stefan Adams]

On Fri, Aug 8, 2014 at 2:51 PM, sri <kra...@googlemail.com> wrote:

What do you think?

I am in favor but cannot articulate why.  :(  I like the idea of organization.  I also like features and flexibility.

Can you share some of what the division among the core devs is?

[sri]

Can you share some of what the division among the core devs is?

Half of us think it's useful, the other half thinks it isn't. No technical reasons, just matter of taste.

--

sebastian

[sri]

No technical reasons, just matter of taste.

We've just talked about this on IRC, and it might be that we all already have our own patterns for working around the issue of cluttering the currently flat helper namespace.

  # Redispatch to method in plugin class

  $c->helper_for_some_plugin(some_method_in_the_plugin_class => @args);

  # Very long helper names

  $c->this_is_a_very_specific_helper_name_so_there_are_no_conflicts(@args);

  # A helper returning an object (for things that don't need $c)

  $c->minion->enqueue(@args);

So, it may very well be that this is a real problem and people are just unwilling to change their ways. :)

--

sebastian

[Ben van Staveren]

Yes please.

I have a few projects going where helpers from plugins and the main app
are clashing, and it got solved by having some very strict naming
guidelines, but it makes things a bit messy. I'd much rather be able to
have a plugin register it's helpers under a namespace (or in the 'main'
app space) to keep things clear and have an easier time. It also makes
more sense from a code perspective (IMO) to see
'$c->auth->is_authenticated' than seeing '$c->is_authenticated' because
it sort of indicates to readers of the code that it's not a native
controller method.

If it's not too much of a pain in the you-know-what to implement I don't
see a problem there. Having the option to do this is great, and if
people don't want to use it that's up to them :D

[sri]

I'm afraid the nested_helpers proposal didn't get enough votes and had to be rejected. :(

--

sebastian

[Scott Wiersdorf]

On Saturday, August 9, 2014 6:25:05 AM UTC-6, sri wrote:

I'm afraid the nested_helpers proposal didn't get enough votes and had to be rejected. :(

I'm a late-comer to the party I realize, but I'm not in favor of the proposed solution either. It feels and looks like a hack, and is barely better than longer helper names. Lately I've been grouping my helpers into classes with their own namespaces, then creating an instance of the class and invoking it like an object. Something like this:

package My::Plugin::Frob;
use Mojo::Base 'Mojolicious::Plugin';

use Frob;  ## the helper class

has 'frob';  ## this plugin's attribute

sub register {
  my ($self, $app, $cfg) = @_;
  $self->frob(Frob->new($cfg));  ## create an instance and assign to the attribute
  $app->helper(frob => sub { $self->frob });  ## make the helper
  ...
}

then elsewhere, say, in a controller:

$c->frob->some_method(...);

If there's a cleaner way to do this with less boilerplate, I'd be in favor of that too, but this has worked very well for over a year.

Scott

[sri]

Lately I've been grouping my helpers into classes with their own namespaces, then creating an instance of the class and invoking it like an object.

Those are not helpers, since they don't get $c as the first argument.

--

sebastian 

[sri]

It feels and looks like a hack, and is barely better than longer helper names.

Also, which part do you consider a hack? The AUTOLOAD code is pretty much exactly what we use for helpers eslewhere already, and the "foo.bar" naming scheme we've used with the stash for a long time. In fact, yesterday i've added this note to the rendering guide.

     https://github.com/kraih/mojo/commit/ae70411796d164db00f7ece8d9d171698c6384ab

--

sebastian

[sri]

Here's also a more minimalistic implementation that only uses a private proxy class.

    https://github.com/kraih/mojo/compare/tiny_nested_helpers

--

sebastian

[sri]

Here's also a more minimalistic implementation that only uses a private proxy class.

    https://github.com/kraih/mojo/compare/tiny_nested_helpers

And some documentation to make everything more clear.

    https://github.com/kraih/mojo/commit/9ed50ab18b13d946fc9845a3e556d708cb989536

--

sebastian 

[sri]

    https://github.com/kraih/mojo/compare/tiny_nested_helpers

And this branch got the necessary votes, nested helpers are now in master. \o/

--

sebastian 

[sri]

And released to CPAN with Mojolicious 5.27. \o/

--

sebastian

[sri]

WARNING: This is a little big technical. :)

Funny thing, nested helpers actually have the potential to be a lot faster than normal helpers. You may be aware that helpers use AUTOLOAD, and are therefor quite a bit slower than normal method calls. Here's a little benchmark i did recently.

    https://gist.github.com/anonymous/0de87d1408f5e98707a1

As you can see, currently nested helpers basically cost two normal helper calls. Getting it that low actually took quite a bit of optimizing already. But now we do have the option of turning all nested helper calls into normal method calls with a patch like this.

   https://gist.github.com/anonymous/231abecbb6e09e5fb3f6

Changing our benchmark results to this.

    https://gist.github.com/anonymous/f93e4b96e0cf0082c1ef

So you could actually be using nested helpers as an optimization if you're using a lot of helpers. Only the first helper call (->foo) would use AUTOLOAD, the rest would be plain old method calls.

    my $foo = $c->foo;

    $foo->bar;

    $foo->baz;

Of course there's a downside to all of this too... the patch generates anonymous classes on demand, which may leak if you instantiate Mojolicious::Renderer objects dynamically and destroy them again while using nested helpers.

Thoughts?

--

sebastian

[Ben van Staveren]

[snippity]

I think with appropriate warnings in the documentation about potential
leaks in the case you described, it would be something I'd love to see.
I have one app that has a somewhat ridiculously high amount of helpers,
and not only would namespaced helpers be beneficial for cleaning up the
code a little, speeding up helper calls would benefit that app as well.
So that's my vote, put it in :)

[sri]

I think with appropriate warnings in the documentation about potential
leaks in the case you described, it would be something I'd love to see.

And that's where the problems start, how do you warn about a problem like this appropriately? Without making the implementation look like a hack, or confusing people which most likely have no idea how memory management works in Perl. (seriously... like 90% of the folks i meet have no clue)

--

sebastian

[sri]

And another problem comes to mind... classes get compiled once a helper gets called for the first time, at which point you can't add new nested helpers anymore... but application helpers may be called even during startup time.

   app->foo->bar;

--

sebastian

[Ben van Staveren]

Big bold text, flashing lights, loud noises?

All kidding aside, I don't know how to properly warn people in the
documentation and still make it look good :(

[Ben van Staveren]

Hmm and that would be a problem since a copious amount of helpers get
called in the startup of the app I mentioned.

The idea itself is good though, so it'd be a bummer not to have it, but
with problems like this cropping up I don't think it's worth putting the
time in to make it work - then again, I have no idea how much time that
would take or whether it's realistically possible. I'll change my "gimme
gimme" vote to a "would be nice to have at some point" :)

[sri]

> And another problem comes to mind... classes get compiled once a helper
> gets called for the first time, at which point you can't add new nested
> helpers anymore... but application helpers may be called even during
> startup time.
>
>     app->foo->bar;

Hmm and that would be a problem since a copious amount of helpers get
called in the startup of the app I mentioned.

Allright, scratch that, we can clear the cache when new helpers are added. :)

    https://gist.github.com/anonymous/36ac0cf73d3f17f7e6ba

So we are back to "only" the leak problem.

--

sebastian 

[sri]

So we are back to "only" the leak problem.

Considering a line like this in the helper section of the rendering guide.

    "Nested helpers are a lot faster than normal helpers, but because proxy objects use dynamically generated classes, their memory cannot be freed before the end of program execution."

Of course this might be misunderstood to mean "every $c->foo->bar call leaks". :S

--

sebastian

[sri]

So we are back to "only" the leak problem.

I have a branch that may prevent the leak now. :)

    https://github.com/kraih/mojo/compare/faster_helpers

This one is pretty radical though, it also doubles the performance of template helpers by 100%. Here are also a few before/after benchmarks. (summary: after is always *much* better)

    https://gist.github.com/anonymous/fc47b339b009924120a7

    https://gist.github.com/anonymous/7f271e5aa352c19bea6a

    https://gist.github.com/anonymous/b6a915ce905790195b65

    

--

sebastian 

[sri]

...doubles the performance of template helpers by 100%...

Got a little too excited there... xD

--

sebastian 

[sri]

    https://gist.github.com/anonymous/fc47b339b009924120a7

    https://gist.github.com/anonymous/7f271e5aa352c19bea6a

    https://gist.github.com/anonymous/b6a915ce905790195b65

And merged into master, 5.28 is going to be a really fast release. :)

    https://github.com/kraih/mojo/commit/d32baef1560b73c3211143c5be921ee36205cb71

--

sebastian 

[sri]

And one more change that can double the performance of all helpers in controllers.

    https://gist.github.com/anonymous/be16369a565b5dacd4d6

You will have to use the $c->helpers method though.

    https://github.com/kraih/mojo/compare/9b7c1e234cb2...676d4216e5c1

--

sebastian

[sri]

And one more change that can double the performance of all helpers in controllers.

Actually, if you have to call multiple helpers and can reuse the proxy object, things get really fricking fast. ;)

    https://gist.github.com/anonymous/cbe8fa3dabf6497ec7b9

--

sebastian 

[Stefan Adams]

On Aug 15, 2014 1:34 AM, "sri" <kra...@googlemail.com> wrote:
>
>     https://gist.github.com/anonymous/cbe8fa3dabf6497ec7b9

Wow! That's enormous! What a slight change in the application code for developers to make and what an enormous payback!!

I'm really looking forward to implementing this in my projects!

Thanks for pushing forward to get nested helpers in Mojolicious and to tweak the performance to such drastic improvement.

[sri]

Thanks for participating in the discussion, i'm really happy with the implementation now. Helpers were a bit of a scalability weak spot in my opinion, but no more! Maybe i should bring up more design decisions on the mailing list. :)

--

sebastian