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 Improvement: optimized DatabaseChangeLog.normalizePath() #3853
Conversation
The validation errors appear to be caused by a separate change that was already in master. #3881 |
…rmance # Conflicts: # liquibase-core/src/main/java/liquibase/changelog/DatabaseChangeLog.java
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.
Let's get the performance back!
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.
This PR is a performance improvement to avoid a processing bottleneck while loading large changelogs.
- Functional and test harness executions passing.
- No additional testing required.
APPROVED
Impact
Description
PR #3063 improved performance of
DatabaseChangeLog.normalizePath()
by no longer re-compiling regular expressions, but it is a method that is called often and with a large changelog it still appears to be a bottleneck.This PR replaces the regular expressions with more efficient "replace" calls and/or adds a "if it might be a problem" gate around the regexp call.
From testing, it removed about half the time spent based on profiling data from #3816
Fixes partially #3816
Things to be aware of
It may be possible to reduce the number of calls to this function in the first place as well. As part of the review process, it is probably worth looking to see if we keep re-calling normalizePath for the same paths over and over or not.
In testing this, the customer reported that it caused some validation errors (see #3816). I'm not sure how it could off hand, since it's just in the normalizePath, it's possibly causing some changesets to be identical now? So needs a bit more unit testing
Things to worry about
Not much, there are existing tests around the normalizePath() which would fail as I pulled out each regexp before replacing it with a comparable replace() call
Additional Context