U4-9337 - Double-checking urls is killing the CPU

Created by Stephan 04 Jan 2017, 06:58:31 Updated by Brian Powell 23 Jan 2017, 19:38:32

Tags: Unscheduled

Relates to: U4-9430

Relates to: U4-9121

Whenever the NiceUrl routing creates a url it double-checks whether that url can collide with another node. For every node. And that is killing the CPU on republish of large branches. But really, only the top-level nodes should be impacted? Investigate.

1 Attachments

Comments

Shannon Deminick 04 Jan 2017, 07:41:13

In this screenshot you can see the CPU perf profiling of what methods are chewing through the CPU. This sample is taking during a rename of a parent node that has 10,000+ children (in a list view) and the CPU due to the 301 tracker is killing perf.

the first part is during the initial event, the 2nd is during the final event.

I've noticed that we have perf/allocation issues with our config section which is not good. I've fixed this in this PR too but only for one section, we need to fix it for all sections.

PR so far: https://github.com/umbraco/Umbraco-CMS/pull/1671


Shannon Deminick 04 Jan 2017, 09:14:27

NOTE: Need to change the XmlElement cast to an as XmlElement and null check because it could be XmlWhiteSpace


Stephan 04 Jan 2017, 13:02:02

I have taken care of that note

I have updated the PR with the following changes:

  • Before 7.5.4 whenever we GetUrl() we would cache the resulting Url, which in case of a collision would cause great confusion because depending on which node did GetUrl() last, the routing may differ
  • In 7.5.5 whenever we GetUrl() we would check that the resulting Url was not colliding with another one, and in case of a collision we would report it and not cache the url
  • That proved to be expensive

The fix is: we do ''not'' cache the url at all anymore when GetUrl(), only on inbound (when routing a request). BUT in the UI whenever we display a content, after we GetUrl(), we try to route that url to see if it reaches the content, or another one.

UI-wise the result is the same. Perfs-wise the result is the same in the UI (one routing per url). The good thing is that this routing takes place only in the UI and not all the time.

The semi-bad thing is that now if you render a page with 100 links, these will be fast to generate, but we won't cache the urls, meaning when a visitor tries any of these links we'll have to try to route the request (and cache the result). But I believe it's a minor hit (again once the page has been visited once the url is cached)


Shannon Deminick 05 Jan 2017, 22:44:40

Regarding rendering a page with 100 links, does this mean that without caching that everytime this page renders it needs to recalculate all 100 links?


Stephan 06 Jan 2017, 06:22:26

as long as none of these links have been visited, yes

so... I can see the problem, yes. had not thought about it. know how to fix, we need asymetric caching. will look into it today.


Shannon Deminick 06 Jan 2017, 06:43:33

The good news is with the current changes, renaming a node that has thousands of children now only takes a few seconds, before it was minutes.


Stephan 06 Jan 2017, 08:15:19

Have pushed a new commit, now the routes caching is asymmetric:

  • if we GetUrl, the id->route is cached but not the route->id, so next GetUrl will be fast but an inbound request will not use the (possibly colliding) route
  • if we route an inbound request, both id->route and route->id are cached, so next GetUrl and routing will be fast


Priority: Major

Type: Task

State: Fixed

Assignee: Shannon Deminick

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions: 7.5.5, 7.5.6

Due in version: 7.5.7

Sprint: Sprint 50

Story Points:

Cycle: