-
Notifications
You must be signed in to change notification settings - Fork 754
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
Performance problems when huge number of portal aliases is in use #2514
Performance problems when huge number of portal aliases is in use #2514
Conversation
We have a number of sites with thousands of portals, so definitely interested in how this shakes out (also, cc @mitchelsellers). Do you have before & after metrics of this change yet? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change looks good on the surface, will need to see details of how this affects sites before final approval
@bdukes According to preliminary tests, this is now 5x faster. |
To see whether new implementation brings any improvements or not and how it is valuable, there was a separate tool created to simulate and compare behaviors to find out the best approach. Summary: I expect 5x faster URL validation. |
I'm in 100% agreement with @bdukes on this. On the surface, things make sense. But I'd really want to see real-world performance numbers on this to prove this out.. As the impacts of a behavior change here could be catastrophic. Honestly, I'd like to see a testing plan for this that includes a very large number of unit tests to validate inputs & outputs. In addition to the true performance validation as the diagnostics of issues here could be very very hard. |
@mitchelsellers @bdukes @daguiler Testing environment detailsWindows Server 2012 R2 Standard, Intel(R) Xeon(R) CPU E5-2670 v2 @ 2.50GHz 2.49GHr, 15 GB RAM DotNetNukeEvoq edition, v.9.1.1.129 PreconditionsPortals created: 10, aliases: 5000. No other tables were modified. We want to isolate alias validation functionality, so this data is enough to proceed. Load testing is performed to determine system behavior and compare responsiveness of an application using following approaches to validate alias:
Purpose is to determine if the implementation proposed in this PR is more efficient under heavy load. Load test strategy
ConclusionTwo charts below show the number of requests and response time (vertical axis) needed to complete each of consecutive request. It can be seen that most requests using current implementation requires more time to be completed (red line) vs. compiled regex (blue line). Using 260 concurrent requests, we can observe following responsiveness. Please, note, elapsed time measured below is a difference between time when request was sent and time when response has been fully received. Chart shows new implementation can process requests faster than current one. Considering results below, we can say it is possible to process 2X more concurrent requests now at the same time which is the goal of proposed changes (of course depending on hardware and environment infrastructure). Load testing toolsVS2017, Apache JMeter Chart sources |
I'd like to add some unit testing results. I executed all tests in the DotNetNuke.Tests.Urls assembly (1596) and all of them still pass. Coverage is particularily good in the affected classes:
Some screenshots: |
This feedback looks great. @bdukes any thoughts on testing? |
@mitchelsellers, @bdukes there seems to be an issue with case sensitivity that we are trying to identify and fix. |
looks fine to me, though I am curious about the effect on performance in real live scenarios. |
Case sensitivity issue was found and fixed by @mikebigun and new integration tests were added to cover mixed-cased URLs. All 1991 URL tests pass: |
…nsoftware#2514) * DNN-27498 Performance Issues * DNN-27498 Performance Issues * minor formatting * Fixed case sensitivity issue * Added mixed cased alias support to unit tests * Fixed VanityUrl unit tests
* DNN-27517: force user logout after password changed in other place. * DNN-27517: update code by review. * DNN-27517: add host settings to control whether force logout after password changed. * NOJIRA: mark as stable. * Fixed bugs on add/remove user permissions for modules * Change algorithm to SHA1CryptoServiceProvider * Updated Issue Templates to include new RFC template and to support submissions for 9.3.0 release * Corrected structure to avoid issue linking * code review * User registration: end the response after redirect (#2511) * Initial New User Email Not Sending At Time of Creation (#2492) This is alternative way to fix above issue proposed in dnnsoftware/Dnn.AdminExperience#174 As per @sleupold , we need to move email notifications from UI to core part. Once this will be approved and merged, we can remove email notifications from UI and replace it with updated controller method to let notifications to be send to their recipients. fixes #2424 * Fix for missing SQL change (#2522) Fixes #2521 by rebuilding the PortalsDefaultLanguage view * Resolve UserProfile Loading Errors From Unsecure pages (#2494) * NOJIRA: mark as stable. * DNN-21637: add config key. * DNN-26576: prevent same-origin errors when loading popup and iframes from a secure page. * code review * Code review * (DNN-10795) - All pages except home page return 404 (#2032) * DNN-10795 - All pages except home page return 404 I have witnessed that occasionally on app pool recycle all,except the home page, will return 404 until the application pool is recycled a second time. I've reviewed the code & believe that the root cause of the issue is due to the fact that the code that builds the tab index, portalDepths dictionary & tabPaths dictionary is not thread safe. I can see code in the method TabIndexController.FetchTabDictionary is using SharedDictionary classes to store the tab dictionaries, however the code is not thread safe when adding the dictionaries to the cache. Therefore when multiple threads are executing the FetchTabDictionary method it's possible for an empty dictionary to be added to the cache. To resolve this issue the code has been updated so that only one thread can add the dictionaries to cache at a time. * Updated comment to trigger Code Licence workflow. * Added compiled DLLs that include the fix for bug DNN-10795 (All pages except home page return 404) for DNn versions 8.0.4 through 9.2.2 * Recursive read lock acquisitions not allowed (#2423) * DNN-23293 Recursive read lock acquisitions not allowed in this mode. * DNN-23293 Recursive read lock acquisitions not allowed in this mode. * Performance problems when huge number of portal aliases is in use (#2514) * DNN-27498 Performance Issues * DNN-27498 Performance Issues * minor formatting * Fixed case sensitivity issue * Added mixed cased alias support to unit tests * Fixed VanityUrl unit tests * Fixed broken LockStrategy unit tests (#2531) * Delete Fixed-DLLs folder that was added as part of PR for bug DNN-10795. (#2535) * Modules > ModuleCreator > fixed path error (#2527) * Fixed issue in ModuleCreator > Web > template.ascx * Update DNN Platform/Admin Modules/Dnn.Modules.ModuleCreator/Templates/Web/Module - HTML/template.ascx Co-Authored-By: mean2me <emanuele.colonnelli@gmail.com> * All languages are highlighted along with current - add css for languages * Log name of package when uninstalling extensions (#2557) * remove spaces * DNN-20856 After export with Content Localization site language flags disappears from pages (#2578) * Fixed parallel build (#2562) * Set active Nuget package source to All * Fixed parallel build * Inclusion of NDepend logo on the readme. (#2598) * Fix for missing SQL change Fixes #2521 by rebuilding the PortalsDefaultLanguage view * Added attribution to NDepend for the usage of their ADO tooling * Fix image/link markdown * Get language from transferred parameter (#2607) * switch encrypt method. (#2616) * DNN-29484: switch encrypt method. * NuGet Package Improvements Changes to modernize the NuGet packages published by the DNN Platform, fixes #2586. The below-submitted changes in structure have been validated by consultation with the DNN Platform Community, Microsoft Representatives, as well as validation of documentation per the published .nuspec file definition (https://docs.microsoft.com/en-us/nuget/reference/nuspec) In detail, the following items have been changed: * Migration of license information to the suggested <license> node rather than the deprecated <licenseurl> node. * Inclusion of target framework for all included .dll files, this prevents installation of the package to pre-4.5 projects protecting downstream users. * Improved package descriptions based on discussions held in the RFC regarding these improvements * Added Package-to-Package dependencies to ensure quick usage and inclusion * Updated the WebAPI and MVC packages to be holistic packages, including references to ALL needed items to develop using those patterns. All changes are current for DNN Platform version 9.3.0 or later. Packages have been built & tested locally with success. ## Suggested Usage With these improved packages, development & references should be easier. ### MVC Modules `Install-Package DotNetNuke.Web.Mvc` Should be the only needed package installation. It will install all needed dependencies, including the items necessary for WebAPI ### Modules Needing WebAPI (Not MVC) `Install-Package DotNetNuke.WebApi` Should be the only needed package for extensions not using MVC, however, needing to use WebApi for services. This will work well for WebForms or Library projects, etc. that don't need the extra references for MVC/Razor ### WebForms/Limited Modules `Install-Package DotNetNuke.Core` The most simple modules, still using the WebForms pattern can use this package for the smallest footprint For #2600 * Adjust the Source package to include changes from GitVersion (#2609) * remove old ckeditor packaging steps * Remove version to allow GitVersion to set it at build time (#2639) * Adding 09.03.01.SqlDataProvider file * Upgrade DNN to .NET Framework 4.7.2 (#2644) * Upgraded app projects to .NET Framework 4.7.2; Added missing dependency to DotNetNuke.Tests.Core as it was missing DotNetNuke.Web.Client * Removed targetframework web.config reference from Dnn.Modules.Console * Reverted unintended changes
fixes #2513
The main concern I see is how the alias validation is implemented on incoming requests. There is a regex expressions used to validate alias. There are multiple purposes to use such regular expressions:
a) validate whether requested URL has a proper format and b) check if alias provided in URL exists in the system. For that, we are caching list of all aliases available in system and generating list of accordingly created NON-COMPILED regex objects (one regex per one alias). Having ~5K aliases, system generates ~5K expressions by replacing ALIAS placeholder in a pattern by alias value. On every request, system iterates over ~5K expressions in a loop and checks whether requested URL matches to one of them. Does not look like a really good idea in terms of performance. Now let's imagine ~2K requests is coming at the same time and for every request we should apply ~5K expressions. On top of that, no one regex is compiled, this is impacting system a lot.
The idea is to move from generating a huge number of expressions and keeping all of them in a cache to a single COMPILED regex. Iterating over aliases is enough to satisfy our purposes. This approach will bring a lot of benefits. We'll not need to store all regex in a cache, hence it will not require us to search it there on every iteration. At the end, we don't need to apply every regex to a single url (this is the most important thing).
Moving to compiled regex is recommended for a frequent use. Moreover, it is senseless to validate the whole URL format if it does not contain expected alias.