Re: [DISCUSS] Stream Pipelines on hot paths

I brought this topic to commons-collections because we use some streaming in the Bloom filter implementation where we are very sensitive to processing time. I received this answer over there and thought I would bring the information here: You need to test it with some realistic data for a

Re: [DISCUSS] Stream Pipelines on hot paths

Agreed Aleksey. I wouldn’t be opposed to more nuanced use but the burden that adds seems impractical. A simple rule is easier. Jordan On Fri, Jun 7, 2024 at 05:59 Aleksey Yeshchenko wrote: > It am okay with its use off hot paths in principle, and I’ve done it > myself. > > But as others have

Re: [DISCUSS] Stream Pipelines on hot paths

It am okay with its use off hot paths in principle, and I’ve done it myself. But as others have mentioned, it’s not obvious to every contributor what is and isn’t a hot path. Also, the codebase is a living, shifting thing: a cold path today can suddenly become hot tomorrow - it’s not uncommon.

Re: [DISCUSS] Stream Pipelines on hot paths

I think the value of this conversation is surfacing the problem with streams and raising awareness. If you use them in some test that sounds good to me. Same happens with some loops that trigger iterators that might inadvertently hydrate lots of unnecessary stuff into memory etc. Keeping

Re: [DISCUSS] Stream Pipelines on hot paths

I think it makes sense to use streams to make the life easier for a dev when constructing some log messages or something like that in clearly not hot paths. Nothing wrong with that ... Collectors.joining(", ") and that kind of stuff. I do not think that doing this aggressively and "orthodoxly" is

Re: [DISCUSS] Stream Pipelines on hot paths

I have to admit I didn’t expect when I raised this to be in a minority ok with *some* stream use :)Works for me though, definitely preferable to the status quo.On 7 Jun 2024, at 10:10, Aleksey Yeshchenko wrote:Ban in all new non-test code seems like the most pragmatic approach to me as well.On 7

Re: [DISCUSS] Stream Pipelines on hot paths

Ban in all new non-test code seems like the most pragmatic approach to me as well. > On 7 Jun 2024, at 06:32, Jordan West wrote: > > Similarly in the "don't use them in the main project but am ok with tests" > camp > > On Thu, Jun 6, 2024 at 4:46 AM Štefan Miklošovič

Re: [DISCUSS] Stream Pipelines on hot paths

Similarly in the "don't use them in the main project but am ok with tests" camp On Thu, Jun 6, 2024 at 4:46 AM Štefan Miklošovič < stefan.mikloso...@gmail.com> wrote: > I have created > > https://issues.apache.org/jira/browse/CASSANDRA-19673 > > to gather all your ideas about what to remove. If

Re: [DISCUSS] Stream Pipelines on hot paths

I have created https://issues.apache.org/jira/browse/CASSANDRA-19673 to gather all your ideas about what to remove. If you stumble upon some code which is susceptible to rewriting, just put it there. On Wed, Jun 5, 2024 at 6:35 PM wrote: > I would like to vote for banning streams in all

Re: [DISCUSS] Stream Pipelines on hot paths

I would like to vote for banning streams in all non-test code. It may not be easy for new contributors to distinguish between hot path and non-hot path. So would be great if we can simply block them in non-test code and update codestyle to detect the usage. > On Jun 4, 2024, at 6:26 PM, Josh

Re: [DISCUSS] Stream Pipelines on hot paths

+1 to ban them, considering we also do not have any regular performance testing as part of the project test suites. Jacek has a good point about the checkstyle - there is separate one for tests. Though I would not object if people want them banned from tests too. I guess if we ban them this would

Re: [DISCUSS] Stream Pipelines on hot paths

+1 (from the peanuts gallery) Removing streams from anything that looks like an hot path is indeed a good thing. Please balance with 'don't fix things that aren't broken'. While doing such changes seems a great idea, sometimes it may have side effects that you don't see until you run on real

Re: [DISCUSS] Stream Pipelines on hot paths

+1 agree with all this.  Also fine to just use in tests or ban completely.On Jun 2, 2024, at 11:58 AM, Jake Luciani wrote:+1 Java streams cause perf issues in hot paths. Its fine for tests and slow paths. But for clairity its fine to ban it as well if the majority agrees. On Sun, Jun 2, 2024 at

Re: [DISCUSS] Stream Pipelines on hot paths

Usages of them in tests are ok I think. We have a separate checkstyle file for the test code. - - -- --- - - Jacek Lewandowski pt., 31 maj 2024 o 19:14 David Capwell napisał(a): > I am cool for forbidding with a callout that tests are ok. I am cool with > forbidding

Re: [DISCUSS] Stream Pipelines on hot paths

> > I think this is a good general principle for *raising standards* in the > codebase like this: if somebody says something is important, and cannot be > convinced otherwise, then it should generally be treated as important. This > is different from cases where there are simply competing

