Explicit require template engine to prevent warning

[huydx]

Implicit require engine seem to be a non thread-safe way

WARN: tilt autoloading 'tilt/erb' in a non thread-safe way; explicit require 'tilt/erb' suggested.
WARN: tilt autoloading 'tilt/haml' in a non thread-safe way; explicit require 'tilt/haml' suggested.

[runlevel5]

I don't think we should include haml. There are many people out there don't use it. We let user explicitly require if needed

[coveralls]

Changes Unknown when pulling 20d6ff6 on huydx:master into * on lotus:master*.

[runlevel5]

👍

[coveralls]

Changes Unknown when pulling c7685bd on huydx:master into * on lotus:master*.

[coveralls]

Changes Unknown when pulling 6f95a08 on huydx:master into * on lotus:master*.

[jodosha]

@huydx I don't think suppressing an eventual exception is a good idea. Suppose that we want to use HAML, but we haven't included it in the Gemfile. We want the application to stop at that point.

You make the assumption that the file extension corresponds to the library. This isn't a general rule of thumb. Please check Tilt README and you'll find soon that files with .coffee ext are handled by coffee-script library.

If we want to have a solid implementation for this we should introspect Tilt.default_mapping. It's a registry which holds those informations about exts, libs and engines.

[huydx]

@jodosha i agree that handling this behavior in all cases seems not to be a good way. We need warning to tell user to require the library explicitly.
However, warning display in the test case is not good either. I think at least we should require 'tilt/erb' and 'tilt/haml' in the test cases because the green test should define correct behavior, and warning display is not a correct behavior. How do you think about that?

[jodosha]

@huydx TL;DR This is only an annoying issue for the test suite. Let's leave things as they are.

Lotus loading model

When Lotus::View is used in standalone mode, we explicitly ask developers boot the framework in a thread-safe context.

When we load a Lotus application, the internal loader wraps all those operations with a Mutex.

In both the production scenarios above, adapters are autoloaded, but in a safe context. Tilt isn't aware of this sane way that we use to load it. Its vision of the world is limited to the autorequire, which is a worrying issue in case you don't design those mechanisms carefully.

Test suite

Why we don't explicitly require those adapters in tests? Because we don't need to. As mentioned above, Lotus has a safe way to load frameworks and applications.

At the same time, we want to assert the Tilt will keep to autoload those adapters. Leaving tests as they are, lets us to notice ASAP if something in Tilt has changed.

What to do

Because those warnings are only generated during the tests, I'm for leaving the things as they are.

An alternative would be to silence those warnings during for test fixtures. But this may hide other important messages that come from Tilt.

/cc @joneslee85

[huydx]

@jodosha as lotus loader already wrapped all loading progress in a mutex, i think it's ok to leave things as they are. As well as warning display exactly explicit what it should be, this issue should be closed.

[jodosha]

@joneslee85 This is all yours 💃

[lstrzebinczyk]

Also remember that 'require' is rather expensive operation and it's better to avoid it inside methods, especially rather often called initializer. I was going to prepare benchmarks for this, but I see it's not going to be merged anyway ; )

[runlevel5]

@killapl please feel free to bench this, I'd love to see the difference. And yes, this is likely to be merged after another round of review ;)

[jodosha]

@killapl @joneslee85 Please remember that Template#initialize is called n times only when the application is loaded. Where n is the number of templates in an application.

This doesn't affects the runtime.

[lstrzebinczyk]

Slowing down the startup is a legitimate concern. I will compile down a benchmark for this (hopefully) tomorrow.

[jodosha]

@killapl I agree, but it's less worrying of a runtime overhead, where you pay performances for each request.

Anyway, do you folks have news about this?

/cc @huydx @joneslee85

[lstrzebinczyk]

A quick benchmark results in:

20000 times without require  0.600000   0.080000   0.680000 ( 0.671526) 
20000 times with require     0.780000   0.060000   0.840000 ( 0.841235)

Which is around 25% slowdown, or in another words, 0.16s for 20k template loads. This is the case in ruby 2.0.0 and 2.1.3, problem I was thinking must have been relevant in older rubies.

I suppose this kind of performance regression is ok if it solves an actual problem.

Also, sorry for the delay, builders cut of my internet connection for a couple of days: (

EDIT

Mentioning @jodosha @joneslee85

[jodosha]

@killapl In other words a ~0.00008 sec perf penalty multiplied the number of templates for an application, each time the application starts.

Real world example. At work we have 572 templates, the cost of this would be 0.04576 sec each time the app starts (production, tests, console, etc..).

[lstrzebinczyk]

In one of my previous projects we used this method:

http://www.ruby-doc.org/stdlib-1.9.3/libdoc/pathname/rdoc/Pathname.html#method-i-mkpath

And we found out that using just

FileUtils.mkpath

is way faster beacuse of require. But this was millions of calls and 1.9.3 times, where #require was O(n) if I remember correctly.

Well, better checked then sorry: )

[jodosha]

@killapl I'm confused, what's the relationship between mkpath and this topic?

[lstrzebinczyk]

There is a require call in mkpath, just like this PR is adding.

[jodosha]

I think the solution here is to have devs to explicitly require the tilt engine during the loading of the code.