-
Notifications
You must be signed in to change notification settings - Fork 565
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
[HV-1928] reduces calls to isSubPathOf #1333
base: main
Are you sure you want to change the base?
Conversation
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.
Hey Thomas!
Thanks for taking a look into this. I've added a couple of comments inline.
Have you tried to run your case with this patch applied and if so did that help with performance?
By the way, we have some simple JMH performance tests (e.g. this one https://github.com/hibernate/hibernate-validator/blob/main/performance/src/main/java/org/hibernate/validator/performance/cascaded/CascadedWithLotsOfItemsValidation.java) that we are using to verify our performance improvement experiments. It would be nice if you can compare the results of existing and patched versions, and maybe try to add a case similar to yours.
...ava/org/hibernate/validator/internal/engine/validationcontext/AbstractValidationContext.java
Outdated
Show resolved
Hide resolved
...ava/org/hibernate/validator/internal/engine/validationcontext/AbstractValidationContext.java
Outdated
Show resolved
Hide resolved
Yes, but as it didn't finish in time for hours before, and it didn't finish with the patch, I cannot say if I gained anything as it did not terminate. 🤷
That is a very good info, I was looking for such a thing. |
Thanks for your pull request! This pull request appears to follow the contribution rules. › This message was automatically generated. |
dea55fe
to
41ed92d
Compare
Hi Marko, I did some more tweaks and have now a version that is substantially faster than before using your tests. Attached, you find the measurements.
|
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 looks good, please see an inline question. And maybe you could squash some of the commits? we usually try to squash the changes if we iterate on an improvement once we are done with it 😃
...ava/org/hibernate/validator/internal/engine/validationcontext/AbstractValidationContext.java
Show resolved
Hide resolved
7e41433
to
998f7ce
Compare
https://hibernate.atlassian.net/browse/HV-1928
does not fix the issue but reduces the calls to isSubPathOf by one.
Refactors isSubPathOf() to pathsSharePrefix() to make the semantic change clear.