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

Added caching option #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jameskimsca
Copy link

This is in relation to:
#6

Added a configurable option to cache the node collection serverside.

@PerplexDaniel
Copy link
Member

Hi James,

Thank you for your pull request!

I do feel you are adding a lot of code for just caching some tree nodes though, I'm hesitant to merge stuff like that ArrayExtensions class when the same can be achieved with a simple for loop instead, which does not require that entire file.

Same with the CacheService, could you perhaps try if the same can be achieved using Umbraco's ApplicationContext.Current.ApplicationCache? That would cut away so much code so we can focus on the actual issue.

About that, it seems you are caching the result of FormTreeController.GetTreeNodes, which is an Umbraco Forms class, so am I right in asserting that this issue also affects default Umbraco Forms without our extension? In that case it would perhaps even be wise to submit this report to the Forms Core team directly, then everybody can enjoy the speed-up.

I will be unavailable in the coming 2 weeks but will look also have a look at slowdowns after that, I also enjoy good performance so any speed-ups are definitely appreciated :-)!

@JeffreyPerplex
Copy link

Hi James,

Thanks for contributing, it's really appreciated!

Just out of curiosity: the pull request is fixing the fact that the forms are rendered slow? Can you give an indication of how many forms you have and how slow the experience is?

Regards,

Jeffrey

@jameskimsca
Copy link
Author

Hi @PerplexDaniel,

I have refactored the code to use the Appcontext and minimized the changes.

It is an inherent issue with Umbraco Forms but they have no concept of folders so it's not really as much of a concern for them. I will raise it with them as well but usually they take forever with these feature requests.

As a Perplex user it has become very unperformant without caching and in regards to your question @JeffreyPerplex we have about a thousand forms in the system in production and it takes around 2-3 minutes to expand a folder. I have tested with the cache in place and it's so much faster on subsequent folder usage and it would help the editors greatly with frequently accessed sections.

Please review it again and consider adding it to Perplex.

Regards,

James

@PerplexDaniel
Copy link
Member

@jameskimsca Great! As mentioned I was gone for 2 weeks, but am back now and will have a look at this sometime later this week. If all is well I can then release a new version of our package. In the meantime you can of course build the solution with your code to already have the improved performance for your clients.

Good that you also bring it up with Umbraco though, as it seems they should fix the core issue instead so even the first load will be fast.

@PerplexDaniel
Copy link
Member

@jameskimsca

I just checked out the PR. Caching of forms seems to work fine, but there were some issues with folders. Creating a folder, sorting stuff within a folder, or deleting a folder (and possibly other operations) do not clear the cache. This would need to be added as well before it is actually usable, as is this breaks all folder-related operations so I unfortunately cannot merge this one in right away.

Could you have a look at that? For example:

  1. Add Form
  2. Add Folder (right click on root Forms node > Create Folder)
  3. Observe that the Folder is not visible in the tree due to the cache not being updated.
  4. Add another Form (to clear cache)
  5. Folder of step 3 is now visible.

Same applies to other folder operations like delete / sort etc.

Oh, and also when renaming a form the name in the tree does not update.

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.

3 participants