Skip property population if entity can be constructed entirely using a full constructor [DATAJDBC-546]

[spring-projects-issues]

MyeongHyeonLee opened DATAJDBC-546 and commented

Improves the performance of the BasicJdbcConverter$ReadingContext populateProperties.

In populateProperties, PreferredConstructor checks to populate only the property, not the constructor parameter.

However, in a concurrency situation, PreferredConstructor#isConstructorParameter is CPU intensive and causes delays.

 

In fact, when the concurrent traffic was high in the service using Spring Data JDBC, the CPU usage increased a lot. As a result of profiling, PreferredConstructor#isConstructorParameter was using a lot of CPU. (Related issue and profiling: https://jira.spring.io/browse/DATACMNS-1706)

 

In my case, I have 15 columns mapped to a property in one table, and 120 row ResultSets in one SQL. Hit PreferredConstructor#isConstructorParameter method 1800(15*120) times in one request.

At 600 TPS, there is a delay as CPU usage increases. (Hit 1,080,000 per 1 sec)

 

Without calling PreferredConstructor#isConstructorParameter, it can improve to populate by filtering only the properties that are not used in Constructor.

 

Simply compare the delay time with a benchmark and you will see results that improve performance. (https://github.com/mhyeon-lee/spring-data-jdbc/pull/1)

 

 

---

Affects: 2.0 GA (Neumann)

Issue Links:

• DATACMNS-1706 PreferredConstructor needs to improve bottleneck occurrence

Referenced from: pull request #220

Backported to: 2.0.1 (Neumann SR1), 1.1.8 (Moore SR8)

[spring-projects-issues]

Mark Paluch commented

Since we intend to improve the implementation of isConstructorParameter, there's little value in tweaking the individual store implementations

[spring-projects-issues]

MyeongHyeonLee commented

Mark Paluch

It is important to improve isConstructorParameter, but I think that it is necessary to improve it by invoking many unnecessary calls.

After the constructor object is created, all properties call isConstructorParameter.

[Spring Data JDBC|https://github.com/spring-projects/spring-data-jdbc/blob/8e630841624e93b9261557430540b5fe7dd8c529/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/BasicJdbcConverter.java#L403]

[Spring Data Redis|https://github.com/spring-projects/spring-data-redis/blob/2bcb06acc88ee23d2d355568ed96ca303d9f86a3/src/main/java/org/springframework/data/redis/core/convert/MappingRedisConverter.java#L235]

[Spring Data MongoDB|https://github.com/spring-projects/spring-data-mongodb/blob/41bb2ed4bfd3c6f5b200181785fe74de485a13be/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/DtoInstantiatingConverter.java#L98]

This is an unnecessary call to any object that implements  AllArgsConstructor.

 

In my opinion,

I suggest that PersistentEntity should provide a list of properties excluding ConstructorParameter as cached.

 

Then PersistentEntity doesn't need to consider concurrency after initialization at startup.

 

[spring-projects-issues]

Mark Paluch commented

Thanks for your feedback myeonghyeonLee.

Closing this ticket since the underlying performance contention issue is fixed in Spring Data Commons (DATACMNS-1706).

Feel free to leave a comment if there's still something that we can optimize in a meaningful way

[spring-projects-issues]

Mark Paluch commented

Sorry, I closed the ticket before realizing that we can add another optimization which skips property population (as mentioned) if that's not necessary. MongoDB already checks for entity.requiresPropertyPopulation() and that's what we can do here as well

[spring-projects-issues]

Mark Paluch commented

That's fixed now. The submitted pull request changed too many things in a way that are not consistent with how Spring Data handles object materialization hence closing it without a merge. Thanks for your contribution though

[spring-projects-issues]

MyeongHyeonLee commented

Thank you for considering.

I expect to be able to optimize it in a better way.