HV-2127 Make Path implementation not rely on Lists (or other collections)

[marko-bekhta]

https://hibernate.atlassian.net/browse/HV-2127

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on licensing, please check here.

---

[marko-bekhta]

Can be looked at by commit. The general idea here was to drop the use of the lists in the path implementation and just build the "chain" from the nodes.
One "downside" of this is that we do not have an easy way to iterate the path from the "root" as we only have the leaf node. If we add the root to the path, it won't help much since we
have the one-way links to the parent. I think adding the link to the leaf node won't help as we could have multiple leaves... and otherwise... reconstructing the whole path from the root to create a new leaf would probably take us back to where we were with the list ...

[gsmet]

One "downside" of this is that we do not have an easy way to iterate the path from the "root" as we only have the leaf node.

But do we need to? Or does the user need to?

[marko-bekhta]

and some numbers:

main:

Benchmark                                                                                                          Mode  Cnt      Score    Error   Units
o.h.v.p.cascaded.CascadedWithLotsOfItemsAndMoreConstraintsValidation.testCascadedValidationWithLotsOfItems        thrpt   20   9210.215 ± 54.555   ops/s
o.h.v.p.simple.SimpleValidation.testSimpleBeanValidation                                                          thrpt   20  17446.229 ± 95.293  ops/ms

Benchmark                                                                                                          Mode  Cnt      Score    Error   Units
o.h.v.p.cascaded.CascadedWithLotsOfItemsAndMoreConstraintsValidation.testCascadedValidationWithLotsOfItems        thrpt   20   9447.360 ± 28.172   ops/s
o.h.v.p.simple.SimpleValidation.testSimpleBeanValidation                                                          thrpt   20  15886.786 ± 39.997  ops/ms


change:
Benchmark                                                                                                          Mode  Cnt      Score    Error   Units
o.h.v.p.cascaded.CascadedWithLotsOfItemsAndMoreConstraintsValidation.testCascadedValidationWithLotsOfItems        thrpt   20  13766.536 ± 43.834   ops/s
o.h.v.p.simple.SimpleValidation.testSimpleBeanValidation                                                          thrpt   20  26797.112 ± 80.112  ops/ms

Benchmark                                                                                                          Mode  Cnt      Score    Error   Units
o.h.v.p.cascaded.CascadedWithLotsOfItemsAndMoreConstraintsValidation.testCascadedValidationWithLotsOfItems        thrpt   20  14434.296 ± 67.744   ops/s
o.h.v.p.simple.SimpleValidation.testSimpleBeanValidation                                                          thrpt   20  23922.016 ± 89.195  ops/ms

[marko-bekhta]

One "downside" of this is that we do not have an easy way to iterate the path from the "root" as we only have the leaf node.

But do we need to? Or does the user need to?

I hope not, and I hope others would just use the string representation of the path ... but it may be that some lib goes through the path to construct the "violation" .. I'm thinking about something like org.zalando:problem (https://github.com/zalando/problem) I'll take a look at it a bit later to see what's happening there 🙈

[marko-bekhta]

One "downside" of this is that we do not have an easy way to iterate the path from the "root" as we only have the leaf node.

But do we need to? Or does the user need to?

I hope not, and I hope others would just use the string representation of the path ... but it may be that some lib goes through the path to construct the "violation" .. I'm thinking about something like org.zalando:problem (https://github.com/zalando/problem) I'll take a look at it a bit later to see what's happening there 🙈

ok... I had a look at Quarkus and there are a few places where the path iterator is used to construct the response... e.g.:

https://github.com/quarkusio/quarkus/blob/ebee0a4b66d1b4dba9c51165e78d22fbf3b90d3e/extensions/hibernate-validator/runtime/src/main/java/io/quarkus/hibernate/validator/runtime/jaxrs/ResteasyReactiveViolationExceptionMapper.java#L59-L69

or here:

https://github.com/quarkusio/quarkus/blob/ebee0a4b66d1b4dba9c51165e78d22fbf3b90d3e/extensions/hibernate-validator/runtime/src/main/java/io/quarkus/hibernate/validator/runtime/interceptor/AbstractMethodValidationInterceptor.java#L155-L161

I could add some HV-specific methods to address these use cases... would Quarkus be able to leverage those?

[gsmet]

I could add some HV-specific methods to address these use cases... would Quarkus be able to leverage those?

I think I misunderstood you previously: I don't think you can break the contract of Path. It extends Iterable so iterator() should work.

Now the question is: can you somehow materialize the Path when we get a constraint violation? Because having a constraint violation is somehow a corner case. Most of the time, you shouldn't have one.

[gsmet]

I added a few comments.

FWIW, we could have a specific benchmark with only things that are valid and measure the improvements there.

Then have one where we have failures to measure that things have not gone too badly for this case.

[gsmet]

@marko-bekhta were you able to re-run the benchmarks with this new code?

[marko-bekhta]

yes. it seems still better than what we currently have 🙂 here's what I saved from Friday:

main:
Benchmark                                                     Mode  Cnt      Score    Error   Units
SimpleSingleElementValidation.invalidObjectValidation        thrpt   20   6474.390 ± 31.799  ops/ms
SimpleSingleElementValidation.validObjectValidation          thrpt   20  17840.565 ± 51.801  ops/ms

this patch:
Benchmark                                                     Mode  Cnt      Score     Error   Units
SimpleSingleElementValidation.invalidObjectValidation        thrpt   20   8527.579 ±  40.861  ops/ms
SimpleSingleElementValidation.validObjectValidation          thrpt   20  33352.222 ± 402.655  ops/ms

or you mean just the few latest commits with the MaterializedPath ?

(btw ^ this is from a "new benchmark" I've added with one simple entity being either valid or invalid. I noticed that some of our other benchmarks that use random can have a quite different results from one run to the other) so I wanted to have something more "stable")

it seems still better than what we currently have

which does make sense since we do not create/copy the lists on each operation, but just create one array for the failing constraint.

[gsmet]

(btw ^ this is from a "new benchmark" I've added with one simple entity being either valid or invalid. I noticed that some of our other benchmarks that use random can have a quite different results from one run to the other) so I wanted to have something more "stable")

Yeah, it makes perfect sense.

Looks all good to me if you want to merge it.

[marko-bekhta]

Thanks for having a look at this one, Guillaume! 🙏🏻 🙇🏻 🙂
yeah.. I should probably merge it before switching to the bean tracking PR so that there would be less problems with rebasing... I'll have a look at what Sonar suggests to fix and then merge 🙂

[sonarqubecloud]

Quality Gate passed

Issues
6 New issues
1 Accepted issue

Measures
0 Security Hotspots
84.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud