To be honest, Pavel, your benchmark is not quite correct. Please, rewrite it using BlackHole
чт, 9 сент. 2021 г. в 10:28, Nikolay Izhikov <nizhi...@apache.org>: > +1 to ban Streams usage. > > > > > 9 сент. 2021 г., в 02:59, Valentin Kulichenko < > valentin.kuliche...@gmail.com> написал(а): > > > > Pavel, > > > > Quite frankly, I think we used to lean into performance too much. We > > generally preferred it over data consistency, project modularity and code > > readability. Performance, of course, plays a very important rule in > Ignite, > > but it's possible to overdo anything. > > > > There are certainly parts of the project that can benefit from features > > like Stream API, without significant concern over performance. CLI is an > > obvious example, but I'm sure it's not the only one. > > > > That said, I don't think that banning something is productive. At the > same > > time, we should make sure we pay more attention to performance during > > reviews. Maybe we should have a checklist of major things to look for? > Not > > as a set of strict rules, but more as a guideline for contributors and > > committers. > > > > We might also want to start benchmarking the code and tracking the > progress. > > > > -Val > > > > On Wed, Sep 8, 2021 at 12:09 PM Pavel Tupitsyn <ptupit...@apache.org> > wrote: > > > >> Alexander, Ivan, > >> > >>> not very productive to assume that 100% of your code is > >>> on the hot path > >> > >> That is exactly what we should be doing. > >> When I joined Ignite community 6 years ago, this was the prevalent > mindset. > >> > >> I'm not sure which part of Ignite can be considered "not on a hot path". > >> Create/alter table (mentioned above) should perform well too. > >> > >>> measured first and only then optimized > >>> https://wiki.c2.com/?OptimizeLater > >> > >> Extra allocations are a "death by a thousand cuts". > >> They add up here and there, and then there are GC pauses. > >> This would be hard to "optimize later". > >> > >> It is common for perf-oriented projects to avoid some techniques. > >> For example, LINQ (streams analog from C# with similar perf issues) is > >> avoided in libraries and compilers [1]. > >> > >> [1] https://github.com/dotnet/roslyn/blob/main/CONTRIBUTING.md > >> > >> On Wed, Sep 8, 2021 at 9:49 PM Valentin Kulichenko < > >> valentin.kuliche...@gmail.com> wrote: > >> > >>> I don't think we should ban anything. Streams API is just a tool in the > >>> toolbox - it should be used appropriately. It's up to the contributor > and > >>> reviewer(s) to identify whether a particular usage might cause > >> performance > >>> issues. > >>> > >>> -Val > >>> > >>> On Wed, Sep 8, 2021 at 8:01 AM Alexander Polovtcev < > >>> alexpolovt...@gmail.com> > >>> wrote: > >>> > >>>> -1 > >>>> I think that it is not very productive to assume that 100% of your > code > >>> is > >>>> on the hot path, it would be impossible to write and maintain. Humans > >> are > >>>> not very good at guessing where the performance bottlenecks are, so > the > >>>> performance of the possible hot paths should be measured first and > only > >>>> then optimized and documented. > >>>> > >>>> On Wed, Sep 8, 2021 at 5:53 PM Ivan Pavlukhin <vololo...@gmail.com> > >>> wrote: > >>>> > >>>>> Does not this trivial strategy work for us? > >>>>> https://wiki.c2.com/?OptimizeLater > >>>>> > >>>>> 2021-09-08 13:52 GMT+03:00, Andrey Gura <ag...@apache.org>: > >>>>>> Agree that any additional object creation on a hot path could be a > >>>>>> problem. So hot path should not contain stream API and any other > >>>>>> potentially problem code (e.g. iterator instead of for by index). > >>>>>> > >>>>>> On Wed, Sep 8, 2021 at 1:45 PM Pavel Tupitsyn < > >> ptupit...@apache.org> > >>>>> wrote: > >>>>>>> > >>>>>>> Ok, maybe a total ban is overkill, but right now streams are used > >>> even > >>>>> on > >>>>>>> some hot paths like getAllAsync [1]. > >>>>>>> > >>>>>>> Another issue is that Collectors.toList() and other variants don't > >>>>> accept > >>>>>>> capacity, and we end up with unnecessary reallocations of > >> underlying > >>>>>>> arrays. > >>>>>>> > >>>>>>> [1] > >>>>>>> > >>>>> > >>>> > >>> > >> > https://github.com/apache/ignite-3/blob/1d7d703ff2b18234b15a9a7b03104fbb73388edf/modules/table/src/main/java/org/apache/ignite/internal/table/KVBinaryViewImpl.java#L83 > >>>>>>> > >>>>>>> On Wed, Sep 8, 2021 at 1:06 PM Konstantin Orlov < > >>> kor...@gridgain.com> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> Hi! > >>>>>>>> > >>>>>>>> Agree with Ivan that it’s an overkill. Code readability and > >>>>>>>> maintainability should have > >>>>>>>> the same priority as the performance (with some exceptions of > >>>> course). > >>>>>>>> > >>>>>>>> BTW the result of your benchmark looks quite strange. The > >>>> performance > >>>>>>>> penalty on > >>>>>>>> my laptop (Core i7 9750H, 6 cores up to 4.50 GHz) is 25%, not 8 > >>>> times: > >>>>>>>> > >>>>>>>> Benchmark Mode Cnt Score Error > >>>>>>>> Units > >>>>>>>> JmhIncrementBenchmark.loopSum thrpt 10 32347.819 ± 676.548 > >>>>>>>> ops/ms > >>>>>>>> JmhIncrementBenchmark.streamSum thrpt 10 24459.196 ± 610.152 > >>>>>>>> ops/ms > >>>>>>>> > >>>>>>>> > >>>>>>>> -- > >>>>>>>> Regards, > >>>>>>>> Konstantin Orlov > >>>>>>>> > >>>>>>>> > >>>>>>>>> On 8 Sep 2021, at 12:23, Ivan Bessonov <bessonov...@gmail.com > >>> > >>>>> wrote: > >>>>>>>>> > >>>>>>>>> Hello Igniters, > >>>>>>>>> > >>>>>>>>> I object, banning streams is an overkill. I would argue that > >>> most > >>>> of > >>>>>>>>> the > >>>>>>>>> code > >>>>>>>>> is not on hot paths and that allocations in TLAB don't create > >>> much > >>>>>>>> pressure > >>>>>>>>> on GC. > >>>>>>>>> > >>>>>>>>> Streams must be used cautiously, developers should know > >> whether > >>>> they > >>>>>>>>> write hot methods or not. And if methods are not hot, code > >>>>> simplicity > >>>>>>>> must > >>>>>>>>> be > >>>>>>>>> the first priority. I don't want Ignite 3 code to look like > >>>> Ignite 2 > >>>>>>>> code, > >>>>>>>>> where > >>>>>>>>> people would iterate over Lists using explicit access by > >>> indexes, > >>>>>>>> because it > >>>>>>>>> saves them a single Iterator allocation. That's absurd. > >>>>>>>>> > >>>>>>>>> ср, 8 сент. 2021 г. в 11:43, Pavel Tupitsyn < > >>> ptupit...@apache.org > >>>>> : > >>>>>>>>> > >>>>>>>>>> Igniters, > >>>>>>>>>> > >>>>>>>>>> Java streams are known to be slower and cause more GC > >> pressure > >>>> than > >>>>>>>>>> an > >>>>>>>>>> equivalent loop. > >>>>>>>>>> Below is a simple filter/map/reduce scenario (code [1]): > >>>>>>>>>> > >>>>>>>>>> * Benchmark > >>>>> Mode > >>>>>>>> Cnt > >>>>>>>>>> Score Error Units > >>>>>>>>>> > >>>>>>>>>> * StreamVsLoopBenchmark.loopSum > >>>>>>>>>> thrpt > >>>>>>>> 3 > >>>>>>>>>> 7987.016 ± 293.013 ops/ms > >>>>>>>>>> * StreamVsLoopBenchmark.loopSum:·gc.alloc.rate > >>>>>>>>>> thrpt > >>>>>>>> 3 > >>>>>>>>>> ≈ 10⁻⁴ MB/sec > >>>>>>>>>> * StreamVsLoopBenchmark.loopSum:·gc.count > >>>>>>>>>> thrpt > >>>>>>>> 3 > >>>>>>>>>> ≈ 0 counts > >>>>>>>>>> > >>>>>>>>>> * StreamVsLoopBenchmark.streamSum > >>>>>>>>>> thrpt > >>>>>>>> 3 > >>>>>>>>>> 1060.244 ± 36.485 ops/ms > >>>>>>>>>> * StreamVsLoopBenchmark.streamSum:·gc.alloc.rate > >>>>>>>>>> thrpt > >>>>>>>> 3 > >>>>>>>>>> 315.819 ± 10.844 MB/sec > >>>>>>>>>> * StreamVsLoopBenchmark.streamSum:·gc.count > >>>>>>>>>> thrpt > >>>>>>>> 3 > >>>>>>>>>> 55.000 counts > >>>>>>>>>> > >>>>>>>>>> Loop is several times faster and does not allocate at all. > >>>>>>>>>> > >>>>>>>>>> 1. Performance is one of the most important features of our > >>>>> product. > >>>>>>>>>> 2. Most of our APIs will be on the hot path. > >>>>>>>>>> > >>>>>>>>>> One can argue about performance differences in real-world > >>>>> scenarios, > >>>>>>>>>> but increasing GC pressure just to make the code a little bit > >>>> nicer > >>>>>>>>>> is > >>>>>>>>>> unacceptable. > >>>>>>>>>> > >>>>>>>>>> I propose to ban streams usage in the codebase (except for > >> the > >>>>>>>>>> tests). > >>>>>>>>>> > >>>>>>>>>> Thoughts, objections? > >>>>>>>>>> > >>>>>>>>>> [1] > >>>>>>>>>> > >>>> https://gist.github.com/ptupitsyn/5934bbbf8f92ac4937e534af9386da97 > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> -- > >>>>>>>>> Sincerely yours, > >>>>>>>>> Ivan Bessonov > >>>>>>>> > >>>>>>>> > >>>>>> > >>>>> > >>>>> > >>>>> -- > >>>>> > >>>>> Best regards, > >>>>> Ivan Pavlukhin > >>>>> > >>>> > >>>> > >>>> -- > >>>> With regards, > >>>> Aleksandr Polovtcev > >>>> > >>> > >> > > -- Sincerely yours, Ivan Daschinskiy
