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
Improved "include" fix performance: optimized normalizePath method #3063
Conversation
…ssue: Optimize updates liquibase#1856
Thanks @lzxgyh! I worry about the creation of the matcher static variables, though. If this gets used in a multithreaded way, I think those will cause problems. If you just create the matchers as local variables instead of resetting the static variables does it loose a lot of the performance gains? The original "compile" call should be what makes the most difference, right? |
Hi nvoxland I think you are right, in the past, I always thought that liquibase was a single-threaded tool |
Are you able to fix that up quick, @lzxgyh? |
Sure, I pushed a new commit to remove the matcher static variables , Please review @nvoxland |
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.
Code review and test results:
Things to be aware of:
- Just moves implementation of where the regular expressions are managed from one spot to another, no functionality change
- I did not re-test the performance improvements, but I can see from the performance dumps that this what a large problem in the code and the PR will fix it as the contributor says it does
Things to worry about:
- Nothing
.replaceFirst("^\\.?/", "") | ||
; | ||
|
||
String noClassPathReplaced = NON_CLASSPATH_PATTERN.matcher(filePath).replaceFirst(""); |
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.
Thank you, @lzxgyh - This is a nice improvement to code readability!
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.
- Fix improves performance of normalizePath.
- Fix improves readability of REGEX matching in DatabaseChangelog.java
- No additional testing necessary.
APPROVED
Impact
Description
Replaces on-the-fly regular expression usage with pre-compiled versions.
Fixes #2221
Fixes #1856
Things to be aware of
Things to worry about
Additional Context
Add any other context about the problem here.