1

Closed

View files should not be required when deploying a precompiled application

description

If you precompile your MVC4 website, you can remove all your views.
The views are just turned into markers (a holdover from webforms), but they are not used at all. You can simply delete and not worry about deploying anything from /views or /areas.

In MVC5 that does not appear to be the case. It appears the FIRST thing the view engine location does is to check to see if the file exists in one of the prescribed locations. That is a change and is not required.

Why look for a file to make sure it is present when you will in fact never even use that file. There is also internal 'caching' inside the view engine code to help speed this up as it does not want to check every time.

None of that is necessary when running a precompiled application. Can we get the MVC4 behavior back?

1) You could try and load the precompiled class for the view before checking for the file on disk. Perhaps this would be expensive for those not using precompilation?
2) There is a 'PrecompiledApp.config' file that is in the root of the website. In the case when this file is present, you would know not to look for files. This is a required file for distribution and contains contents like <precompiledApp version="2" updatable="false"/>

Thoughts?
Closed May 1 at 11:50 PM by trdai
The bug is fixed and verified. No performance regression was found in extensive performance tests.

comments

yishaigalatzer wrote Feb 23 at 7:16 PM

Thanks @WayneBrantley for reporting this. We weren't aware of a regression here, it will be helpful if you can share your repro project so we make sure we address your specific scenario.

WayneBrantley wrote Feb 24 at 12:41 AM

I actually do not have a repo project. File|New mvc project and then precompile - should be able to remove views in deployment. No special setup required.

I have MVC4 sites that I do this with today.....in trial deployments of upgrading to MVC5 I ran into this. Really appreciate you looking into this!

pranavkm wrote Apr 21 at 9:53 PM

Fixed in changeset 56a9d42334448c04b8366b113a9f3b985f87ea77

WayneBrantley wrote Apr 21 at 10:40 PM

Your implementation is not the best performer.
When precompiled non-updatable, the view file will not be present and if it it was should not be used.

Your code first uses the FileExistanceCache which will look for the file on disk. However, you already know if precompiled non-updatable, so no need to look there (which does a file exists).

So, this code should look more like this, which saves a file exist access check among other things.

if (IsPrecompiledNonUpdateableSite)
return BuildManager.FileExists(virtualPath);
return _fileExistsCache.FileExists(virtualPath);

I added a note at your checked in code too.
https://github.com/ASP-NET-MVC/aspnetwebstack/commit/56a9d42334448c04b8366b113a9f3b985f87ea77

What do you think?

pranavkm wrote Apr 22 at 1:01 AM

Unless you specifcially delete them, aren't the stub views located on disk? In which case, it's actually quicker to ask the VPP \ FileExistenceCache rather than the BuildManager.

WayneBrantley wrote Apr 22 at 3:05 AM

It is common practice to not distribute any thing in /views or /areas folder. By default the asp.net precompiler does leave the marker files around - because that code has not been changed in 10 years! However, I do not deploy exactly what the precompiler generates, but instead only deploy what is needed so I never have any marker files.

Of course, the asp.net precompiler should not create all those marker files - and I asked them to fix that, they of course suggested to just delete them as they do not want to change that code. I would think most people with any sort of automated deployment would not deploy those marker files. (why copy around 100's of files that are not even used).

In the situation where people precompile - I would think they would not deploy the files and you code is going to force a file check each time and then check with BuildManager, making it slower...

The actual best solution would be if the _fileExistsCache could look for the file on disk and/or via the build manager (based on a constructor parameter represeting IsPrecompiledNonUpdateableSite ) and cache the result of that. This would result in caching of build manager results (which you say are slower than a file check, so that would be important!) Any possibility or thoughts around that?

yishaigalatzer wrote Apr 22 at 4:07 AM

Wayne,

I would say this is now a theoretical discussion. There is no supporting evidence if this change actually makes a measurable difference (or a significant one at least). This current design as based on that assumption that there is no significant perf impact here, and we will look at our perf tests to validate that. Checking for file on disk is actually not slow as things typically are cached by the OS anyways. We don't want to add a caching layer just because we think things are slow. Many a times this proves to slow things down or have unexpected effects.

If you believe that this impacts your scenarios in a significant way, I'd recommend performing a perf test and sharing the results with us. We will gladly consider it as a follow up to this fix.

WayneBrantley wrote Apr 23 at 12:35 AM

Well, checking for the existence of a file we know does not exist makes no sense.

kamehrot wrote Apr 28 at 6:42 PM

Assigning to Troy who is verifying this.

yishaigalatzer wrote May 1 at 7:03 PM

Wayne,

We run an extensive perf test, and we don't see any difference between all the scenarios. We compared the 4.0, 5.0 (with views on the server), and 5.2 with and without views. The results are identical (less than 1% variation).

We try not to make perf code changes based on Gut feel and just use measurements to drive the decisions. Perf improvements are typically a source of regressions in edge case scenarios, and hence we are going to keep the code as is at this point.

Like I mentioned above, if you want to run your own perf test and show a case where there is a significant degradation we will be taking another look at it. The code is available as nightly so feel free to give it as a spin at your convenience, note that we plan to lock down the 5.2 release coding pretty soon.

We really appreciate your report and sticking with us in this process!

WayneBrantley wrote May 1 at 11:15 PM

Thank you guys for fixing this issue.