Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazily recompile PureLive models and lazily create models for nucache #6347

Merged
merged 45 commits into from
Sep 18, 2019

Conversation

Shazwazza
Copy link
Contributor

@Shazwazza Shazwazza commented Sep 16, 2019

Purposes of this PR:

  1. Change the perceived performance of saving a document type (or other schema items like a Data Type) to be nearly 'instant'.
  2. Fix package installation/uninstallation to execute in a single transaction/scope
  3. Fix package installation/uninstallation to not recompile pure live models dozens of times ... which causes all sorts of other odd issues

Perceived perf of saving doc type

The concern behind this is that in v8 when you save schema items and you are running in PureLive mode, the models must be rebuilt in order to update the nucache structure with the new models and currently all of this is done in the request thread that was used during the saving of a document type. This means that in some cases admins might be waiting for a much longer period than they anticipated when saving the document type.

In v7, the perceived performance doesn't affect saving a document type because in v7, the models and cache updates occur when a front-end request is executed after a schema item has changed. So the performance 'hit' occurs on a front-end request, not a back office request.

The changes in this PR make v8 behave like v7. Changes to nucache mean that the strongly typed models are created lazily when needed (i.e. on the front-end) and when these models are created is when the pure live models are recompiled - if they are flagged to do so. When saving schema items in the back office, pure live models are not recompiled because there is nothing in those back office requests requiring strongly typed front-end models.

We will need to change how this works in 8.3 when there is an embedded version of MB, currently in this PR we are using reflection to set the flag to recompile models (i.e. ResetModels). In 8.3 we will change to not use reflection.

single trans/scope for package install/uninstall

This is an easy one, we just create an outer scope for when we install/uninstall the data from a package. Previous to this change there was multiple transactions occuring for data installation of a package which means if there was any errors, things wouldn't be rolling back as expected. It also meant that cache refresher notifications were being executed during package install instead at the end of the scope.

package install/uninstall and MB Purelive recompiles

With the 1st change above, this no longer occurs

Other Notable changes

  • In some previous PR/merge, the unit tests and benchmark projects were excluded from the sln in debug mode from building, this regression has been fixed
  • Moves all reserved path detection and determining if it's a front-end request to a new class RoutableDocumentFilter ... Let me know if you hate this name. This is for a couple reasons:
    • This fixes v8: Fix up GlobalSettingsExtensions.IsReservedPathOrUrl #4646
    • the reserved path logic used to be in static methods and it was all ugly,
    • now all of this logic is in one place, in a class without ugly statics
    • The way that route detection worked was totally broken if new routes were added dynamically at runtime
  • Mistakenly on startup with PureLive enabled we were forcing models to always regenerate from scratch which is bad for startup performance. MB already uses a caching system to use cached models on startup if they existed. This was simply an oversight, and a simple code change on startup from publishedModelFactory.WithSafeLiveFactory(LoadCachesOnStartup); to LoadCachesOnStartup(); means we can use MBs model caches on startup if they exist. My tests show at least a 3 second improvement in startup time with a shallow boot (i.e. bumping web.config) when models already exist in PureLive mode.
  • There was a nasty bug in nucache that is fixed - nucache operates on snapshots which are tracked with a generation number. When content or schema changes, the gen is increased and a new snapshot is created. When a request begins, it should be pinned to a specific gen, however this wasn't happening and calls to GetRequiredLinkedNode, GetParentLink, etc... were returning the very latest gen even if the current request wasn't pinned to it. This resulted in race conditions and panic exceptions when changing schema items while trying to render front-end pages.
  • I've disabled the ModelBindingExceptionFilter unless running in PureLive since that is the only reason this filter exists and I've added the ability to disable the filter via appSettings for testing. After much testing and debate - it is near impossible to prevent model binding errors in PureLive and doing so would require an immense effort for very little gain, instead when this occurs we just refresh automatically and everything is fine, the end user will not see errors.

Testing

Note: I have tested the changes made to the RoutableDocumentFilter.ReservedPaths (since this was moved from UmbracoModule.ReservedPaths). This collection deals with Identity providers and I have tested using a Google identity provider for the back office and have verified that this collection is populated correctly and is correctly used in the changes in RoutableDocumentFilter.IsReservedPathOrUrl

The test of the testing is:

  • Code review
  • Running in PureLive mode, go save a document type, and then refresh a front-end page probably within a couple seconds. You should see that the document type saves quite quickly AND that the tree is refreshed quite quickly (this proves that the back office requests are not blocked) and you'll see that the front-end page will take some time to render (most likely on a few seconds). You can also try saving a document type and then refreshing front-end pages on multiple tabs, you should see that all of these tabs will be waiting for the background operation to complete.
  • Run the same test when not using PureLive, the front-end requests will not block and should be more or less instant.
  • Install Umbraco from scratch with the Starter Kit, ensure there are no exceptions and everything works
  • Uninstall the package, ensure there are no exceptions and everything works
  • Install the Starter Kit from the back office, ensure there are no exceptions and everything works

Shazwazza and others added 30 commits September 9, 2019 23:32
…e can block until the background notifier is completed
…quest, required caching the check for a front-end request which required moving the check to it's own class.
… they are not there or if the hashes don't match so let MB use it's cached models if it can.
Lazily create content and models in NuCache
@Shazwazza Shazwazza changed the title temp - changes to nucache and model regeneration Lazily recompile PureLive models and lazily create models for nucache Sep 17, 2019
@Shazwazza Shazwazza marked this pull request as ready for review September 17, 2019 03:17
@bergmania
Copy link
Member

Hi @Shazwazza.

Doing tests, My application sometimes hit the breakpoint in ThrowModelBindingException when opening multiple tabs that waits for the background operation to complete.
6347-hits-breakpoint

Also I got this exception when I tried to reload the website (/) after saving the Home doc-type (Starter kit):
image

@bergmania
Copy link
Member

I can't make a fresh install. I get this error:

I just make a default install using SqlCE:
image

@Shazwazza
Copy link
Contributor Author

@bergmania I've fixed the issue with

Failed to get sibling with Id

... it was a particularly nasty bug and I'm glad we caught it

I've updated my notes in the task and have removed the testing notes about the model binder exception. I've changed a few things in the Model binder filter but the fact is that we cannot prevent these model binding errors from occurring because razor compilation is a global thing and it's triggered lazily when models need to be recompiled, which are triggered lazily when content needs to be fetched and no matter what, we can never prevent one request from fetching a content model while another thread forces views to recompile before the first thread renders it's views - which now means the first thread is trying to bind a stale model to a view that requires newer models. There is one way to deal with it but would require a huge amount of work with tracking if models are reset and then tracking all active requests, then blocking all requests once there are no active requests until views and models are recompiled which would be real nasty. We'll let the model binder exception filter just handle this case. This is only an issue with PureLive and changing schema types but the end-user won't see it, so we'll just live with it.

I'm just figuring out the issue with the installer now.

…a trans, fixes nucache changes to not pass around _liveGen since sometimes we want the latest that is not _liveGen
@Shazwazza
Copy link
Contributor Author

@bergmania I've fixed the installer issue now too, was a silly IEnumerable lazy iteration issue i introduced. I think all test should now pass with latest changes.

@warrenbuckley
Copy link
Contributor

If @bergmania has code reviewed I have re-tested with the notes above with SK install/uninstall & reinstall without no issues.

Same for the notes about ModelsBuilder & saving Doctypes.
Blocks frontend request as opposed to backend when in PureLive mode & no problem for backend or frontend when switched to Dll mode

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v8: Fix up GlobalSettingsExtensions.IsReservedPathOrUrl
3 participants