mybatis / mybatis-3

MyBatis SQL mapper framework for Java
http://mybatis.github.io/mybatis-3/
Apache License 2.0
19.72k stars 12.82k forks source link

#101: Add support for collection constructor creation #3108

Open epochcoder opened 6 months ago

epochcoder commented 6 months ago

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

coveralls commented 6 months ago

Coverage Status

coverage: 87.257% (+0.09%) from 87.169% when pulling 7d36682856085f9d06a83bd3e7f3224ea3243327 on epochcoder:feature/101-support-constructor-collection-injection into 043270b83a01162d50a05ae1656ee57f57336a88 on mybatis:master.

epochcoder commented 6 months ago

Add_debugging_info_for_DefaultResultHandler.patch

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

Breus commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

epochcoder commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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

harawata commented 5 months ago

@epochcoder ,

Not at the moment.

harawata commented 5 months ago

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 commented 5 months ago

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?

GeorgeSalu commented 4 months ago

@epochcoder Does it work with java 16 record https://www.guiadojava.com.br/2021/04/java-records.html ?

epochcoder commented 4 months ago

@GeorgeSalu Indeed it does, I converted the test cases to use records in the attached patch, and it worked out of the box.

101_use_records.patch

Imran-imtiaz48 commented 3 months ago

101 Add Support for Constructor Collection Injection

MyBatis now supports creating completely immutable objects, including nested collections.

To achieve this, another meta object is maintained in memory until it is ready for creation. This detection is based on CacheKey.

This feature is hidden behind a configuration flag called experimentalConstructorCollectionMapping. By default, this flag is set to false, so MyBatis retains its current behavior unless the flag is enabled. This approach allows the community to test different use cases and report possible issues without affecting current workloads.

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

I have also added debug symbols as an additional patch, which helps in understanding the process.

Remaining Tasks:

gewoonrik commented 2 weeks ago

This PR would be very helpful for me! Is there any update?

hazendaz commented 1 week ago

This PR would be very helpful for me! Is there any update?

sounds like this was candidate for mybatis 4.0.0. So I don't know when it would get into the project. At the moment, those of us working on mybatis are somewhat spread thin with other projects. We are raising up all of mybatis to java 17 soon and doing a small update to 11 from 8 currently. Also addressing some longer standing issues and that all takes a bit of extra time. I'm pretty sure we will have one more java 8 release on the core here before the jump to 11 and then we have a good number of repos to sync up then jump to 17 before we get to this. So it could be a good number of months from now. Its also been a while since anyone looked at this and there are a lot of files so that will again take some time. Sorry for the delay this causes.