HV-1694 HV-1695 HV-1696 HV-1697 Reduce memory allocations #1020

gsmet · 2019-02-21T13:18:11Z

@marko-bekhta could you take a look at this one?

We had some reports of higher memory allocation rate when validating unconstrained beans (compared to old 5.2) and I can see how that could be, considering how we restructured the code: we would create a context even if not required at all.

That being said, benchmarks don't really say it's faster but I can see at least less allocations which is good.

I also modified some of the benchmarks so that they allocate less things in the benchmark itself. It avoids polluting the results.

Better reviewed commit per commit.

Note that there are a lot more things we could do to improve memory allocation rate when bootstrapping the metadata but it's less pressing so maybe one day :) - one thing I noticed is that we should cache getAnnotatedType() and getAnnotations() in the org.hibernate.validator.internal.properties.javabean classes. Not totally sure it's worth the bandwidth though so enough for one day.

It generates too much noise, in particular when testing unconstrained beans.

This way we can compare current to latest released 6.1

gsmet · 2019-02-21T15:11:03Z

@marko-bekhta I added one further improvements to ValidationContexts. This one makes a nice difference in the SimpleValidation.

I also added a hv-6.1 target so I would be interested if you could also get some numbers on your laptop for:

hv-5.2
hv-6.0
hv-6.1 (this is the latest alpha)
hv-current (mvn clean install engine before this one :))

for the SimpleValidation and the UnconstrainedBeanValidation benchmarks.

gsmet · 2019-02-21T15:22:17Z

engine/src/main/java/org/hibernate/validator/internal/metadata/BeanMetaDataManagerImpl.java

+			return beanMetaData;
+		}
+
+		beanMetaData = (BeanMetaData<T>) beanMetaDataCache.computeIfAbsent( beanClass,
 				bc -> createBeanMetaData( bc ) );


This one is interesting: this code creates a new instance of the lambda for each call so we end up creating a lot of lambda instances.

hmm... so it looks like the ConcurrentReferenceHashMap that we are using for this cache is actually using the default impl of computeIfAbsent. And that default one doesn't have any synchronised blocks. So I wonder if maybe we should change this last line to something like:

BeanMetaDataImpl<T> beanMetaData = createBeanMetaData( beanClass ); beanMetaDataCache.putIfAbsent( beanClass, beanMetaData ); return beanMetaData;

Right. So I switched to that but I get the previous value from putIfAbsent and return that if there is one.

gsmet · 2019-02-21T15:22:55Z

@gunnarmorling if you have some time, could you take a look at this one? Better reviewed commit per commit.

marko-bekhta

Nice improvements! I've added a few comments/questions inline. Now - benchmarks!

marko-bekhta · 2019-02-21T21:17:02Z

performance/pom.xml

@@ -245,7 +245,7 @@
            <properties>
                <validation-api.version>1.1.0.Final</validation-api.version>
                <beanvalidation-impl.name>Hibernate Validator</beanvalidation-impl.name>
-                <beanvalidation-impl.version>5.4.2.Final</beanvalidation-impl.version>
+                <beanvalidation-impl.version>5.4.3.Final</beanvalidation-impl.version>
            </properties>
            <dependencies>
                <dependency>


Cannot leave a comment on a renamed file below. Not sure why SimpleComposingConstraintValidation got moved to bv2 sources ?

Because it's using BV 2 constraints :).

I had an issue when trying to build with BV 1.1 and couldn't find nice BV 1.1 constraints so I just decided to move it as it's more a test to validate recent performance improvements.

marko-bekhta · 2019-02-21T21:28:05Z

engine/src/main/java/org/hibernate/validator/internal/engine/ValidatorImpl.java

+			return Collections.emptySet();
+		}
+
+		BaseBeanValidationContext<T> validationContext = getValidationContextBuilder().forValidate( rootBeanClass, rootBeanMetaData, object );

 		if ( !validationContext.getRootBeanMetaData().hasConstraints() ) {


this one is probably redundant now as we already checked for constraints on line 158.

Ah yes, sure it was supposed to be removed, missed it, will fix.

Good catch, there were a few other glitches. I fixed them all, hopefully.

marko-bekhta · 2019-02-21T21:57:33Z

engine/src/main/java/org/hibernate/validator/internal/metadata/BeanMetaDataManagerImpl.java

+			return beanMetaData;
+		}
+
+		beanMetaData = (BeanMetaData<T>) beanMetaDataCache.computeIfAbsent( beanClass,
 				bc -> createBeanMetaData( bc ) );


