From 71fcc5303f3cbd2747016fa69d59ab95bfd6cc78 Mon Sep 17 00:00:00 2001 From: Richard Jackson Date: Sat, 26 Oct 2024 00:47:38 +0100 Subject: [PATCH 1/4] Update common-pitfalls.md Have done this for Umbraco 15 docs only. Can go back through earlier docs and copy/paste in the updates if the points are relevant there too. --- 15/umbraco-cms/reference/common-pitfalls.md | 82 ++++++++++----------- 1 file changed, 40 insertions(+), 42 deletions(-) diff --git a/15/umbraco-cms/reference/common-pitfalls.md b/15/umbraco-cms/reference/common-pitfalls.md index 041245eb8e1..bf728a7645b 100644 --- a/15/umbraco-cms/reference/common-pitfalls.md +++ b/15/umbraco-cms/reference/common-pitfalls.md @@ -4,15 +4,15 @@ description: "Information on common Pitfalls and Anti-Patterns in Umbraco" # Common Pitfalls & Anti-Patterns -This section highlights common pitfalls that developers often encounter. Some of the anti-patterns discussed here can lead to memory leaks, instability, or poor performance on your site. Be sure to read this section—it could save your site. +This section highlights common pitfalls that developers often encounter. Some of the anti-patterns discussed here can lead to memory leaks, instability, or poor performance on your site. Be sure to read this section — it could save your site. ## Usage of Singletons and Statics Generally speaking, if you are writing software these days you should be using Dependency Injection (DI) principles. If you do this, you probably are not using [Singletons](https://en.wikipedia.org/wiki/Singleton\_pattern) or [Statics](https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/static-classes-and-static-class-members), and for the most part you should not be. -Since Umbraco comes with dependency injection out of the box, there really is not any reason to use singletons or statics. It makes your code difficult to test and hard to manage. Furthermore, the APIs become leaky and you will end up with more problems than when you started. +Since Umbraco comes with dependency injection out of the box, there really is no reason to use singletons or statics. It makes your code difficult to test and hard to manage. Furthermore, the APIs become leaky and you will end up with more problems than when you started. -Dependency injection is available everywhere, and you can register your own services as well. Additionally, some resources are available through properties on certain base classes. For example, all Razor views that Umbraco creates expose an `UmbracoHelper` property you can access through `@Umbraco`. The other base classes that expose some things you might need like `UmbracoContext` are things like `SurfaceController`. Even here the services are initially gotten through DI, and you can inject further Umbraco and custom services that you might need. +Dependency injection is available everywhere, and you can register your own services as well. Additionally, some resources are available through properties on certain base classes. For example, all Razor views that Umbraco creates expose an `UmbracoHelper` property you can access through `@Umbraco`. The other base classes expose some things you might need like `UmbracoContext`, and things like `SurfaceController`. Even here the services are initially obtained through DI, and you can inject further Umbraco and custom services that you might need. For more information about consuming and registering your own dependencies have a look at the [Dependency Injection](using-ioc.md) documentation. @@ -36,9 +36,9 @@ public class ContactFormSurfaceController : SurfaceController [HttpPost] public IActionResult SubmitForm(ContactFormModel model) { - // All normal form processing logic is left out of this example for brevity - // You can access all of these because they are properties of the base class, - // if you need something else you can inject it in the constructor. + // All normal form processing logic is left out of this example for brevity. + // You can access all of these properties because they are properties of the base class. + // If you need something else you can inject it in the constructor. //Profiling logger using (ProfilingLogger.TraceDuration("Start", "stop")) @@ -76,14 +76,14 @@ public class BadApiController : Controller This practice can cause memory leaks along with inconsistent data results when using this `_umbracoHelper` instance. -It is important to understand the difference between an object that has a Request-based scope and an object that has a Singleton/Application-based. +It is important to understand the difference between an object that has a Request-based scope and an object that has a Singleton/Application-based scope. -* **Application scope**: If an object has a singleton/application scope, that means that this single object instance will exist for the lifetime of the application. The single instance will be shared by every thread that accesses it. Static variables will always be application lifespan. -* **Request scope**: The web world is made up of requests and each request has its own thread. When an object is in the scope of a Request it only survives as long as the web request survives. At the end of the web request, it may either be disposed of or cleared from memory by the garbage collector. Request scoped object instances are not accessed by every other thread in the application unless you do something like the above. +* **Application scope**: If an object has a singleton/application scope, that single object instance will exist for the lifetime of the application. The single instance will be shared by every thread that accesses it. Static variables will always exist for the lifespan of the application. +* **Request scope**: The web world is made up of requests and each request has its own thread. When an object is in the scope of a Request it only survives as long as the web request survives. At the end of the web request, the object may either be disposed of or cleared from memory by the garbage collector. Request scoped object instances are not accessed by every other thread in the application unless you do something like the above. -An example of a request-scoped instance is the `HttpContext`. This object exists for a single request and it cannot be shared between other threads. Especially not other request threads. This is because it is where the security information for a given user is stored. The `UmbracoContext` is also a request-scoped object. In fact, it relies directly on an instance of `HttpContext`. The `UmbracoHelper` is request-scoped as well. +An example of a request-scoped instance is the `HttpContext`. This object exists for a single request and it cannot be shared between other threads, especially not other request threads. This is because the object's thread is where the security information for a given user is handled. The `UmbracoContext` is also a request-scoped object. In fact, it relies directly on an instance of `HttpContext`. The `UmbracoHelper` is request-scoped as well. -In the example above, the `UmbracoHelper` which has a request-scoped lifetime, will be statically assigned to a variable. It means that this particular request-scoped object is now bound to an Application-scope lifetime and will never go away. This could mean that under certain circumstances an entire Umbraco cache copy is stuck in memory. It could also mean that the `Security` property of the context will be accessed by multiple threads. However, this now contains the security information for a user from another request. +In the example above, the `UmbracoHelper`, which has a request-scoped lifetime, will be statically assigned to a variable. This request-scoped object is now bound to an Application-scope lifetime and will exist after the request has ended. This could mean that under certain circumstances an entire Umbraco cache copy is stuck in memory. It could also mean that the `Security` property of the context will be accessed by multiple threads, and these threads may now contain the security information for a user from another request. Additionally there is never really any reason to use static references. Instead, you should always inject your required resources, and let the DI container handle the lifetimes of the objects. @@ -116,9 +116,9 @@ You create a menu on your Home page like: The query above renders out: _Root, Home, Blog, Office Locations, About Us, Contact Us_ -This is going to iterate over every single node in Umbraco, all 10,000 of them. This will have a negative effect on the sites general performance. +This is going to iterate over every single node in Umbraco, all 10,000 of them. This will have a negative effect on the site's general performance. -Instead of using the snippet above, something similar to the sample below can be used: +Instead of using the snippet above, something similar to the snippet below can be used: ```csharp ``` -In many cases, you might know that there is only ever going to be a small number of Descendants. If so using Descendants or DescendantsOrSelf will not have a negative affect on the sites performance. It is important to always be aware of the implications of what you are writing. +In many cases, you might know that there is only ever going to be a small number of Descendants. If so, using Descendants or DescendantsOrSelf will not have a negative effect on the site's performance. It is important to always be aware of the implications of what you are writing. -## Too much querying (Over querying) +## Too much querying ("Over querying") -Querying and traversing content is not free. Anytime you make a query or resolve a property value there is overhead involved. Think about every query you make as an SQL call: Too many requests can have a negative effect on the sites performance. +Querying and traversing content is not free. Anytime you make a query or resolve a property value there is overhead involved. Think about every query you make as an SQL call; too many requests can have a negative effect on the site's performance. -Here is a common pitfall in relation to this. +Here is a common pitfall in relation to this: Following the example above, the menu is going to be rendered using the current page's root node: @@ -150,7 +150,7 @@ Following the example above, the menu is going to be rendered using the current ``` -The `@Model.Root()` syntax is shorthand for doing this: `Model.AncestorOrSelf(1)`. This means that it is going to traverse up the tree until it reaches an ancestor node with a level of one. As mentioned above, traversing costs resources and in this example, there are 3x traversals being done for the same value. +The `@Model.Root()` syntax is shorthand for doing this: `Model.AncestorOrSelf(1)`. This will traverse up the tree until it reaches an ancestor node with a level of one. As mentioned above, traversing costs resources and in this example, there are 3x traversals being done for the same value. Consider writing something similar to the example below: @@ -192,9 +192,9 @@ If you are using services in your views, you should figure out why this is being ## Using Umbraco content items for volatile data -This is one of the anti-patterns that could have the highest negative impact on your sites performance. +This is one of the anti-patterns that could have the most negative impact on your site's performance. -Umbraco content should not be used for volatile data. The Umbraco APIs and the way Umbraco data is persisted was not designed for this. When you need to store, write or track data that changes a lot use a custom database table or another service. Do not use Umbraco content nodes for this. +Umbraco content should not be used for volatile data. The Umbraco APIs, and the way Umbraco data is persisted, was not designed for this. When you need to store, write or track data that changes a lot, use a custom database table or another service. Do not use Umbraco content nodes for this. Some examples of what not to do, and what to do instead: @@ -206,18 +206,16 @@ Some examples of what not to do, and what to do instead: ## Processing during startup -Umbraco allows you to run some initialization code during startup by using `UmbracoApplicationStartingNotification`. Depending on what code is run, it can have an impact on the application startup. This is especially true for Package developer as your code could end up impacting many websites. +Umbraco allows you to run some initialization code during startup by using `UmbracoApplicationStartingNotification`. This codee can have a negative impact on the application startup process. This is especially true for Package developers as your code could end up impacting many websites. -In many cases, [initialization code can be done lazily instead of eagerly](https://msdn.microsoft.com/en-us/library/dd997286\(v=vs.110\).aspx). Instead of initialization everything you need as soon as the application starts you could execute your initialization code only when it is required. This can be achieved in different ways: +In many cases, [initialization code can be done lazily instead of eagerly](https://msdn.microsoft.com/en-us/library/dd997286\(v=vs.110\).aspx). Instead of initialization everything you need as soon as the application starts, you could execute your initialization code only when it is required. This can be achieved in different ways, such as: -* Using [`Lazy`](https://msdn.microsoft.com/en-us/library/dd642331\(v=vs.110\).aspx) and put the initialization logic in its callback. +* Using [`Lazy`](https://msdn.microsoft.com/en-us/library/dd642331\(v=vs.110\).aspx) and putting the initialization logic in its callback. * Using [`LazyInitializer`](https://msdn.microsoft.com/en-us/library/system.threading.lazyinitializer\(v=vs.110\).aspx?f=255\&MSPPError=-2147217396). * Putting logic in a property getter with a lock and setting a flag when it is processed. -* Putting logic in a method with a lock and setting a flag when it is processed +* Putting logic in a method with a lock and setting a flag when it is processed. -The list above is not a complete list of options, as there are many different ways of achieving this. - -It is important to ensure that the initialization logic executes only once for the lifetime of the application even when your app domain is restarted. If your initialization logic creates a database table that should only be executed one time, set a persistence flag. A persistence flag will indicate to your own logic that the initialization code has already been executed and does not need to be done again. +It is important to ensure that the initialization logic executes only once for the lifetime of the application, even when your app domain is restarted. If your initialization logic creates a database table that should only be executed one time, set a persistence flag. A persistence flag will indicate to your own logic that the initialization code has already been executed and does not need to be done again. ## Rebuilding indexes @@ -232,15 +230,15 @@ It is not recommended to rebuild your indexes unless you absolutely need to. If ## Performing lookups and logic in Examine events -There is a couple of well known Examine events: `TransformingIndexValues` and `DocumentWriting`. Both of these events allow the developer to modify the data that is going into the Lucene index. Many times we see developers performing service lookups in these methods. For example, using `IContentService.GetById(e.NodeId)` inside of these events could cause an `N + 1` problem. This is because these events are executed for every single document being indexed. If you are rebuilding an index, this will mean that this logic will fire for every single document and media item going into each index. That could mean a large number of lookups and impact the site performance negatively. +There are a couple of well-known Examine events: `TransformingIndexValues` and `DocumentWriting`. Both of these events allow the developer to modify the data that is going into the Lucene index. We often see developers performing service lookups in these methods. For example, using `IContentService.GetById(e.NodeId)` inside of these events could cause an `N + 1` problem. This is because these events are executed for every single document being indexed. If you are rebuilding an index, this will mean that this logic will fire for every single document and media item going into each index. That could mean a large number of lookups, which can negatively impact on the site's performance. -Similarly, when executing logic in these events that perform poorly, then anytime you save or publish content or media it will slow that process down. And if you rebuild an index then any slow code running in these events will cause the indexing to go even slower. +Similarly, if you are executing inefficient logic in these events, anytime you save or publish content or media that logic will slow the process down. If you rebuild an index, any slow code running in these events will cause the indexing to go even slower. ## RenderTemplateAsync -The API method is called `RenderTemplateAsync` allows you to be able to render a particular content item's template and get a `IHtmlEncodedString` in response. This could be useful if you want to send an email based on a content item and its template. However, you must be careful not to use this for purposes it is not meant to be used for. +The API method called `RenderTemplateAsync` allows you to render a particular content item's template and get a `IHtmlEncodedString` in response. This could be useful if you want to send an email based on a content item and its template. However, you must be careful not to use this for purposes it is not meant to be used for. -Do not use this method for rendering content as this could cause severe performance problems if abused. For normal content rendering of module type data from another content item, you should use Partial Views instead. +Do not use this method for rendering content as this could cause severe performance problems. For you are rendering normal content of module type data from another content item, you should use Partial Views instead. ## Do not put logic inside your constructors @@ -305,15 +303,15 @@ You run the following code to show the favorites: ``` -To show the top 10 voted recipes's this will end up doing the following: +To show the top 10 voted recipes, this code will end up doing the following: -* It will iterate over all 5000 Recipes. -* It will create and allocate 5000 instances of `RecipeModel`. +* Iterate over all 5000 Recipes. +* Create and allocate 5000 instances of `RecipeModel`. * For each `RecipeModel` created, it will traverse upwards, iterate all 5000 recipes then resolve property data for 2 properties. This means that there is now an additional 5,000 new objects created and allocated in memory. The number of traversals/visits to each of these objects is now `5000 x 5000 = 25,000,000`. -The other problem is that the logic used to lookup related recipes is inefficient. Instead, each recipe should have a picker to choose its related recipe's and then each of those can be looked up by their ID. +The other problem is that the logic used to lookup related recipes is inefficient. Instead, each recipe should have a picker to choose its related recipes, and then each of those can be looked up by their ID. ## Do not eager load data, lazy load it instead @@ -346,7 +344,7 @@ The above example could be rewritten like this: The code will still iterate over all Recipes meaning that the number of traversals/visits to each of these objects will be 5000. -There really is not much reason to create a `RecipeModel`. Instead it could be written like: +There really is not much reason to create a `RecipeModel`. Instead, it could be written like: ```csharp @{ @@ -365,19 +363,19 @@ There really is not much reason to create a `RecipeModel`. Instead it could be w ## Not caching expensive lookups -Based on the above 2 points, you can see that iterating content with the traversal APIs ends up being expensive in terms of performance. +Based on the above two points, you can see that iterating content with the traversal APIs ends up being expensive in terms of performance. -How to solve performance issues, will always depend on the specific scenario. One thing to consider is to cache the IDs of the content you need in your critical code. Then you could retrieve it from the cache by ID. +How to solve performance issues will always depend on the specific scenario. One thing to consider is to cache the IDs of the content you need in your critical code. Then you could retrieve the content from the cache by ID. -When you need to render the same four pieces of content for your navigation, then cache, or hardcode, the IDs of those content items. Then you can retrieve them with the ID using `Umbraco.Content`. This will always be faster than trying to traverse your content tree and finding the content programmatically. It will do a direct lookup in the cache, meaning that your code does not have to do many traversals to get your content. +When you need to render the same four pieces of content for your navigation, we recommend caching, or hardcoding, the IDs of those content items. You can retrieve the content from their IDs using `Umbraco.Content`. This will always be faster than trying to traverse your content tree and finding the content programmatically. It will do a direct lookup in the cache, meaning that your code does not have to do many traversals to get your content. ## Be mindful about memory -When memory is used, for instance creating 5,000 recipe models with a `Select` statement, Garbage Collection needs to occur. This turnover can cause performance problems. The more objects created, the more items allocated in memory, the harder the job is for the Garbage Collector == more performance problems. +When memory is used, for instance creating 5,000 recipe models with a `Select` statement, [Garbage Collection](https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/) needs to occur. This turnover can cause performance problems. The more objects created, the more items allocated in memory, the harder the job is for the Garbage Collector, resulting in more performance problems. -Even worse is when you allocate a lot of or large items in memory. They will remain in memory for a long time ending up in "Generation 3" which the GC tries to ignore for as long as possible. It does so because it knows it is going to take a lot of resources to clean up. +Even worse is when you allocate a lot of large items in memory. These items will remain in memory for a long time, ending up in "[Generation 3](https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/fundamentals#generations)" which the Garbage Collector tries to ignore for as long as possible. It does so because it knows it is going to take a lot of resources to clean up. ## Best practices when using Models Builder Extending models should be used to add stateless, local features to models. It should not be used to transform content models into view models or manage trees of content. -You can read more about this in the [Understanding and Extending Models Builder documentation](templating/modelsbuilder/understand-and-extend.md) \ No newline at end of file +You can read more about this in the [Understanding and Extending Models Builder documentation](templating/modelsbuilder/understand-and-extend.md) From 32ec80f4176548b839ee3584d0417d9f393a3f41 Mon Sep 17 00:00:00 2001 From: Richard Jackson Date: Sun, 27 Oct 2024 11:14:22 +0000 Subject: [PATCH 2/4] Update common-pitfalls.md --- 15/umbraco-cms/reference/common-pitfalls.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/15/umbraco-cms/reference/common-pitfalls.md b/15/umbraco-cms/reference/common-pitfalls.md index bf728a7645b..0e10fcb4a5f 100644 --- a/15/umbraco-cms/reference/common-pitfalls.md +++ b/15/umbraco-cms/reference/common-pitfalls.md @@ -76,14 +76,14 @@ public class BadApiController : Controller This practice can cause memory leaks along with inconsistent data results when using this `_umbracoHelper` instance. -It is important to understand the difference between an object that has a Request-based scope and an object that has a Singleton/Application-based scope. +It is important to understand the difference between an object with Request-based scope and Singleton/Application-based scope. * **Application scope**: If an object has a singleton/application scope, that single object instance will exist for the lifetime of the application. The single instance will be shared by every thread that accesses it. Static variables will always exist for the lifespan of the application. * **Request scope**: The web world is made up of requests and each request has its own thread. When an object is in the scope of a Request it only survives as long as the web request survives. At the end of the web request, the object may either be disposed of or cleared from memory by the garbage collector. Request scoped object instances are not accessed by every other thread in the application unless you do something like the above. An example of a request-scoped instance is the `HttpContext`. This object exists for a single request and it cannot be shared between other threads, especially not other request threads. This is because the object's thread is where the security information for a given user is handled. The `UmbracoContext` is also a request-scoped object. In fact, it relies directly on an instance of `HttpContext`. The `UmbracoHelper` is request-scoped as well. -In the example above, the `UmbracoHelper`, which has a request-scoped lifetime, will be statically assigned to a variable. This request-scoped object is now bound to an Application-scope lifetime and will exist after the request has ended. This could mean that under certain circumstances an entire Umbraco cache copy is stuck in memory. It could also mean that the `Security` property of the context will be accessed by multiple threads, and these threads may now contain the security information for a user from another request. +In the example above, the `UmbracoHelper`, which has a request-scoped lifetime, will be statically assigned to a variable. This request-scoped object is now bound to an Application-scope lifetime and will exist after the request has ended. This could mean that under certain circumstances an entire Umbraco cache copy is stuck in memory. It could also mean that the `Security` property of the context will be accessed by multiple threads. These threads may now contain the security information for a user from another request. Additionally there is never really any reason to use static references. Instead, you should always inject your required resources, and let the DI container handle the lifetimes of the objects. @@ -373,7 +373,7 @@ When you need to render the same four pieces of content for your navigation, we When memory is used, for instance creating 5,000 recipe models with a `Select` statement, [Garbage Collection](https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/) needs to occur. This turnover can cause performance problems. The more objects created, the more items allocated in memory, the harder the job is for the Garbage Collector, resulting in more performance problems. -Even worse is when you allocate a lot of large items in memory. These items will remain in memory for a long time, ending up in "[Generation 3](https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/fundamentals#generations)" which the Garbage Collector tries to ignore for as long as possible. It does so because it knows it is going to take a lot of resources to clean up. +Even worse is when you allocate a lot of large items in memory. These items will remain in memory for a long time, ending up in "[Generation 3](https://learn.microsoft.com/en-us/dotnet/standard/garbage-collection/fundamentals#generations)" which the Garbage Collector ignores for as long as possible. It does so because it knows it is going to take a lot of resources to clean up. ## Best practices when using Models Builder From ba2a8ed77b64c5c4ede285456215bd59496166a6 Mon Sep 17 00:00:00 2001 From: Richard Jackson Date: Tue, 29 Oct 2024 12:55:37 +0000 Subject: [PATCH 3/4] Update 15/umbraco-cms/reference/common-pitfalls.md Sounds good! Thanks Sofie! Co-authored-by: sofietoft --- 15/umbraco-cms/reference/common-pitfalls.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/15/umbraco-cms/reference/common-pitfalls.md b/15/umbraco-cms/reference/common-pitfalls.md index 0e10fcb4a5f..2a7e00f135c 100644 --- a/15/umbraco-cms/reference/common-pitfalls.md +++ b/15/umbraco-cms/reference/common-pitfalls.md @@ -206,7 +206,7 @@ Some examples of what not to do, and what to do instead: ## Processing during startup -Umbraco allows you to run some initialization code during startup by using `UmbracoApplicationStartingNotification`. This codee can have a negative impact on the application startup process. This is especially true for Package developers as your code could end up impacting many websites. +Umbraco allows you to run some initialization code during startup by using `UmbracoApplicationStartingNotification`. This code can have a negative impact on the application startup process. This is especially true for Package developers as your code could end up impacting many websites. In many cases, [initialization code can be done lazily instead of eagerly](https://msdn.microsoft.com/en-us/library/dd997286\(v=vs.110\).aspx). Instead of initialization everything you need as soon as the application starts, you could execute your initialization code only when it is required. This can be achieved in different ways, such as: From a3bf8d5436a263f576a494a82d3bd1abd9ae1f2b Mon Sep 17 00:00:00 2001 From: sofietoft Date: Wed, 30 Oct 2024 11:00:08 +0100 Subject: [PATCH 4/4] Trigger GitBook checks --- 15/umbraco-cms/reference/common-pitfalls.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/15/umbraco-cms/reference/common-pitfalls.md b/15/umbraco-cms/reference/common-pitfalls.md index 2a7e00f135c..cf2999a8b42 100644 --- a/15/umbraco-cms/reference/common-pitfalls.md +++ b/15/umbraco-cms/reference/common-pitfalls.md @@ -1,10 +1,10 @@ --- -description: "Information on common Pitfalls and Anti-Patterns in Umbraco" +description: Information on common Pitfalls and Anti-Patterns in Umbraco --- # Common Pitfalls & Anti-Patterns -This section highlights common pitfalls that developers often encounter. Some of the anti-patterns discussed here can lead to memory leaks, instability, or poor performance on your site. Be sure to read this section — it could save your site. +This section highlights common pitfalls that developers often encounter. Some of the anti-patterns discussed here can lead to memory leaks, instability, or poor performance on your site. Reading this section could save your site. ## Usage of Singletons and Statics