Alexey, I propose to include [1] issue to the 2.9 release. Since this issue is related to the new master key change functionality which haven't been released yet I think it will be safe to cherry-pick commit to the release branch.
[1] https://issues.apache.org/jira/browse/IGNITE-13390 On Tue, 1 Sep 2020 at 12:13, Nikolay Izhikov <nizhi...@apache.org> wrote: > > Hello, Igniters. > > Alexey, please, include one more Python thin client fix [1] into the 2.9 > release > It fixes kinda major issue - "Python client returns fields in wrong order > since the 2 row when fields_count>10" > > [1] https://issues.apache.org/jira/browse/IGNITE-12809 > [2] > https://github.com/apache/ignite/commit/38025ee4167f05eaa2d6a2c5c2ab70c83a462cfc > > > 31 авг. 2020 г., в 19:23, Alexey Goncharuk <alexey.goncha...@gmail.com> > > написал(а): > > > > 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? > >>>> > >>> >