hmm... so it looks like the ConcurrentReferenceHashMap that we are using for this cache is actually using the default impl of computeIfAbsent. And that default one doesn't have any synchronised blocks. So I wonder if maybe we should change this last line to something like:

BeanMetaDataImpl<T> beanMetaData = createBeanMetaData( beanClass ); beanMetaDataCache.putIfAbsent( beanClass, beanMetaData ); return beanMetaData;

…Impl It significantly reduces memory allocation as it avoids the creation of a lambda instance.

marko-bekhta · 2019-02-21T23:01:29Z

Here's the results.. Some of them are a bit unexpected (6.0 unconstrained):

hv-current:
# Run complete. Total time: 00:00:36

Benchmark                                                     Mode  Cnt     Score     Error  Units
SimpleValidation.testSimpleBeanValidation                    thrpt   20  3580.428 ± 250.562  ops/ms
UnconstrainedBeanValidation.testUnconstrainedBeanValidation  thrpt   20  7065.427 ± 107.262  ops/ms

6.1.alpha
# Run complete. Total time: 00:00:37

Benchmark                                                     Mode  Cnt     Score     Error  Units
SimpleValidation.testSimpleBeanValidation                    thrpt   20  3176.884 ± 219.034  ops/ms
UnconstrainedBeanValidation.testUnconstrainedBeanValidation  thrpt   20  7429.412 ± 114.166  ops/ms

6.0
# Run complete. Total time: 00:00:37

Benchmark                                                     Mode  Cnt     Score     Error  Units
SimpleValidation.testSimpleBeanValidation                    thrpt   20  2799.963 ± 76.101  ops/ms
UnconstrainedBeanValidation.testUnconstrainedBeanValidation  thrpt   20  9013.694 ± 95.510  ops/ms
UnconstrainedBeanValidation.testUnconstrainedBeanValidation  thrpt   20  7882.279 ± 104.074  ops/ms

5.4
# Run complete. Total time: 00:00:30

Benchmark                                                     Mode  Cnt     Score     Error  Units
SimpleValidation.testSimpleBeanValidation                    thrpt   20  334.601  ± 6.362    ops/ms
UnconstrainedBeanValidation.testUnconstrainedBeanValidation  thrpt   20  7126.291 ± 102.859  ops/ms

5.2
# Run complete. Total time: 00:00:31

Benchmark                                                     Mode  Cnt     Score     Error  Units
SimpleValidation.testSimpleBeanValidation                    thrpt   20  337.434  ± 10.004   ops/ms
UnconstrainedBeanValidation.testUnconstrainedBeanValidation  thrpt   20  7541.114 ± 123.081  ops/ms

gsmet · 2019-02-21T23:03:49Z

Yeah I had the same sort of issues for the unconstrained benchmark. I don't know what's going on exactly.

gsmet · 2019-02-21T23:08:02Z

I just run it again and I have:

hv-current

Result "org.hibernate.validator.performance.unconstrained.UnconstrainedBeanValidation.testUnconstrainedBeanValidation":
  8544.618 ±(99.9%) 90.401 ops/ms [Average]
  (min, avg, max) = (8373.998, 8544.618, 8684.439), stdev = 104.106
  CI (99.9%): [8454.217, 8635.019] (assumes normal distribution)

hv-6.0

Result "org.hibernate.validator.performance.unconstrained.UnconstrainedBeanValidation.testUnconstrainedBeanValidation":
  7794.353 ±(99.9%) 128.405 ops/ms [Average]
  (min, avg, max) = (7591.621, 7794.353, 8221.196), stdev = 147.872
  CI (99.9%): [7665.948, 7922.758] (assumes normal distribution)

Which JDK do you use to run the benchmarks? I use a good old JDK 8.

gsmet · 2019-02-21T23:13:42Z

And with JDK 11.0.1, it's significantly slower, especially hv-current which then get slower than hv-6.0.

marko-bekhta · 2019-02-21T23:14:46Z

Just tried it again with your latest change:
hv-current:

