Alexey, thanks, got it. I am not sure we can optimize anything out of the message factory with suppliers (at least I have no ideas right now), so most likely the only move here is to switch back to the switch approach somehow preserving the metrics part. Probably, inlining the Ignite messages to the IgniteMessageFactoryImpl should do the trick. Let me explore the code a bit.
P.S. I am surprised by the impact this part makes for the performance. Message creation is indeed on the hot path, but a single virtual call should not make that much of a difference given the amount of other work happening during the message processing. пн, 31 авг. 2020 г. в 18:33, Alex Plehanov <plehanov.a...@gmail.com>: > Alexey, sorry, I wrongly interpreted our benchmark results. Actually, we > were looking for a drop using bi-sect in the range between e6a7f93 (first > commit in the 2.9 branch after 2.8 branch cut) and 6592dfa5 (last commit in > the 2.9 branch). And we found these two problematic commits. > > Perhaps only IGNITE-13060 (Tracing) is responsible for a drop between > 2.8.1 and 2.9 (we have benchmarked 2.8.1 vs 2.9 with reverted IGNITE-13060 > now and performance looks the same) > > Ticket IGNITE-12568 (MessageFactory refactoring) is not related to drop > between 2.8.1 and 2.9, but still has some performance problem, and we can > win back IGNITE-13060 drop by this ticket. > > Do we need more investigation on IGNITE-13060 or we leave it as is? > > What should we do with IGNITE-12568 (MessageFactory refactoring)? > > пн, 31 авг. 2020 г. в 13:25, Alexey Goncharuk <alexey.goncha...@gmail.com > >: > >> Alexey, >> >> While investigating, I found that IGNITE-12568 has an incorrect fix >> version and is actually present in ignite-2.8.1 branch [1], so it cannot be >> the source of the drop against 2.8.1. >> >> P.S. Looks like we need to enforce a more accurate work with fix versions >> or develop some sort of tooling to verify the fix versions. >> >> --AG >> >> [1] >> https://github.com/apache/ignite/commit/3e492bd23851856bbd0385c6a419892d0bba2a34 >> >> пн, 31 авг. 2020 г. в 12:42, Alexey Goncharuk <alexey.goncha...@gmail.com >> >: >> >>> пт, 28 авг. 2020 г. в 11:16, Alex Plehanov <plehanov.a...@gmail.com>: >>> >>>> Guys, >>>> >>>> We have benchmarked 2.9 without IGNITE-13060 and IGNITE-12568 (reverted >>>> it >>>> locally) and got the same performance as on 2.8.1 >>>> >>>> IGNITE-13060 (Tracing) - some code was added to hot paths, to trace >>>> these >>>> hot paths, it's clear why we have performance drop here. >>>> >>>> IGNITE-12568 (MessageFactory refactoring) - switch/case block was >>>> refactored to an array of message suppliers. The message factory is on >>>> the >>>> hot path, which explains why this commit has an impact on total >>>> performance. >>>> I've checked JIT assembly output, done some JMH microbenchmarks, and >>>> found >>>> that old implementation of MessageFactory.create() about 30-35% faster >>>> than >>>> the new one. The reason - approach with switch/case can effectively >>>> inline >>>> message creation code, but with an array of suppliers relatively heavy >>>> "invokeinterface" cannot be skipped. I've tried to rewrite the code >>>> using >>>> an abstract class for suppliers instead of an interface (to >>>> replace "invokeinterface" with the "invokevirtual"), but it gives back >>>> only >>>> 10% of method performance and in this case, code looks ugly (lambdas >>>> can't >>>> be used). Currently, I can't find any more ways to optimize the current >>>> approach (except return to the switch/case block). Andrey Gura, as the >>>> author of IGNITE-12568, maybe you have some ideas about optimization? >>>> >>>> Perhaps we should revert IGNITE-12568, but there are some metrics >>>> already >>>> created, which can't be rewritten using old message factory >>>> implementation >>>> (IGNITE-12756). Guys, WDYT? >>>> >>> >>> Alexey, >>> >>> I see that IGNITE-12756 (metrics improvements) is already released in >>> Ignite 2.8.1 while IGNITE-12568 (message factory) is only present in Ignite >>> 2.9. Let's revert both IGNITE-12568 and whichever new metrics created for >>> 2.9 that depend on the new message factory to unblock the release and deal >>> with the optimizations in 2.10? >>> >>
