Hi Max and Rene, thanks for this work!
I merged it locally, which worked as expected with svn patch. I did run the maven build locally, and no errors were reported. When I inspect the implementation of a strategy, e.g. in the generated torque-templates/target/generated-sources org.apache.torque.test.BaseCRecordMapper class in the method processRow Previously (release 7), if criteria was not null the strategy (if set) gets applied. Reading the code two things are now explicitely: (1) if criteria is a composite or (2) columns without offset is empty (that is all columns do have an offset) processRow returns a null object. (1) may or may be correct, I am not sure. (2) I stumbled over the name columnsWithoutOffset. If I check the filtering skip(rowOffset) will only return an empty list if rowOffset is greater than the number of the selected columns. This might be almost an error, if not a warning IMO. Could we find a better name for the variable columnsWithoutOffset (might be columnsAfterOffset or shiftedColumns)? Concerning backward compatibility: This seems only an issue, if you want to use the Torque classes without regenerating the sources. But that's true, with the current patch this will fail. We could do a null check of the mappingStrategy field in ResultsetSpliterator tryAvance method and then calling the default processRow (which would in an old recordMapper code call a new MappingStrategy in the removed method initStrategy method - this may then be again backward comnpatible. Another backward issue would be that the Mappingstrategy method finish before had two parameters. We might resolve this latter issue also with a dummy method (we failed to declare an interface before, which had showed us this immediately 😉 Nevertheless I think, we should merge it soon into the trunk and provide later some documentation or updates. We may wait a little bit more time (until next week or end of month?) to get some more considerations 😉 Have a nice weekend! Best regards, Georg -----Ursprüngliche Nachricht----- Von: Max Philipp Wriedt <[email protected]> Gesendet: Mittwoch, 15. Oktober 2025 21:52 An: [email protected] Betreff: Thread-safe record mapper speed-ups Hi, after initially reporting a n^2 problem in the processRow method of RecordMappers in TORQUE-364, fixing it - only to then find our suspected concurrency troubles to be confirmed in TORQUE-372 - we can (hopefully) report a far more robust solution. Looking into the SQL result stack we would propose to re-introduce a slightly modified MappingStrategy. The key insight to fixing the concurrent modification issue lies in doing so at the ResultsetSpliterator level. This ensures that every query gets its own MappingStrategy. Doing this would also require two new methods for every RecordMapper to generate a fitting MappingStrategy and to process a row with a potential strategy. To keep backwards compatibility both would be defaulted in the interface to do nothing and forward to the old method respectively. We have already implemented a test of this locally and can see similar performance to the old, but flawed, version and are sharing the patch here. (Hopefully this actually works...) However, because the patch changes the methods the ResultsetSpliterator expects a RecordMapper to have (by calling a generateStrategy method to either do just that or return null) this would break backwards compatibility with old RecordMappers and we wanted to a feedback cycle early. Best regards, Max & Rene
smime.p7s
Description: S/MIME cryptographic signature