Result "org.hibernate.validator.performance.unconstrained.UnconstrainedBeanValidation.testUnconstrainedBeanValidation":
  6913.940 ±(99.9%) 170.225 ops/ms [Average]
  (min, avg, max) = (6747.430, 6913.940, 7360.837), stdev = 196.031
  CI (99.9%): [6743.715, 7084.165] (assumes normal distribution)

6.0:

Result Result "org.hibernate.validator.performance.unconstrained.UnconstrainedBeanValidation.testUnconstrainedBeanValidation":
  8164.377 ±(99.9%) 240.792 ops/ms [Average]
  (min, avg, max) = (7777.431, 8164.377, 8744.066), stdev = 277.296
  CI (99.9%): [7923.585, 8405.168] (assumes normal distribution)

And I'm on VM version: JDK 1.8.0_161, VM 25.161-b12

gsmet · 2019-02-21T23:19:43Z

This is weird to say the least because it definitely does less work. Could you check what's going on with a profiler? Not sure it will give interesting results though :/.

Wondering if maybe the method was inlined and that it's now too big to be inlined.

gsmet · 2019-02-22T11:26:14Z

@marko-bekhta I pushed an additional commit pursuing the idea of inlining.

It seems to improve things here (but tbh, it's not very stable).

marko-bekhta · 2019-02-22T14:10:34Z

Yeah I tried these last changes as well. And I cannot explain the behavior. I run benchmarks on different env today. When I tried them with 1.8.0_111 they gave expected results, where hv-current is slightly better than hv-6.0. Then when I tried to reproduce the results from yesterday I though I'll update to latest build - 1.8.0_201. With this version it started to show slower results for hv-current. But when I tried to profile it with VisualVm the iteration results would become similar between hv-current and hv-6.0. In profiling results the only thing to notice is that constructor of ValidationContext is not called in hv-current (which is expected).

gsmet · 2019-02-22T14:21:37Z

OK, so you have weird results even with this very last commit: 581be9a ?

marko-bekhta · 2019-02-22T14:56:06Z

Yes

gsmet · 2019-02-27T16:31:15Z

@marko-bekhta so I discussed with @barreiro from our performance team and it appears our usage of Random was flawed. When several threads access a Random instance, it can become quite slow. And the faster you are, the more constrained you get, explaining why we ended up with worst results.

I changed our code to use ThreadLocalRandom.

With 6.1:

Benchmark                                                     Mode  Cnt      Score     Error   Units
UnconstrainedBeanValidation.testUnconstrainedBeanValidation  thrpt   20  10662.005 ± 142.829  ops/ms

With current:

Result "org.hibernate.validator.performance.unconstrained.UnconstrainedBeanValidation.testUnconstrainedBeanValidation":
  231520.643 ±(99.9%) 1596.706 ops/ms [Average]
  (min, avg, max) = (227430.245, 231520.643, 234223.436), stdev = 1838.769
  CI (99.9%): [229923.937, 233117.349] (assumes normal distribution)

So the new code appears to be 20 times faster on my laptop.

Could you check you have a similar tendency?

marko-bekhta · 2019-02-27T16:37:38Z

Hi @gsmet yes, I just saw that you've pushed new changes and run the benchmarks right away 😄 . My results are similar:

hv-curr
# Run complete. Total time: 00:00:41

Benchmark                                                     Mode  Cnt      Score      Error   Units
UnconstrainedBeanValidation.testUnconstrainedBeanValidation  thrpt   20  88921.833 ± 1115.665  ops/ms
Benchmark                                   Mode  Cnt     Score    Error   Units
SimpleValidation.testSimpleBeanValidation  thrpt   20  1867.365 ± 39.177  ops/ms


hv-6.0
# Run complete. Total time: 00:00:43

Benchmark                                                     Mode  Cnt      Score     Error   Units
UnconstrainedBeanValidation.testUnconstrainedBeanValidation  thrpt   20  13163.132 ± 258.242  ops/ms
Benchmark                                   Mode  Cnt     Score    Error   Units
SimpleValidation.testSimpleBeanValidation  thrpt   20  1032.561 ± 17.536  ops/ms

Nice that we have an explanation of that behavior.

HV-1694 Add a benchmarck for unconstrained beans and update benchmarks

3f7b519

gsmet changed the title ~~HV-1694 Reduce memory allocation for unconstrained beans~~ HV-1694 HV-1695 HV-1696 Reduce memory allocations Feb 21, 2019

gsmet added 4 commits February 21, 2019 15:59

HV-1694 Reduce the memory allocations of the benchmarks

3c165a8

It generates too much noise, in particular when testing unconstrained beans.

HV-1694 Add a benchmark entry for 6.1

c7f372c

This way we can compare current to latest released 6.1

HV-1694 Reduce memory allocation for unconstrained beans

9464463

HV-1695 Avoid creating an empty map for group conversions

115d72e

gsmet force-pushed the HV-1694 branch 3 times, most recently from efbddab to 3c49312 Compare February 21, 2019 15:07

gsmet requested a review from marko-bekhta February 21, 2019 15:11

gsmet commented Feb 21, 2019

View reviewed changes

gsmet changed the title ~~HV-1694 HV-1695 HV-1696 Reduce memory allocations~~ HV-1694 HV-1695 HV-1696 HV-1697 Reduce memory allocations Feb 21, 2019

gsmet added the Ready for review label Feb 21, 2019

marko-bekhta reviewed Feb 21, 2019

View reviewed changes

gsmet added 2 commits February 21, 2019 23:46

HV-1696 Avoid using computeIfAbsent every time in BeanMetaDataManager…

57dda43

…Impl It significantly reduces memory allocation as it avoids the creation of a lambda instance.

HV-1697 Reduce memory allocation in ValidationContexts

22fd3d5

gsmet force-pushed the HV-1694 branch from 3c49312 to 22fd3d5 Compare February 21, 2019 22:52

gsmet force-pushed the HV-1694 branch from 581be9a to f75fd01 Compare February 27, 2019 16:17

HV-1694 Fix our benchmarks following a discussion with Luis Barreiro

077827f

gsmet force-pushed the HV-1694 branch from f75fd01 to 077827f Compare February 27, 2019 16:23

gsmet merged commit 077827f into hibernate:master Feb 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HV-1694 HV-1695 HV-1696 HV-1697 Reduce memory allocations #1020

HV-1694 HV-1695 HV-1696 HV-1697 Reduce memory allocations #1020

gsmet commented Feb 21, 2019 •

edited

gsmet commented Feb 21, 2019 •

edited

gsmet Feb 21, 2019

marko-bekhta Feb 21, 2019

gsmet Feb 21, 2019

gsmet commented Feb 21, 2019

marko-bekhta left a comment

marko-bekhta Feb 21, 2019

gsmet Feb 21, 2019

marko-bekhta Feb 21, 2019

gsmet Feb 21, 2019

gsmet Feb 21, 2019

marko-bekhta Feb 21, 2019

marko-bekhta commented Feb 21, 2019

gsmet commented Feb 21, 2019

gsmet commented Feb 21, 2019

gsmet commented Feb 21, 2019

marko-bekhta commented Feb 21, 2019

gsmet commented Feb 21, 2019 •

edited

gsmet commented Feb 22, 2019

marko-bekhta commented Feb 22, 2019

gsmet commented Feb 22, 2019

marko-bekhta commented Feb 22, 2019

gsmet commented Feb 27, 2019

marko-bekhta commented Feb 27, 2019

HV-1694 HV-1695 HV-1696 HV-1697 Reduce memory allocations #1020

HV-1694 HV-1695 HV-1696 HV-1697 Reduce memory allocations #1020

Conversation

gsmet commented Feb 21, 2019 • edited

gsmet commented Feb 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet commented Feb 21, 2019

marko-bekhta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marko-bekhta commented Feb 21, 2019

gsmet commented Feb 21, 2019

gsmet commented Feb 21, 2019

gsmet commented Feb 21, 2019

marko-bekhta commented Feb 21, 2019

gsmet commented Feb 21, 2019 • edited

gsmet commented Feb 22, 2019

marko-bekhta commented Feb 22, 2019

gsmet commented Feb 22, 2019

marko-bekhta commented Feb 22, 2019

gsmet commented Feb 27, 2019

marko-bekhta commented Feb 27, 2019

gsmet commented Feb 21, 2019 •

edited

gsmet commented Feb 21, 2019 •

edited

gsmet commented Feb 21, 2019 •

edited