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

Performance Optimization - don't force field invalidations for non-velocity fields #24781

Closed
wezell opened this issue Apr 27, 2023 · 3 comments · Fixed by #24783
Closed

Performance Optimization - don't force field invalidations for non-velocity fields #24781

wezell opened this issue Apr 27, 2023 · 3 comments · Fixed by #24783
Labels

Comments

@wezell
Copy link
Contributor

wezell commented Apr 27, 2023

Problem Statement

When saving/updating content, we have to invalidate the content in cache. To do this, we run through the content type fields and do a velocity template cache invalidation of all the fields for that content type. Each one of these invalidations emits a pub/sub message and when checking in a ton of content, this can slow the db down. The issue is that only fields that are rendered with velocity in them actually require invalidation - like a widget's Code Field for example. We should not run an invalidation for all the content type's fields, only for those fields that need it.

Take a look at the thread stack - fully 25% of the server's resources is being spent invalidating the little used field cache.

Screen Shot 2023-04-27 at 3 57 54 PM

Steps to Reproduce

Hard to test - you need to run 1000's of content checkins under load.

Acceptance Criteria

Make sure that content and widgets used on velocity pages can be edited and show up as updated on the pages.

dotCMS Version

23.05

Proposed Objective

Application Performance

Proposed Priority

Priority 2 - Important

External Links... Slack Conversations, Support Tickets, Figma Designs, etc.

No response

Assumptions & Initiation Needs

No response

Quality Assurance Notes & Workarounds

No response

Sub-Tasks & Estimates

No response

@john-thomas-dotcms
Copy link
Contributor

john-thomas-dotcms commented May 1, 2023

I think the challenge is that velocity code can (and sometimes is) used in almost any text field. So, while it makes perfect sense to exclude fields like Select and Radio, it seems likely to cause problems to not invalidate all text and textarea-type fields.

Wondering if it would make sense to use a field variable in some way instead. The default behavior of invalidating all fields hasn't bitten anyone hard before now. So, maybe it would make sense to make that the default behavior for all text-containing fields, and then allow the addition of a field variable to exclude them for customers that run into this problem?

The default could be the opposite (exclude unless a field var is used to include them). But for both backward compatibility and for what seems like the most common case, I think a default to include them and only exclude based on a field var makes more sense. Changing the default behavior is likely to cause issues (and support tickets).

@wezell
Copy link
Contributor Author

wezell commented May 1, 2023

So the code uses the same logic as our parsing code does when deciding if a field is velocity or not - meaning, if dotcms would parse the field as velocity when rendering, then we will invalidate it as it might contain Velocity. If the field would not be parsed as velocity due to field type or lack of velocity characters in the code field, then we skip the invalidation (just like we would skip trying to parse it as velocity when displaying it).

Does this make sense?

@erickgonzalez erickgonzalez linked a pull request May 4, 2023 that will close this issue
erickgonzalez added a commit that referenced this issue May 9, 2023
@erickgonzalez erickgonzalez added the Release : 23.01.3 Included in LTS patch release 23.01.3 label May 9, 2023
@erickgonzalez erickgonzalez added LTS : Next Ticket that will be added to LTS and removed LTS : Next Ticket that will be added to LTS Triage labels May 15, 2023
erickgonzalez added a commit that referenced this issue May 18, 2023
* #24781 skip unneeded field invalidations

* #24781 invalidate RelationshipField

---------

Co-authored-by: erickgonzalez <erick.gonzalez@dotcms.com>
@erickgonzalez
Copy link
Contributor

This was QA also by a customer in LTS who saw a performance upgrade and hasn't heard any issues from them.

@erickgonzalez erickgonzalez added LTS: Excluded Ticket that has been excluded from at least one LTS Next LTS Release OKR : Customer Success Owned by Arno OKR : Customer Support Owned by Scott and removed LTS: Excluded Ticket that has been excluded from at least one LTS labels Jun 5, 2023
erickgonzalez added a commit that referenced this issue Jun 14, 2023
@erickgonzalez erickgonzalez added the Release : 22.03.7 Included in LTS patch release 22.03.7 label Jun 16, 2023
@AndreyDotcms AndreyDotcms added the Release : 23.01.4 Included in LTS patch release 23.01.4 label Jul 11, 2023
@AndreyDotcms AndreyDotcms removed the Release : 23.01.4 Included in LTS patch release 23.01.4 label Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants