Jim Ok - understood. The trade-off that appears to be getting made to my eyes looks like less expressive code. If it were substantially valuable in terms of cpu, heap, io then I think it makes the case more compelling. I've not validated this with true testing but my intuition for this example is that it would be barely measurable. If you have different benchmark results to share that would be interesting to learn more about.
Thanks On Mon, Jun 17, 2024 at 8:23 AM Jim Steinebrey <jrsteineb...@gmail.com> wrote: > Joe, > Thanks for your detailed response and the separate email about moving NiFi > forward. > My intent in this PR was to decrease memory allocation of a FF and thereby > decrease memory that needed to be gc’ed. > Given your feedback in this email thread and some other PR feedback I got > this morning, > I have concluded this PR (https://github.com/apache/nifi/pull/8968) > to avoid a single FF allocation is not worth the complication of the API > and applying in so many places, > so I am going to close the PR in a little while if no one minds. > > Thanks, > Jim > > > > On Jun 15, 2024, at 12:59 PM, Joe Witt <joe.w...@gmail.com> wrote: > > > > Jim, > > > > Missing from this proposal is an explanation of the core 'why' or benefit > > this provides. I could see two potential ones based on the comment and > > this seems related to recent patterns of code changes in this similar > > vein. > > > > 1. We avoid a temporary java object representing the flow file. > > 2. It could reduce lines of code. > > > > If these are the only potential benefits I don't find them compelling. > > The cost in a memory/garbage collection sense I would expect is nearly > > immeasurably minor. While it would be great if it meant some > considerable > > improvement in code required or readability I dont see that in this case. > > From the example processor change shown for GenerateTableFetch the code > to > > me looks less readable and less expressive. > > > > So then there are other things we'd consider which we cannot see just > from > > the PR. > > > > 1. Does this make the system run measurably faster such that it reduces > CPU > > usage, lowers GC? > > 2. Does this drive down memory usage measurably/etc..? > > 3. Does this make our codebase in general more maintainable/etc..? > > > > If you could share more on what led you to feel this is a helpful > addition > > that information would be great for the community to know in considering > > this change. > > > > As it is right now I am not supportive. However, I want to be clear > about > > a couple things as while the feedback above is negative relative to the > > proposal... > > > > It is excellent that you're looking into things and considering > > opportunities to improve. > > > > It is excellent that you didn't take it so far but instead had an idea, > > wrote it down in such a way others can review, and offer feedback. > > > > This PR and the related ones prior to it make me think there is value in > a > > thread that outlines things people can do who are truly motivated to > really > > help drive things forward. I'll share some of those thoughts in another > > thread. > > > > The above is of course just my view. Will keep an eye for others' > inputs. > > > > Thanks > > > > > > > > On Sat, Jun 15, 2024 at 9:24 AM Jim Steinebrey <jrsteineb...@gmail.com> > > wrote: > > > >> Hi, > >> > >> https://github.com/apache/nifi/pull/8968 > >> > >> This PR is a proposal to see if the PMC agrees with this API change. > >> I have not written any new unit tests for this PR yet until I get buy-in > >> on it. > >> > >> I added two new create methods to the API of ProcessSession to allow the > >> option of > >> passing in ff attributes to be added to the flow file being created. > This > >> is not > >> a breaking change because the original create methods are still there. > >> > >> ProcessSession > >> FlowFile create(FlowFile parent, final Map<String, String> attributes); > >> FlowFile create(final Map<String, String> attributes); > >> > >> Changing a core API is a very significant change but I hope people will > >> see it > >> as worthwhile to allow us to to avoid creating an extra FlowFile object > in > >> many > >> places that FlowFiles are created. Not all places set attributes right > >> after \ > >> FF creation, but very many of them do and could benefit. > >> > >> There are 150+ places where these new methods can be used and I only > >> changed the > >> GenerateTableFetch processor to call them so you how the new methods are > >> used. > >> I expect using these new create methods has the potential to > >> avoid noticeable transient memory allocation like the earlier API > addition > >> of > >> SessionContext.isAutoTerminated did. > >> > >> Also if this PR is approved, myself and others can change processors to > >> call these > >> new methods in a future PRs (not as part of this PR). > >> > >> Regards, > >> Jim Steinebrey > >