Re: [DISCUSS] Stream Pipelines on hot paths

I am cool for forbidding with a callout that tests are ok. I am cool with forbidding in tests as well, but thats just for consistency reasons than anything. > On May 31, 2024, at 8:12 AM, Brandon Williams wrote: > > > On Fri, May 31, 2024 at 9:35 AM Abe Ratnofsky >

Re: [DISCUSS] Stream Pipelines on hot paths

On Fri, May 31, 2024 at 9:35 AM Abe Ratnofsky wrote: > +1 to forbidding Stream usage entirely; the convenience of using them > outside of hot paths is less than the burden of figuring out whether or not > a particular path is hot. > I think I have most frequently appreciated them in tests,

Re: [DISCUSS] Stream Pipelines on hot paths

+1 to forbidding Stream usage entirely; the convenience of using them outside of hot paths is less than the burden of figuring out whether or not a particular path is hot. Even for reviewers it can be difficult to tell whether a particular path is hot; hard-to-diagnose bugs like CASSANDRA-18110

Re: [DISCUSS] Stream Pipelines on hot paths

I think I have already proposed a simple solution to this problem on the thread: if anyone says it’s a hot path (and cannot be persuaded otherwise), it should be treated as such. Saves argument, but permits an easy escape hatch if everyone agrees with minimal discussion. I think this is a good

Re: [DISCUSS] Stream Pipelines on hot paths

+1 to either forbid them entirely or not at all. pt., 31 maj 2024, 16:05 użytkownik Benjamin Lerer napisał: > For me the definition of hot path is too vague. We had arguments with > Berenger multiple times and it is more a waste of time than anything else > at the end. If we are truly

Re: [DISCUSS] Stream Pipelines on hot paths

For me the definition of hot path is too vague. We had arguments with Berenger multiple times and it is more a waste of time than anything else at the end. If we are truly concerned about stream efficiency then we should simply forbid them. That will avoid lengthy discussions about what constitute

Re: [DISCUSS] Stream Pipelines on hot paths

My concept of hot path is simply anything we can expect to be called frequently enough in normal operation that it might show up in a profiler. If it’s a library method then it’s reasonable to assume it should be able to be used in a hot path unless clearly labelled otherwise. In fact, I’d say the

Re: [DISCUSS] Stream Pipelines on hot paths

+1 on avoiding streams in hot paths On 31/5/24 9:48, Benedict wrote: My concept of hot path is simply anything we can expect to be called frequently enough in normal operation that it might show up in a profiler. If it’s a library method then it’s reasonable to assume it should be able to be

Re: [DISCUSS] Stream Pipelines on hot paths

My concept of hot path is simply anything we can expect to be called frequently enough in normal operation that it might show up in a profiler. If it’s a library method then it’s reasonable to assume it should be able to be used in a hot path unless clearly labelled otherwise. In my view this

Re: [DISCUSS] Stream Pipelines on hot paths

+1 and I am happy to go over streams and at least map where we are at to see what could be done about that. As David mentioned, the main problem is to distinguish what counts as a hot path and what not. On Thu, May 30, 2024 at 6:29 PM Benedict wrote: > Since it’s related to the logging

Re: [DISCUSS] Stream Pipelines on hot paths

+1. To not using streams in hot paths. Regarding string concatenation in logging, for debug and trace it makes sense to avoid concatenation. For info and error I don't think it matters and it can be more concise to concatenate. It's not a big deal to standardize on one just because the extra

Re: [DISCUSS] Stream Pipelines on hot paths

Definitely +1 on this. We saw in the early days of SAI development that stream pipelines had a substantial impact on performance. On Thu, 30 May 2024 at 19:28, Caleb Rackliffe wrote: > +1 > > On Thu, May 30, 2024 at 11:29 AM Benedict wrote: > >> Since it’s related to the logging discussion

Re: [DISCUSS] Stream Pipelines on hot paths

+1 On Thu, May 30, 2024 at 11:29 AM Benedict wrote: > Since it’s related to the logging discussion we’re already having, I have > seen stream pipelines showing up in a lot of traces recently. I am > surprised; I thought it was understood that they shouldn’t be used on hot > paths as they are

Re: [DISCUSS] Stream Pipelines on hot paths

As a general statement I agree with you (same for String.format as well), but one thing to call out is that it can be hard to tell what is the hot path and what isn’t. When you are doing background work (like repair) its clear, but when touching something internal it can be hard to tell; this

[DISCUSS] Stream Pipelines on hot paths

Since it’s related to the logging discussion we’re already having, I have seen stream pipelines showing up in a lot of traces recently. I am surprised; I thought it was understood that they shouldn’t be used on hot paths as they are not typically as efficient as old skool for-each constructions