[7.x] Allow disabling checks for expired views #31206

browner12 · 2020-01-22T22:29:13Z

This is a resubmission of #31195 with some performance results.

I did my best to isolate this change, so we could see the performance impact specifically from this change, so I kept the request as simple as possible.

Route:

Route::get('/', 'MainController@index');

Controller:

class MainController
{
    public function index(Request $request)
    {
        return view ('parent');
    }
}

Parent View:

@for($a=1; $a<=50; $a++)
    @include('child');
@endfor

Child View:

dummy content

The execution savings are entirely dependent on how many views need to be rendered for a given request. In the example above it is set at 51 (1 parent + 50 children). I varied this number to see savings for different scenarios.

The following is the inclusive time of the isExpired() method:

View Count	Sample 1	Sample 2	Sample 3	Average
10	7.95 ms	7.34 ms	7.36 ms	7.55 ms
25	17 ms	20 ms	18.5	18.50 ms
50	35.5 ms	38.5 ms	33.3 ms	35.77 ms
100	73.6 ms	80.3 ms	62.8 ms	72.23 ms

The following are the total times of the request:

View Count	Sample 1	Sample 2	Sample 3	Average
10	410 ms	346 ms	331 ms	362 ms
25	343 ms	382 ms	375 ms	367 ms
50	398 ms	414 ms	387 ms	400 ms
100	478 ms	489 ms	408 ms	458 ms

Based on these numbers our average savings per request by implementing this feature are:

View Count	Inclusive Time	Total Time	Savings
10	7.55 ms	362 ms	2.08%
25	18.50 ms	367 ms	5.05%
50	35.77 ms	400 ms	8.95%
100	72.23 ms	458 ms	15.76%

So we see, as expected, the more views that need to be rendered, the bigger percentage it takes up of your request. Looking through some of our sites, it's not uncommon to see some pages with 40-50 views rendered.

Here is a screenshot of Blackfire comparing a 100 view run with the new feature disabled to with it enabled.

We can see the calls are identical, except we drop a large number of calls to isExpired() and the filesystem functions it calls.

add a new boolean on the `CompilerEngine` that lets us turn off checking if views are expired and recompiling them.

make sure we keep the behavior the same if the config variable doesn't exist

GrahamCampbell · 2020-01-23T08:17:39Z

What are the results without blackfire. Blackfire doesn't give an accurate view of the real improvements, because of the profiling overhead. Could you do a comparison just by timing the http requests?

browner12 · 2020-01-23T16:23:49Z

It's very difficult to get a good grasp of the impact this makes when comparing across multiple requests, because of the variance we see when just refreshing a simple page. For example, from my simple example above I can see variation from 400ms to 900ms by just refreshing the page without changing anything.

That's why I went the route of showing, on average, how much time the isExpired function uses per the number of views being loaded. The percentage of the total time this encompasses is going to vary wildly depending on all the other things you are doing in the request that add to the total time.

Therefore, the important thing to focus on is that if you're loading 50 views, on average you will take off 35ms from whatever your total request time would have been.

dwightwatson · 2020-03-06T02:39:42Z

Just had a go at running this in production (setting VIEW_CHECK_EXPIRATION=false and calling php artisan view:cache on deployment) but ran into issues.

production.ERROR: include(/home/forge/default/storage/framework/views/f96159782440dc8632475a608b1caf105cd54bba.php): failed to open stream: No such file or directory (View: /home/forge/default/releases/20200306015729/vendor/davejamesmiller/laravel-breadcrumbs/views/bootstrap3.blade.php)

It seems that view:cache only caches your app's views, not views served from packages. So when the app tries to fetch one of those views it simply fails.

I wonder if there is an easy way to include views from packages in the precompilation, or whether there should be a clear caveat in config/view.php or the documentation that this feature cannot be used in this scenario.

stayallive · 2020-03-06T08:20:59Z

I have the exact same issue but this time with Horizon:

include(/var/www/staging/storage/framework/views/737e90b7e3fd02fa774d19f2efa426527c54bb06.php): failed to open stream: No such file or directory (View: /var/www/staging/releases/20200305214528/vendor/laravel/horizon/resources/views/layout.blade.php)

I really like this feature but it looks like the php artisan view:cache never generated cached views for packages in the first place, not an issue with this PR but it is a side effect that is surfaced because the cache generation is effectively disabled here.

