Fix/ensured that we not load multiple times same datatypes in each context #15573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi there @bielu, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
|
@bielu do you know if V8 was also affected by this? We have a V8.18.12 instance with way more nodes (I'd say more than 15k) and that often takes minutes to publish pages with lots of blocks in a block list property. In our case, even though we have quite a few languages enabled in the CMS, we are not using the language variants. Loading such nodes in the backoffice also takes a while (nearly 1 minute just the GetById API call) |
|
@bergmania / @kjac as you were involved in similar pr #15184, can you maybe review this one? as it would be great to get it merged to 13.1 so we can base new projects based on umbraco which is actually usable on reasonable azure tiers :) |
|
Hi @bielu 👋 Thanks a lot for this! We're going to have a look at it as soon as possible. This won't be a 13.1 candidate, as 13.1 is going out next week. We'd like this to be included in an RC, given it's potential impact. So - 13.2 is likely the target 😄 |
|
@kjac that is undestable, can you maybe tell me potential timeframe for 13.2? I just need some info for my team to share :). |
|
13.2 is in ~7 weeks. 6 weeks after 13.1 |
|
Thanks Guys! |
|
Hey @bielu, thank you so much for taking the time and effort to create this PR. Alas we will not be accepting this PR in its current state as it is a bandaid implemented in an undesirable state. On the good side, we have been able to identify a very likely root cause because of your efforts for which we are very grateful. Since we are not certain yet that this is the only root cause causing issues with big/nested doctypes we are not pushing the change just yet, but it would be good to see if the change has a similar performance impact on your end https://github.com/umbraco/Umbraco-CMS/tree/v13/fix/datatype-configuration-serialization As we still have more investigating to do towards effectives and impact of the change, this might not be the way we end up going even more so as some things regarding the impacted code paths have changed in v14. In case we can not treat the root cause(s), we might apply a bandaid similar to how you proposed, but in a more opt-in way. This would look like a service that wraps the DataTypeService that can be injected in the affected classes (and 3th party property editors). This wrapper service would cache the datatypes in a dictionary like you did with a scoped lifetime so that there is little to no chance of having sideeffects. Looking forward to your findings and the continued collaboration! |
|
@Migaroez one of issues is locking on memory cache, which cause memory cache to take 1-3ms per request to memory cache, another one operations on top of memory cache done in datatype service. |
|
1-3ms per call to the memory cache seems very very wrong. |
|
@Migaroez I was debugging it already, just reading of memory cache should be instant, but Umbraco has a lot wrapped around in implementation for AppCache. Starting from CheckCloneableAndTracksChange in case o Clonable AppCache or ending on FastDictionaryAppCacheBase where we create new locks on top of memory cache. Both are causing memory cache to respond anywhere between 0.5ms up to even 3ms. Both clonning and creating locks are creating this delay on top of memory cache. Also second part of issues on top of it Idatatypeservice has ConvertMissingEditorOfDataTypeToLabel, which takes again anywhere from 0.5 up to 1.2ms (wasn't debugging it further to figure out why it happens) |
|
also as you wanted, there is serialization for datatypes: not sure how it will help you, but there you go :) |
|
@Migaroez any progress on issue? |
|
Nothing meaningful, can't seem to figure out why certain individual calls are taking so much longer for you than for me with the data I have. It seems like we will have to go the bandaid route, at least for now. |
|
hey @Migaroez, |
|
Hi is there any update about this issue? |
|
Hi @Migaroez, is there a timeline for implementing the band-aid solution for the route issue? Some of our clients are experiencing this exact problem, resulting in page load times of 20 to 30 seconds in the backoffice due to the GetById route. |
|
We are looking into the band-aid solution today. |
|
@Migaroez let me know when you have some POC so i can test if your solution helps or make it worst. |
|
@bielu https://github.com/umbraco/Umbraco-CMS/tree/v13/fix/excesive-datatype-load-times-bandaid |
|
@Migaroez so far it doesnt even boot 😢 |
|
Sorry, should have actually run it before going to lunch. Resolving now |
|
@bielu It runs, starting testing on my end. |
|
@Migaroez it is better but not as good as result of this bandaid 😓 |
|
@Migaroez just by skipping locking system from AppCaches, I managed to get 200ms+ lower |
|
@bielu Got time for a meeting? My tests indicate that the locking should not have that big of an impact. This is on 100000 calls to the cache, that's a difference of 21ms No lock methodology (what that IRequestCache uses underneath Pure call to the IRequestCache |
|
@Migaroez sure, i will find few |
|
@Migaroez send me dm with details for call when you find few |
|
@Migaroez I am not available tomorrow and Wednesday so lets catch up on thursday. |
|
So with @Migaroez we managed to get to solution which is compromise of both worlds (speed and maintainbility). There is chance it will get merge to v13.2RC, but umbraco hq is still investigating side effects and pontential breaking behaviours. |
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prerequisites
This pr address: #14936 and #14842
Description
This pr ensuring that we do no load hundrends of times same datatypes in context of loading backoffice and saving content.
Please check this comment: #14936 (comment)
Final result:
0 duplicated operation on RuntimeCache / DataTypService.
Testing
Umbraco Backoffice should not see change in behaviour and saving/publishing also should not be affected. So to test just play around in backoffice and ensured all properties are loading.
for performance testing I prepared this decorators:
https://gist.github.com/bielu/76433f85e21c09ba05b6df9e4bd64e3a
https://gist.github.com/bielu/f7f84550091504916ab07a4341bcdf9e
I register with usage of scrutor:
https://www.nuget.org/packages/Scrutor
via program.cs like this: