#101: Add support for collection constructor creation

[epochcoder]

#101 Add support for constructor collection injection

It is now possible to create completely immutable objects (including nested collections) with MyBatis.

This has been done by keeping another meta object in memory until it is ready for creation, the detection for this is based on CacheKey.

This functionality is fully hidden behind a configuration flag called experimentalConstructorCollectionMapping, the idea is that MyBatis will have exactly the same (current) behaviour when this flag is not set (default false) and the community can enable this to test different use cases and report possible issues.

This would allow us to integrate this behaviour while gathering community feedback while not affecting any current workloads.

New functionality is tested in src/test/java/org/apache/ibatis/immutable/ImmutableConstructorTest.java.

I added debug symbols as an additional patch which really helps understanding what is happening.

What still needs needs to be done

• Documentation
• Testing with more than two levels of nesting (currently Blog -> Post > [Comment + Tag])
  • See ImmutableCollectionConstructorTest.java - House -> Room -> Furniture -> Defect
• Testing result ordering
  • We cannot reliably determine when to build the final object if the results are not ordered, so we require this attribute
• Testing with multiple result sets (throwing exception now)
  • Descoping this for now. could create follow up.
• Testing with mixed mappings (including nestedQuery)
  • See postForConstructorInit in BoundBlogMapper.xml
• Testing with very large resultSets (performance)

[coveralls]

coverage: 87.205% (+0.09%) from 87.116%
when pulling c564187 on epochcoder:feature/101-support-constructor-collection-injection
into 9e3e008 on mybatis:master.

[epochcoder]

Add_debugging_info_for_DefaultResultHandler.patch

Here is the patch to add debug symbols and print lines to see what is happening visually

[Breus]

This would be awesome to have as it is currently the main reason why MyBatis entity objects often can't be immutable Java objects in our code base.

[harawata]

Thank you for the PR, @epochcoder ,

I will take a look, but it may take some time as I don't have much spare time right now.
Please try to eliminate unrelated changes like formatting, import order, etc..

[epochcoder]

Thank you for your feedback @harawata! I completely understand, take your time. Ill make sure to focus on the relevant changes and eliminate any unrelated ones (most of them are as a result of format & impsort plugins), Ill try to revert it to make the PR easier to review. Your input on this is very valuable!

[epochcoder]

@harawata I've removed all unnecessary formatting. I left the headers in tact (2023 -> 2024) please let me know in case you would like me to revert that as well! ;-)

[epochcoder]

FWIW, I tested this change on a large part of our stack which includes 10's of thousands of mapped statements, both with the experimentalConstructorCollectionMapping enabled, and disabled.

However, given that this code activates an entirely new path which used to fail, there are no current use cases which actively invoke it, and by extension, any other users of MyBatis.

But from a backward compatibility point of view, this is solid.

[hazendaz]

@harawata I'm thinking we cut a last 3.5.x release, move this to 3.6.x and then officially move to java 11 or maybe even 17. WDYT?

[epochcoder]

I wrote a rudimentary performance test for this (not using JMH)

The setup is 100 houses (main rows) with 200 rooms subdivided between them, with 50 furniture objects subdivided among those rooms, and 10 defects spread among them.

I then created a identical result maps for both property and constructor injection retrieving the same data.
Each retrieval uses a new sql session.

The results from what I could gather were that using constructor injection (this functionality) is very slightly slower than using property injection.

Please let me know if you would like me to add this setup as another commit (though i'm leaning a bit against it as this PR is huge already).

The results from the following code is:

 Per iteration:
    Property:2.488ms
    Constructor:3.0828ms
 over 10_000 iterations with a warmup of 50 cycles

Ill attach the patch if anyone wants to check it out (based on this branch)

101-perf-test.patch

PS: This is the core of the measurement code:

    for (int i = 0; i < iterations * 2; i++) {
      if (random.nextInt() % 2 == 0) {
        try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
          final HouseMapper mapper = sqlSession.getMapper(HouseMapper.class);
          final long startTime = System.currentTimeMillis();
          assertThat(mapper.getAllHouses()).isNotNull().hasSize(100);
          times.get(HouseMapper.class).addAndGet(System.currentTimeMillis() - startTime);
        }
      } else {
        try (SqlSession sqlSession = sqlSessionFactory.openSession()) {
          final ImmutableHouseMapper mapper = sqlSession.getMapper(ImmutableHouseMapper.class);
          final long startTime = System.currentTimeMillis();
          assertThat(mapper.getAllHouses()).isNotNull().hasSize(100);
          times.get(ImmutableHouseMapper.class).addAndGet(System.currentTimeMillis() - startTime);
        }
      }
    }

[epochcoder]

I managed to improve the performance past the point of the property test baseline, Im doing this by only verifying we can create the result once, (per result set) as we can reasonably assume that the next row would have the exact same mapping and thus would not fail if the first verification did not fail.

Im adding two more commits, one with the improvement and the second with the performance test itself. Please let me know if this is too much and I should rather remove the latter.

The basic test also now verifies that the output of property- and constructor based mapping with collections are identical.

EDIT

I wanted to be a bit more scientific about this and used a JMH Benchmark (code on a separate branch based on this one), here are the results:

Benchmark                                                                        Mode  Cnt  Score   Error  Units
PropertyVsConstructorInjectionBenchmark.retrieveAllUsingConstructorInjection     avgt    5  1.015 ± 0.023  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveAllUsingPropertyInjection        avgt    5  1.213 ± 0.069  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveSingleUsingConstructorInjection  avgt    5  0.049 ± 0.002  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveSingleUsingPropertyInjection     avgt    5  0.039 ± 0.001  ms/op

[harawata]

Hi @epochcoder ,

I'm sorry, but could you add new set(s) of test classes/resources instead of modifying existing ones?

The existing tests should pass as-is after the changes in this PR are applied.
I understand why you did that, but if you move/modify existing files, it's difficult to verify that.

Thank you!

[epochcoder]

Hey @harawata, are you referring to the test related to BoundBlogMapper (postConstructorForInit) in BindingTest?

I changed it because the original test itself was flawed in the sense that it was impossible to make it work with the new functionality.

So I had to make the correct resultMap for it, I then changed the test to assert an exception when the flag is false, which is what happened anyway (it was disabled) then made a copy of it with the flag enabled.

But, if this is not the one you are referring to, could you please let me know which ones, and I can do my best to change / rewrite it ;)

[harawata]

Oops, sorry, I was diff-ing wrong commits.
Please disregard my last comment.

[epochcoder]

Oops, sorry, I was diff-ing wrong commits.
Please disregard my last comment.

No worries, I understand this PR is absolutely huge, and I'll do my best to make it easier to review ;-)

[epochcoder]

I forgot to actually cache the value of the experimentalConstructorCollectionMapping flag while building the ResultMap (similar to hasNestedResultMaps and hasNestedQueries), After doing this, performance has increased even more ;-)

Additionally, it does not affect the default case so much anymore.

Benchmark                                                                        Mode  Cnt  Score   Error  Units
PropertyVsConstructorInjectionBenchmark.retrieveAllUsingConstructorInjection     avgt    5  0.834 ± 0.015  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveAllUsingPropertyInjection        avgt    5  1.164 ± 0.055  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveSingleUsingConstructorInjection  avgt    5  0.045 ± 0.001  ms/op
PropertyVsConstructorInjectionBenchmark.retrieveSingleUsingPropertyInjection     avgt    5  0.038 ± 0.002  ms/op

[epochcoder]

@harawata @hazendaz Is there anything else I can/should do for this PR?

[harawata]

@epochcoder ,

Not at the moment.

[harawata]

I finally found some time to review this, but it's not exactly ready-to-merge state.
I'll revisit this as part of v4 enhancements later.

[epochcoder]

Thanks @harawata , would you mind leaving some comments so I could get it into the desired state, or is that something you prefer we do later?