(I've opened up an issue #31789 to track this)

driesvints · 2020-03-06T13:39:12Z

We're reverting this PR for now: #31798

See #31789

We could maybe reconsider a pr in the future that takes the above bug in mind.

ksdcsk · 2020-03-06T15:15:47Z

I've encountered the same bug and did some research to try to fix it. In my case, it was related to Laravel telescope. My understanding of what was happening is:

Views are cached using their absolute (real) path (see code);
Telescope (or other service providers) registers its views using relative paths

$this->loadViewsFrom(
     __DIR__.'/../resources/views', 'telescope'
);

The path to views that is obtained using the FileViewFinder->find() method is not converted to an absolute path;
As views caching relies on the hash of the path of the view (see code), the hashes of the cached view and the fetched view are different;
This leads to the error reported here.

A way to fix this

This bug could be solved if hinted paths from the FileViewFinder class were resolved: see code.
Here's the patch:

Index: src/Illuminate/View/FileViewFinder.php
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/Illuminate/View/FileViewFinder.php	(revision eac5d68779cc7e9db91a9546ce93f6af605de51f)
+++ src/Illuminate/View/FileViewFinder.php	(revision 372448c1e2c4a4fd9ae65f60cbc7a828dfed645d)
@@ -192,7 +192,7 @@
      */
     public function addNamespace($namespace, $hints)
     {
-        $hints = (array) $hints;
+        $hints = array_map([$this, 'resolvePath'], (array) $hints);
 
         if (isset($this->hints[$namespace])) {
             $hints = array_merge($this->hints[$namespace], $hints);
@@ -210,7 +210,7 @@
      */
     public function prependNamespace($namespace, $hints)
     {
-        $hints = (array) $hints;
+        $hints = array_map([$this, 'resolvePath'], (array) $hints);
 
         if (isset($this->hints[$namespace])) {
             $hints = array_merge($hints, $this->hints[$namespace]);
@@ -228,7 +228,7 @@
      */
     public function replaceNamespace($namespace, $hints)
     {
-        $this->hints[$namespace] = (array) $hints;
+        $this->hints[$namespace] = array_map([$this, 'resolvePath'], (array) $hints);
     }
 
     /**

browner12 · 2020-03-06T15:24:49Z

well this sucks. sorry guys.

I'll do a little digging to see if we can make this play nicely with everything.

stayallive · 2020-03-09T15:05:38Z

As far as I can tell @ksdcsk is on the correct path and #31804 should have fixed the problems.

To provide some extra context / story:

The view:cache command uses the correct real path (source).

The FileViewFinder also resolved paths to their real path (source) for all view paths defined in the view.php file (source).

If we look at how Horizon registers it views (source -> source) but these namespaces are stored as hinst "as is" without resolving the real paths (source).

So this means that a Horizon view is cached as:

/private/var/www/app/vendor/laravel/horizon/src/../resources/views/layout.blade.php

instead of how the view:cache command stores it:

/private/var/www/app/vendor/laravel/horizon/resources/views/layout.blade.php

So either view:cache shouldn't use real paths or #31804 should be reconsidered so we can bring this feature back!

Krisell · 2020-04-06T07:18:53Z

I really like the idea of this PR. It does seem very unnecessary to run these checks in an environment where the files never change (of course assuming the performance difference is noticeable).

Would it make sense to only skip the modified check, and still always check for file existence? This would solve the issue here, and it would not crash if the cached files for any reason are removed. The big question is of course how this affects the performance gain, i.e. the relative weight of one exists() vs two lastModified().

browner12 added 2 commits January 21, 2020 19:39

allow skipping for expired views

7355d01

add a new boolean on the `CompilerEngine` that lets us turn off checking if views are expired and recompiling them.

pass true if config value doesn't exist

3ff195b

make sure we keep the behavior the same if the config variable doesn't exist

taylorotwell merged commit 3ff195b into laravel:master Jan 23, 2020

browner12 deleted the expired-compiled branch January 23, 2020 15:50

browner12 mentioned this pull request Jan 23, 2020

[7.x] add new check_compiled option laravel/laravel#5209

Merged

stayallive mentioned this pull request Mar 6, 2020

Artisan view:cache not caching vendor views #31789

Closed

driesvints mentioned this pull request Mar 6, 2020

[7.x] Revert disabling expired views checks #31798

Merged

ksdcsk mentioned this pull request Mar 6, 2020

[7.x] FileViewFinder: Resolve hinted paths #31804

Closed

Krisell mentioned this pull request Apr 5, 2020

[7.x] Remove mention of disabling view expiration check laravel/docs#5947

Merged

Jimbolino mentioned this pull request Aug 8, 2021

[8.x] Dont recompile view if lastModified is same as cache file #38270

Closed

browner12 mentioned this pull request Oct 21, 2022

[9.x] Allow disabling checks for expired views #44674

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[7.x] Allow disabling checks for expired views #31206

[7.x] Allow disabling checks for expired views #31206

Uh oh!

browner12 commented Jan 22, 2020

Uh oh!

GrahamCampbell commented Jan 23, 2020

Uh oh!

browner12 commented Jan 23, 2020

Uh oh!

dwightwatson commented Mar 6, 2020

Uh oh!

stayallive commented Mar 6, 2020 •

edited

Loading

Uh oh!

driesvints commented Mar 6, 2020

Uh oh!

ksdcsk commented Mar 6, 2020 •

edited

Loading

Uh oh!

browner12 commented Mar 6, 2020

Uh oh!

stayallive commented Mar 9, 2020 •

edited

Loading

Uh oh!

Krisell commented Apr 6, 2020 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

[7.x] Allow disabling checks for expired views #31206

[7.x] Allow disabling checks for expired views #31206

Uh oh!

Conversation

browner12 commented Jan 22, 2020

Uh oh!

GrahamCampbell commented Jan 23, 2020

Uh oh!

browner12 commented Jan 23, 2020

Uh oh!

dwightwatson commented Mar 6, 2020

Uh oh!

stayallive commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

driesvints commented Mar 6, 2020

Uh oh!

ksdcsk commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

A way to fix this

Uh oh!

browner12 commented Mar 6, 2020

Uh oh!

stayallive commented Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Krisell commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

stayallive commented Mar 6, 2020 •

edited

Loading

ksdcsk commented Mar 6, 2020 •

edited

Loading

stayallive commented Mar 9, 2020 •

edited

Loading

Krisell commented Apr 6, 2020 •

edited

Loading