Re: [hibernate-dev] v6 and load-event
I *think* so. On Fri, Jun 5, 2020 at 12:45 PM Steve Ebersole wrote: > Load event handling does not have "anything" parameters. Am I > understanding you correctly? > > On Fri, Jun 5, 2020, 11:42 AM Gail Badner wrote: > >> Hi Steve, >> >> Sorry, I have not read the entire thread carefully, so please disregard >> if not relevant. >> >> Would this provide functionality like what we discussed for an >> OperationContext? >> >> https://hibernate.atlassian.net/browse/HHH-10478 >> >> Thanks, >> Gail >> >> On Fri, May 29, 2020 at 4:21 AM Steve Ebersole >> wrote: >> >>> If/when it comes to needing that, I kind of like that continuation >>> design. >>> But I agree, Yoann is right - let's leave it off until needed or until we >>> have specific requirements. >>> >>> Thanks everyone! >>> >>> On Fri, May 29, 2020 at 2:19 AM Sanne Grinovero >>> wrote: >>> >>> > On Fri, 29 May 2020 at 07:22, Yoann Rodiere >>> wrote: >>> > > >>> > > +1 not to add surround capability initially. Sounds better to start >>> > simple and make things more complex when we actually need it :) >>> > >>> > Right. I didn't mean to raise additional requirements without having >>> > investigated those tracing libraries - what I meant really is just to >>> > raise awareness that we'll likely need to evolve it further when it >>> > comes to finally implement such things. >>> > >>> > > >>> > > Yoann Rodière >>> > > Hibernate Team >>> > > yo...@hibernate.org >>> > > >>> > > >>> > > On Fri, 29 May 2020 at 07:25, Sanne Grinovero >>> > wrote: >>> > >> >>> > >> Yes, I agree. >>> > >> >>> > >> On Thu, 28 May 2020, 22:11 Steve Ebersole, >>> wrote: >>> > >>> >>> > >>> Wanted to clarify - >>> > >>> >>> > >>> Regarding incremental addition of "surround listeners", so long as >>> we >>> > are all in agreement that this simply means there will be absolutely no >>> > surround capability ***initially*** then I am fine with that. >>> > >>> >>> > >>> On Thu, May 28, 2020 at 4:10 PM Steve Ebersole < >>> st...@hibernate.org> >>> > wrote: >>> > >>> > Hm, the dynamic enable/disable stuff should be easy to handle, no? >>> > Depends on what specific library you are thinking of and exactly how >>> that >>> > detail gets propagated to us. At the end of the day, its really as >>> simple >>> > as protecting the creation of some of these objects with `if >>> > (enabled)`-type checks. >>> > >>> > But again, if you have specific details in mind we can take a >>> look. >>> > >>> > Also, I think it is not at all a good idea to even plan for >>> > "different types of events". In fact I'm fine with getting rid of >>> > LoadEvent completely from that contract and simply directly passing the >>> > information that is likely useful. I mean at the end of the day a >>> listener >>> > for load events is going to be interested in the same set of >>> information. >>> > Yes, some will not need all of that information but that's not really a >>> > concern IMO. Especially if we inline the parameters and completely >>> avoid >>> > the event object instantiation >>> > >>> > Regarding incremental addition of "surround listeners", so long >>> as we >>> > are all in agreement that this simply means there will be absolutely no >>> > surround capability then I am fine with that. >>> > >>> > >>> > On Thu, May 28, 2020 at 3:55 PM Sanne Grinovero < >>> sa...@hibernate.org> >>> > wrote: >>> > > >>> > > On Thu, 28 May 2020 at 21:27, Steve Ebersole < >>> st...@hibernate.org> >>> > wrote: >>> > > > >>> > > > Any thoughts on this "continuation" approach? >>> > > >>> > > I love the pattern! Maybe we'll need also some ability to not >>> capture >>> > > the state for events which don't have any? >>> > > >>> > > I wonder if that implies we'll need two different event >>> contracts: >>> > one >>> > > for the listeners which need state and one for those which >>> don't; but >>> > > I'm not eager to overcomplicate this. >>> > > >>> > > > Or maybe its just not important (yet) to handle "surround" >>> > handling? >>> > > >>> > > I'm confident that integration with tracing libraries would be >>> very >>> > > useful and interesting to have - but indeed not having time to >>> > > research it properly I'm a bit afraid that it might need further >>> > > changes to reach excellent performance. >>> > > >>> > > For example one thing I remember is that with some libraries >>> you're >>> > > supposed to have the option to enable/disable the profiling >>> options >>> > > dynamically, and since there's an expectation of no overhead when >>> > it's >>> > > disabled this would need to imply having a way for the overhead >>> of >>> > > allocating space for the captured state to "vanish": this might >>> be a >>> > > bit more complicated, or need to be able to take advantage of JIT >>> > > optimisations. >>> > > >>> > > So if we end up thinking that such event AP
Re: [hibernate-dev] v6 and load-event
Load event handling does not have "anything" parameters. Am I understanding you correctly? On Fri, Jun 5, 2020, 11:42 AM Gail Badner wrote: > Hi Steve, > > Sorry, I have not read the entire thread carefully, so please disregard if > not relevant. > > Would this provide functionality like what we discussed for an > OperationContext? > > https://hibernate.atlassian.net/browse/HHH-10478 > > Thanks, > Gail > > On Fri, May 29, 2020 at 4:21 AM Steve Ebersole > wrote: > >> If/when it comes to needing that, I kind of like that continuation design. >> But I agree, Yoann is right - let's leave it off until needed or until we >> have specific requirements. >> >> Thanks everyone! >> >> On Fri, May 29, 2020 at 2:19 AM Sanne Grinovero >> wrote: >> >> > On Fri, 29 May 2020 at 07:22, Yoann Rodiere >> wrote: >> > > >> > > +1 not to add surround capability initially. Sounds better to start >> > simple and make things more complex when we actually need it :) >> > >> > Right. I didn't mean to raise additional requirements without having >> > investigated those tracing libraries - what I meant really is just to >> > raise awareness that we'll likely need to evolve it further when it >> > comes to finally implement such things. >> > >> > > >> > > Yoann Rodière >> > > Hibernate Team >> > > yo...@hibernate.org >> > > >> > > >> > > On Fri, 29 May 2020 at 07:25, Sanne Grinovero >> > wrote: >> > >> >> > >> Yes, I agree. >> > >> >> > >> On Thu, 28 May 2020, 22:11 Steve Ebersole, >> wrote: >> > >>> >> > >>> Wanted to clarify - >> > >>> >> > >>> Regarding incremental addition of "surround listeners", so long as >> we >> > are all in agreement that this simply means there will be absolutely no >> > surround capability ***initially*** then I am fine with that. >> > >>> >> > >>> On Thu, May 28, 2020 at 4:10 PM Steve Ebersole > > >> > wrote: >> > >> > Hm, the dynamic enable/disable stuff should be easy to handle, no? >> > Depends on what specific library you are thinking of and exactly how >> that >> > detail gets propagated to us. At the end of the day, its really as >> simple >> > as protecting the creation of some of these objects with `if >> > (enabled)`-type checks. >> > >> > But again, if you have specific details in mind we can take a look. >> > >> > Also, I think it is not at all a good idea to even plan for >> > "different types of events". In fact I'm fine with getting rid of >> > LoadEvent completely from that contract and simply directly passing the >> > information that is likely useful. I mean at the end of the day a >> listener >> > for load events is going to be interested in the same set of >> information. >> > Yes, some will not need all of that information but that's not really a >> > concern IMO. Especially if we inline the parameters and completely >> avoid >> > the event object instantiation >> > >> > Regarding incremental addition of "surround listeners", so long as >> we >> > are all in agreement that this simply means there will be absolutely no >> > surround capability then I am fine with that. >> > >> > >> > On Thu, May 28, 2020 at 3:55 PM Sanne Grinovero < >> sa...@hibernate.org> >> > wrote: >> > > >> > > On Thu, 28 May 2020 at 21:27, Steve Ebersole > > >> > wrote: >> > > > >> > > > Any thoughts on this "continuation" approach? >> > > >> > > I love the pattern! Maybe we'll need also some ability to not >> capture >> > > the state for events which don't have any? >> > > >> > > I wonder if that implies we'll need two different event contracts: >> > one >> > > for the listeners which need state and one for those which don't; >> but >> > > I'm not eager to overcomplicate this. >> > > >> > > > Or maybe its just not important (yet) to handle "surround" >> > handling? >> > > >> > > I'm confident that integration with tracing libraries would be >> very >> > > useful and interesting to have - but indeed not having time to >> > > research it properly I'm a bit afraid that it might need further >> > > changes to reach excellent performance. >> > > >> > > For example one thing I remember is that with some libraries >> you're >> > > supposed to have the option to enable/disable the profiling >> options >> > > dynamically, and since there's an expectation of no overhead when >> > it's >> > > disabled this would need to imply having a way for the overhead of >> > > allocating space for the captured state to "vanish": this might >> be a >> > > bit more complicated, or need to be able to take advantage of JIT >> > > optimisations. >> > > >> > > So if we end up thinking that such event APIs need to be different >> > > depending on the need for state, perhaps indeed it's better to >> > > postpone the design of those with state to when someone has time >> to >> > > research an optimal integration with a tracing library. It might >> not >> > >>
Re: [hibernate-dev] v6 and load-event
Hi Steve, Sorry, I have not read the entire thread carefully, so please disregard if not relevant. Would this provide functionality like what we discussed for an OperationContext? https://hibernate.atlassian.net/browse/HHH-10478 Thanks, Gail On Fri, May 29, 2020 at 4:21 AM Steve Ebersole wrote: > If/when it comes to needing that, I kind of like that continuation design. > But I agree, Yoann is right - let's leave it off until needed or until we > have specific requirements. > > Thanks everyone! > > On Fri, May 29, 2020 at 2:19 AM Sanne Grinovero > wrote: > > > On Fri, 29 May 2020 at 07:22, Yoann Rodiere wrote: > > > > > > +1 not to add surround capability initially. Sounds better to start > > simple and make things more complex when we actually need it :) > > > > Right. I didn't mean to raise additional requirements without having > > investigated those tracing libraries - what I meant really is just to > > raise awareness that we'll likely need to evolve it further when it > > comes to finally implement such things. > > > > > > > > Yoann Rodière > > > Hibernate Team > > > yo...@hibernate.org > > > > > > > > > On Fri, 29 May 2020 at 07:25, Sanne Grinovero > > wrote: > > >> > > >> Yes, I agree. > > >> > > >> On Thu, 28 May 2020, 22:11 Steve Ebersole, > wrote: > > >>> > > >>> Wanted to clarify - > > >>> > > >>> Regarding incremental addition of "surround listeners", so long as we > > are all in agreement that this simply means there will be absolutely no > > surround capability ***initially*** then I am fine with that. > > >>> > > >>> On Thu, May 28, 2020 at 4:10 PM Steve Ebersole > > wrote: > > > > Hm, the dynamic enable/disable stuff should be easy to handle, no? > > Depends on what specific library you are thinking of and exactly how that > > detail gets propagated to us. At the end of the day, its really as > simple > > as protecting the creation of some of these objects with `if > > (enabled)`-type checks. > > > > But again, if you have specific details in mind we can take a look. > > > > Also, I think it is not at all a good idea to even plan for > > "different types of events". In fact I'm fine with getting rid of > > LoadEvent completely from that contract and simply directly passing the > > information that is likely useful. I mean at the end of the day a > listener > > for load events is going to be interested in the same set of information. > > Yes, some will not need all of that information but that's not really a > > concern IMO. Especially if we inline the parameters and completely avoid > > the event object instantiation > > > > Regarding incremental addition of "surround listeners", so long as > we > > are all in agreement that this simply means there will be absolutely no > > surround capability then I am fine with that. > > > > > > On Thu, May 28, 2020 at 3:55 PM Sanne Grinovero < > sa...@hibernate.org> > > wrote: > > > > > > On Thu, 28 May 2020 at 21:27, Steve Ebersole > > wrote: > > > > > > > > Any thoughts on this "continuation" approach? > > > > > > I love the pattern! Maybe we'll need also some ability to not > capture > > > the state for events which don't have any? > > > > > > I wonder if that implies we'll need two different event contracts: > > one > > > for the listeners which need state and one for those which don't; > but > > > I'm not eager to overcomplicate this. > > > > > > > Or maybe its just not important (yet) to handle "surround" > > handling? > > > > > > I'm confident that integration with tracing libraries would be very > > > useful and interesting to have - but indeed not having time to > > > research it properly I'm a bit afraid that it might need further > > > changes to reach excellent performance. > > > > > > For example one thing I remember is that with some libraries you're > > > supposed to have the option to enable/disable the profiling options > > > dynamically, and since there's an expectation of no overhead when > > it's > > > disabled this would need to imply having a way for the overhead of > > > allocating space for the captured state to "vanish": this might be > a > > > bit more complicated, or need to be able to take advantage of JIT > > > optimisations. > > > > > > So if we end up thinking that such event APIs need to be different > > > depending on the need for state, perhaps indeed it's better to > > > postpone the design of those with state to when someone has time to > > > research an optimal integration with a tracing library. It might > not > > > be too hard, I just haven't explored it myself yet. > > > > > > Maybe let's do this incrementally, considering the "continuation" > > > approach a next step? > > > > > > Thanks, > > > Sanne > > > > > > > > > > > On Wed, May 27, 2020 at 9:27 AM Steve Ebersole < > > st...@hi
Re: [hibernate-dev] v6 and load-event
If/when it comes to needing that, I kind of like that continuation design. But I agree, Yoann is right - let's leave it off until needed or until we have specific requirements. Thanks everyone! On Fri, May 29, 2020 at 2:19 AM Sanne Grinovero wrote: > On Fri, 29 May 2020 at 07:22, Yoann Rodiere wrote: > > > > +1 not to add surround capability initially. Sounds better to start > simple and make things more complex when we actually need it :) > > Right. I didn't mean to raise additional requirements without having > investigated those tracing libraries - what I meant really is just to > raise awareness that we'll likely need to evolve it further when it > comes to finally implement such things. > > > > > Yoann Rodière > > Hibernate Team > > yo...@hibernate.org > > > > > > On Fri, 29 May 2020 at 07:25, Sanne Grinovero > wrote: > >> > >> Yes, I agree. > >> > >> On Thu, 28 May 2020, 22:11 Steve Ebersole, wrote: > >>> > >>> Wanted to clarify - > >>> > >>> Regarding incremental addition of "surround listeners", so long as we > are all in agreement that this simply means there will be absolutely no > surround capability ***initially*** then I am fine with that. > >>> > >>> On Thu, May 28, 2020 at 4:10 PM Steve Ebersole > wrote: > > Hm, the dynamic enable/disable stuff should be easy to handle, no? > Depends on what specific library you are thinking of and exactly how that > detail gets propagated to us. At the end of the day, its really as simple > as protecting the creation of some of these objects with `if > (enabled)`-type checks. > > But again, if you have specific details in mind we can take a look. > > Also, I think it is not at all a good idea to even plan for > "different types of events". In fact I'm fine with getting rid of > LoadEvent completely from that contract and simply directly passing the > information that is likely useful. I mean at the end of the day a listener > for load events is going to be interested in the same set of information. > Yes, some will not need all of that information but that's not really a > concern IMO. Especially if we inline the parameters and completely avoid > the event object instantiation > > Regarding incremental addition of "surround listeners", so long as we > are all in agreement that this simply means there will be absolutely no > surround capability then I am fine with that. > > > On Thu, May 28, 2020 at 3:55 PM Sanne Grinovero > wrote: > > > > On Thu, 28 May 2020 at 21:27, Steve Ebersole > wrote: > > > > > > Any thoughts on this "continuation" approach? > > > > I love the pattern! Maybe we'll need also some ability to not capture > > the state for events which don't have any? > > > > I wonder if that implies we'll need two different event contracts: > one > > for the listeners which need state and one for those which don't; but > > I'm not eager to overcomplicate this. > > > > > Or maybe its just not important (yet) to handle "surround" > handling? > > > > I'm confident that integration with tracing libraries would be very > > useful and interesting to have - but indeed not having time to > > research it properly I'm a bit afraid that it might need further > > changes to reach excellent performance. > > > > For example one thing I remember is that with some libraries you're > > supposed to have the option to enable/disable the profiling options > > dynamically, and since there's an expectation of no overhead when > it's > > disabled this would need to imply having a way for the overhead of > > allocating space for the captured state to "vanish": this might be a > > bit more complicated, or need to be able to take advantage of JIT > > optimisations. > > > > So if we end up thinking that such event APIs need to be different > > depending on the need for state, perhaps indeed it's better to > > postpone the design of those with state to when someone has time to > > research an optimal integration with a tracing library. It might not > > be too hard, I just haven't explored it myself yet. > > > > Maybe let's do this incrementally, considering the "continuation" > > approach a next step? > > > > Thanks, > > Sanne > > > > > > > > On Wed, May 27, 2020 at 9:27 AM Steve Ebersole < > st...@hibernate.org> wrote: > > >> > > >> Inline... > > >> > > >> On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero < > sa...@hibernate.org> wrote: > > >>> > > >>> At high level I agree, just have 3 more thoughts: > > >>> > > >>> # Regarding the "swap" of information between listeners - could > that > > >>> even work? I might have misunderstood something, but wouldn't we > > >>> require listeners to run in some specific order for such > swapping to > > >>> work? > > >> > > >> > > >> This is why we allow control
Re: [hibernate-dev] v6 and load-event
On Fri, 29 May 2020 at 07:22, Yoann Rodiere wrote: > > +1 not to add surround capability initially. Sounds better to start simple > and make things more complex when we actually need it :) Right. I didn't mean to raise additional requirements without having investigated those tracing libraries - what I meant really is just to raise awareness that we'll likely need to evolve it further when it comes to finally implement such things. > > Yoann Rodière > Hibernate Team > yo...@hibernate.org > > > On Fri, 29 May 2020 at 07:25, Sanne Grinovero wrote: >> >> Yes, I agree. >> >> On Thu, 28 May 2020, 22:11 Steve Ebersole, wrote: >>> >>> Wanted to clarify - >>> >>> Regarding incremental addition of "surround listeners", so long as we are >>> all in agreement that this simply means there will be absolutely no >>> surround capability ***initially*** then I am fine with that. >>> >>> On Thu, May 28, 2020 at 4:10 PM Steve Ebersole wrote: Hm, the dynamic enable/disable stuff should be easy to handle, no? Depends on what specific library you are thinking of and exactly how that detail gets propagated to us. At the end of the day, its really as simple as protecting the creation of some of these objects with `if (enabled)`-type checks. But again, if you have specific details in mind we can take a look. Also, I think it is not at all a good idea to even plan for "different types of events". In fact I'm fine with getting rid of LoadEvent completely from that contract and simply directly passing the information that is likely useful. I mean at the end of the day a listener for load events is going to be interested in the same set of information. Yes, some will not need all of that information but that's not really a concern IMO. Especially if we inline the parameters and completely avoid the event object instantiation Regarding incremental addition of "surround listeners", so long as we are all in agreement that this simply means there will be absolutely no surround capability then I am fine with that. On Thu, May 28, 2020 at 3:55 PM Sanne Grinovero wrote: > > On Thu, 28 May 2020 at 21:27, Steve Ebersole wrote: > > > > Any thoughts on this "continuation" approach? > > I love the pattern! Maybe we'll need also some ability to not capture > the state for events which don't have any? > > I wonder if that implies we'll need two different event contracts: one > for the listeners which need state and one for those which don't; but > I'm not eager to overcomplicate this. > > > Or maybe its just not important (yet) to handle "surround" handling? > > I'm confident that integration with tracing libraries would be very > useful and interesting to have - but indeed not having time to > research it properly I'm a bit afraid that it might need further > changes to reach excellent performance. > > For example one thing I remember is that with some libraries you're > supposed to have the option to enable/disable the profiling options > dynamically, and since there's an expectation of no overhead when it's > disabled this would need to imply having a way for the overhead of > allocating space for the captured state to "vanish": this might be a > bit more complicated, or need to be able to take advantage of JIT > optimisations. > > So if we end up thinking that such event APIs need to be different > depending on the need for state, perhaps indeed it's better to > postpone the design of those with state to when someone has time to > research an optimal integration with a tracing library. It might not > be too hard, I just haven't explored it myself yet. > > Maybe let's do this incrementally, considering the "continuation" > approach a next step? > > Thanks, > Sanne > > > > > On Wed, May 27, 2020 at 9:27 AM Steve Ebersole > > wrote: > >> > >> Inline... > >> > >> On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero > >> wrote: > >>> > >>> At high level I agree, just have 3 more thoughts: > >>> > >>> # Regarding the "swap" of information between listeners - could that > >>> even work? I might have misunderstood something, but wouldn't we > >>> require listeners to run in some specific order for such swapping to > >>> work? > >> > >> > >> This is why we allow control over the ordering of the registered > >> listeners. And yes, that is and was a hokey solution. Nothing > >> changes there really if that is why you are using load listener. > >> > >> > >>> > >>> # The "surround advice" you mention for e.g. timing seems very > >>> interesting, especially as I'd love us to be able to integrate with > >>> tracing libraries - but these would need
Re: [hibernate-dev] v6 and load-event
+1 not to add surround capability initially. Sounds better to start simple and make things more complex when we actually need it :) Yoann Rodière Hibernate Team yo...@hibernate.org On Fri, 29 May 2020 at 07:25, Sanne Grinovero wrote: > Yes, I agree. > > On Thu, 28 May 2020, 22:11 Steve Ebersole, wrote: > >> Wanted to clarify - >> >> Regarding incremental addition of "surround listeners", so long as we are >> all in agreement that this simply means there will be absolutely no >> surround capability ***initially*** then I am fine with that. >> >> On Thu, May 28, 2020 at 4:10 PM Steve Ebersole >> wrote: >> >>> Hm, the dynamic enable/disable stuff should be easy to handle, no? >>> Depends on what specific library you are thinking of and exactly how that >>> detail gets propagated to us. At the end of the day, its really as simple >>> as protecting the creation of some of these objects with `if >>> (enabled)`-type checks. >>> >>> But again, if you have specific details in mind we can take a look. >>> >>> Also, I think it is not at all a good idea to even plan for "different >>> types of events". In fact I'm fine with getting rid of LoadEvent >>> completely from that contract and simply directly passing the information >>> that is likely useful. I mean at the end of the day a listener for load >>> events is going to be interested in the same set of information. Yes, some >>> will not need all of that information but that's not really a concern IMO. >>> Especially if we inline the parameters and completely avoid the event >>> object instantiation >>> >>> Regarding incremental addition of "surround listeners", so long as we >>> are all in agreement that this simply means there will be absolutely no >>> surround capability then I am fine with that. >>> >>> >>> On Thu, May 28, 2020 at 3:55 PM Sanne Grinovero >>> wrote: >>> On Thu, 28 May 2020 at 21:27, Steve Ebersole wrote: > > Any thoughts on this "continuation" approach? I love the pattern! Maybe we'll need also some ability to not capture the state for events which don't have any? I wonder if that implies we'll need two different event contracts: one for the listeners which need state and one for those which don't; but I'm not eager to overcomplicate this. > Or maybe its just not important (yet) to handle "surround" handling? I'm confident that integration with tracing libraries would be very useful and interesting to have - but indeed not having time to research it properly I'm a bit afraid that it might need further changes to reach excellent performance. For example one thing I remember is that with some libraries you're supposed to have the option to enable/disable the profiling options dynamically, and since there's an expectation of no overhead when it's disabled this would need to imply having a way for the overhead of allocating space for the captured state to "vanish": this might be a bit more complicated, or need to be able to take advantage of JIT optimisations. So if we end up thinking that such event APIs need to be different depending on the need for state, perhaps indeed it's better to postpone the design of those with state to when someone has time to research an optimal integration with a tracing library. It might not be too hard, I just haven't explored it myself yet. Maybe let's do this incrementally, considering the "continuation" approach a next step? Thanks, Sanne > > On Wed, May 27, 2020 at 9:27 AM Steve Ebersole wrote: >> >> Inline... >> >> On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero wrote: >>> >>> At high level I agree, just have 3 more thoughts: >>> >>> # Regarding the "swap" of information between listeners - could that >>> even work? I might have misunderstood something, but wouldn't we >>> require listeners to run in some specific order for such swapping to >>> work? >> >> >> This is why we allow control over the ordering of the registered listeners. And yes, that is and was a hokey solution. Nothing changes there really if that is why you are using load listener. >> >> >>> >>> # The "surround advice" you mention for e.g. timing seems very >>> interesting, especially as I'd love us to be able to integrate with >>> tracing libraries - but these would need to be able to co-relate the >>> pre-load event with some post-load event. How would that work? I'd >>> expect these to need having a single listener implementation which >>> implements both PreLoadEventListener and PostLoadEventListener, but >>> also they'll likely need some capability to store some information >>> contextual to the "event". >> >> >> I was just thinking through this one as well. My initi
Re: [hibernate-dev] v6 and load-event
Yes, I agree. On Thu, 28 May 2020, 22:11 Steve Ebersole, wrote: > Wanted to clarify - > > Regarding incremental addition of "surround listeners", so long as we are > all in agreement that this simply means there will be absolutely no > surround capability ***initially*** then I am fine with that. > > On Thu, May 28, 2020 at 4:10 PM Steve Ebersole > wrote: > >> Hm, the dynamic enable/disable stuff should be easy to handle, no? >> Depends on what specific library you are thinking of and exactly how that >> detail gets propagated to us. At the end of the day, its really as simple >> as protecting the creation of some of these objects with `if >> (enabled)`-type checks. >> >> But again, if you have specific details in mind we can take a look. >> >> Also, I think it is not at all a good idea to even plan for "different >> types of events". In fact I'm fine with getting rid of LoadEvent >> completely from that contract and simply directly passing the information >> that is likely useful. I mean at the end of the day a listener for load >> events is going to be interested in the same set of information. Yes, some >> will not need all of that information but that's not really a concern IMO. >> Especially if we inline the parameters and completely avoid the event >> object instantiation >> >> Regarding incremental addition of "surround listeners", so long as we are >> all in agreement that this simply means there will be absolutely no >> surround capability then I am fine with that. >> >> >> On Thu, May 28, 2020 at 3:55 PM Sanne Grinovero >> wrote: >> >>> On Thu, 28 May 2020 at 21:27, Steve Ebersole >>> wrote: >>> > >>> > Any thoughts on this "continuation" approach? >>> >>> I love the pattern! Maybe we'll need also some ability to not capture >>> the state for events which don't have any? >>> >>> I wonder if that implies we'll need two different event contracts: one >>> for the listeners which need state and one for those which don't; but >>> I'm not eager to overcomplicate this. >>> >>> > Or maybe its just not important (yet) to handle "surround" handling? >>> >>> I'm confident that integration with tracing libraries would be very >>> useful and interesting to have - but indeed not having time to >>> research it properly I'm a bit afraid that it might need further >>> changes to reach excellent performance. >>> >>> For example one thing I remember is that with some libraries you're >>> supposed to have the option to enable/disable the profiling options >>> dynamically, and since there's an expectation of no overhead when it's >>> disabled this would need to imply having a way for the overhead of >>> allocating space for the captured state to "vanish": this might be a >>> bit more complicated, or need to be able to take advantage of JIT >>> optimisations. >>> >>> So if we end up thinking that such event APIs need to be different >>> depending on the need for state, perhaps indeed it's better to >>> postpone the design of those with state to when someone has time to >>> research an optimal integration with a tracing library. It might not >>> be too hard, I just haven't explored it myself yet. >>> >>> Maybe let's do this incrementally, considering the "continuation" >>> approach a next step? >>> >>> Thanks, >>> Sanne >>> >>> > >>> > On Wed, May 27, 2020 at 9:27 AM Steve Ebersole >>> wrote: >>> >> >>> >> Inline... >>> >> >>> >> On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero >>> wrote: >>> >>> >>> >>> At high level I agree, just have 3 more thoughts: >>> >>> >>> >>> # Regarding the "swap" of information between listeners - could that >>> >>> even work? I might have misunderstood something, but wouldn't we >>> >>> require listeners to run in some specific order for such swapping to >>> >>> work? >>> >> >>> >> >>> >> This is why we allow control over the ordering of the registered >>> listeners. And yes, that is and was a hokey solution. Nothing changes >>> there really if that is why you are using load listener. >>> >> >>> >> >>> >>> >>> >>> # The "surround advice" you mention for e.g. timing seems very >>> >>> interesting, especially as I'd love us to be able to integrate with >>> >>> tracing libraries - but these would need to be able to co-relate the >>> >>> pre-load event with some post-load event. How would that work? I'd >>> >>> expect these to need having a single listener implementation which >>> >>> implements both PreLoadEventListener and PostLoadEventListener, but >>> >>> also they'll likely need some capability to store some information >>> >>> contextual to the "event". >>> >> >>> >> >>> >> I was just thinking through this one as well. My initial thought was >>> exactly what you proposed - some combination of pre/post listener with some >>> ability to store state between. But that gets ugly. >>> >> >>> >> Another option I thought about is easier to illustrate, but basically >>> works on the principle of "continuation" many surround advice solutions are >>> based on: >>> https://gist.githu
Re: [hibernate-dev] v6 and load-event
Wanted to clarify - Regarding incremental addition of "surround listeners", so long as we are all in agreement that this simply means there will be absolutely no surround capability ***initially*** then I am fine with that. On Thu, May 28, 2020 at 4:10 PM Steve Ebersole wrote: > Hm, the dynamic enable/disable stuff should be easy to handle, no? > Depends on what specific library you are thinking of and exactly how that > detail gets propagated to us. At the end of the day, its really as simple > as protecting the creation of some of these objects with `if > (enabled)`-type checks. > > But again, if you have specific details in mind we can take a look. > > Also, I think it is not at all a good idea to even plan for "different > types of events". In fact I'm fine with getting rid of LoadEvent > completely from that contract and simply directly passing the information > that is likely useful. I mean at the end of the day a listener for load > events is going to be interested in the same set of information. Yes, some > will not need all of that information but that's not really a concern IMO. > Especially if we inline the parameters and completely avoid the event > object instantiation > > Regarding incremental addition of "surround listeners", so long as we are > all in agreement that this simply means there will be absolutely no > surround capability then I am fine with that. > > > On Thu, May 28, 2020 at 3:55 PM Sanne Grinovero > wrote: > >> On Thu, 28 May 2020 at 21:27, Steve Ebersole wrote: >> > >> > Any thoughts on this "continuation" approach? >> >> I love the pattern! Maybe we'll need also some ability to not capture >> the state for events which don't have any? >> >> I wonder if that implies we'll need two different event contracts: one >> for the listeners which need state and one for those which don't; but >> I'm not eager to overcomplicate this. >> >> > Or maybe its just not important (yet) to handle "surround" handling? >> >> I'm confident that integration with tracing libraries would be very >> useful and interesting to have - but indeed not having time to >> research it properly I'm a bit afraid that it might need further >> changes to reach excellent performance. >> >> For example one thing I remember is that with some libraries you're >> supposed to have the option to enable/disable the profiling options >> dynamically, and since there's an expectation of no overhead when it's >> disabled this would need to imply having a way for the overhead of >> allocating space for the captured state to "vanish": this might be a >> bit more complicated, or need to be able to take advantage of JIT >> optimisations. >> >> So if we end up thinking that such event APIs need to be different >> depending on the need for state, perhaps indeed it's better to >> postpone the design of those with state to when someone has time to >> research an optimal integration with a tracing library. It might not >> be too hard, I just haven't explored it myself yet. >> >> Maybe let's do this incrementally, considering the "continuation" >> approach a next step? >> >> Thanks, >> Sanne >> >> > >> > On Wed, May 27, 2020 at 9:27 AM Steve Ebersole >> wrote: >> >> >> >> Inline... >> >> >> >> On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero >> wrote: >> >>> >> >>> At high level I agree, just have 3 more thoughts: >> >>> >> >>> # Regarding the "swap" of information between listeners - could that >> >>> even work? I might have misunderstood something, but wouldn't we >> >>> require listeners to run in some specific order for such swapping to >> >>> work? >> >> >> >> >> >> This is why we allow control over the ordering of the registered >> listeners. And yes, that is and was a hokey solution. Nothing changes >> there really if that is why you are using load listener. >> >> >> >> >> >>> >> >>> # The "surround advice" you mention for e.g. timing seems very >> >>> interesting, especially as I'd love us to be able to integrate with >> >>> tracing libraries - but these would need to be able to co-relate the >> >>> pre-load event with some post-load event. How would that work? I'd >> >>> expect these to need having a single listener implementation which >> >>> implements both PreLoadEventListener and PostLoadEventListener, but >> >>> also they'll likely need some capability to store some information >> >>> contextual to the "event". >> >> >> >> >> >> I was just thinking through this one as well. My initial thought was >> exactly what you proposed - some combination of pre/post listener with some >> ability to store state between. But that gets ugly. >> >> >> >> Another option I thought about is easier to illustrate, but basically >> works on the principle of "continuation" many surround advice solutions are >> based on: >> https://gist.github.com/sebersole/142765fe2417492061e92726e7cb6bd8 >> >> >> >> I kept the name LoadEventListener there, but since it changes the >> contract anyway I'd probably rename this to something like >
Re: [hibernate-dev] v6 and load-event
Hm, the dynamic enable/disable stuff should be easy to handle, no? Depends on what specific library you are thinking of and exactly how that detail gets propagated to us. At the end of the day, its really as simple as protecting the creation of some of these objects with `if (enabled)`-type checks. But again, if you have specific details in mind we can take a look. Also, I think it is not at all a good idea to even plan for "different types of events". In fact I'm fine with getting rid of LoadEvent completely from that contract and simply directly passing the information that is likely useful. I mean at the end of the day a listener for load events is going to be interested in the same set of information. Yes, some will not need all of that information but that's not really a concern IMO. Especially if we inline the parameters and completely avoid the event object instantiation Regarding incremental addition of "surround listeners", so long as we are all in agreement that this simply means there will be absolutely no surround capability then I am fine with that. On Thu, May 28, 2020 at 3:55 PM Sanne Grinovero wrote: > On Thu, 28 May 2020 at 21:27, Steve Ebersole wrote: > > > > Any thoughts on this "continuation" approach? > > I love the pattern! Maybe we'll need also some ability to not capture > the state for events which don't have any? > > I wonder if that implies we'll need two different event contracts: one > for the listeners which need state and one for those which don't; but > I'm not eager to overcomplicate this. > > > Or maybe its just not important (yet) to handle "surround" handling? > > I'm confident that integration with tracing libraries would be very > useful and interesting to have - but indeed not having time to > research it properly I'm a bit afraid that it might need further > changes to reach excellent performance. > > For example one thing I remember is that with some libraries you're > supposed to have the option to enable/disable the profiling options > dynamically, and since there's an expectation of no overhead when it's > disabled this would need to imply having a way for the overhead of > allocating space for the captured state to "vanish": this might be a > bit more complicated, or need to be able to take advantage of JIT > optimisations. > > So if we end up thinking that such event APIs need to be different > depending on the need for state, perhaps indeed it's better to > postpone the design of those with state to when someone has time to > research an optimal integration with a tracing library. It might not > be too hard, I just haven't explored it myself yet. > > Maybe let's do this incrementally, considering the "continuation" > approach a next step? > > Thanks, > Sanne > > > > > On Wed, May 27, 2020 at 9:27 AM Steve Ebersole > wrote: > >> > >> Inline... > >> > >> On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero > wrote: > >>> > >>> At high level I agree, just have 3 more thoughts: > >>> > >>> # Regarding the "swap" of information between listeners - could that > >>> even work? I might have misunderstood something, but wouldn't we > >>> require listeners to run in some specific order for such swapping to > >>> work? > >> > >> > >> This is why we allow control over the ordering of the registered > listeners. And yes, that is and was a hokey solution. Nothing changes > there really if that is why you are using load listener. > >> > >> > >>> > >>> # The "surround advice" you mention for e.g. timing seems very > >>> interesting, especially as I'd love us to be able to integrate with > >>> tracing libraries - but these would need to be able to co-relate the > >>> pre-load event with some post-load event. How would that work? I'd > >>> expect these to need having a single listener implementation which > >>> implements both PreLoadEventListener and PostLoadEventListener, but > >>> also they'll likely need some capability to store some information > >>> contextual to the "event". > >> > >> > >> I was just thinking through this one as well. My initial thought was > exactly what you proposed - some combination of pre/post listener with some > ability to store state between. But that gets ugly. > >> > >> Another option I thought about is easier to illustrate, but basically > works on the principle of "continuation" many surround advice solutions are > based on: > https://gist.github.com/sebersole/142765fe2417492061e92726e7cb6bd8 > >> > >> I kept the name LoadEventListener there, but since it changes the > contract anyway I'd probably rename this to something like > SurroundLoadEventListener > >> > >> > >>> > >>> # To clarify on my previous comment regarding why I'd consider having > >>> an actual Event class more maintainable: > >>> Sure we won't have inline classes widely used for a while, but I > >>> prefer planning for the long term - also we could start using them > >>> very soon via multi-release jars, which would simply imply that users > >>> on newer JDKs w
Re: [hibernate-dev] v6 and load-event
On Thu, 28 May 2020 at 21:27, Steve Ebersole wrote: > > Any thoughts on this "continuation" approach? I love the pattern! Maybe we'll need also some ability to not capture the state for events which don't have any? I wonder if that implies we'll need two different event contracts: one for the listeners which need state and one for those which don't; but I'm not eager to overcomplicate this. > Or maybe its just not important (yet) to handle "surround" handling? I'm confident that integration with tracing libraries would be very useful and interesting to have - but indeed not having time to research it properly I'm a bit afraid that it might need further changes to reach excellent performance. For example one thing I remember is that with some libraries you're supposed to have the option to enable/disable the profiling options dynamically, and since there's an expectation of no overhead when it's disabled this would need to imply having a way for the overhead of allocating space for the captured state to "vanish": this might be a bit more complicated, or need to be able to take advantage of JIT optimisations. So if we end up thinking that such event APIs need to be different depending on the need for state, perhaps indeed it's better to postpone the design of those with state to when someone has time to research an optimal integration with a tracing library. It might not be too hard, I just haven't explored it myself yet. Maybe let's do this incrementally, considering the "continuation" approach a next step? Thanks, Sanne > > On Wed, May 27, 2020 at 9:27 AM Steve Ebersole wrote: >> >> Inline... >> >> On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero wrote: >>> >>> At high level I agree, just have 3 more thoughts: >>> >>> # Regarding the "swap" of information between listeners - could that >>> even work? I might have misunderstood something, but wouldn't we >>> require listeners to run in some specific order for such swapping to >>> work? >> >> >> This is why we allow control over the ordering of the registered listeners. >> And yes, that is and was a hokey solution. Nothing changes there really if >> that is why you are using load listener. >> >> >>> >>> # The "surround advice" you mention for e.g. timing seems very >>> interesting, especially as I'd love us to be able to integrate with >>> tracing libraries - but these would need to be able to co-relate the >>> pre-load event with some post-load event. How would that work? I'd >>> expect these to need having a single listener implementation which >>> implements both PreLoadEventListener and PostLoadEventListener, but >>> also they'll likely need some capability to store some information >>> contextual to the "event". >> >> >> I was just thinking through this one as well. My initial thought was >> exactly what you proposed - some combination of pre/post listener with some >> ability to store state between. But that gets ugly. >> >> Another option I thought about is easier to illustrate, but basically works >> on the principle of "continuation" many surround advice solutions are based >> on: https://gist.github.com/sebersole/142765fe2417492061e92726e7cb6bd8 >> >> I kept the name LoadEventListener there, but since it changes the contract >> anyway I'd probably rename this to something like SurroundLoadEventListener >> >> >>> >>> # To clarify on my previous comment regarding why I'd consider having >>> an actual Event class more maintainable: >>> Sure we won't have inline classes widely used for a while, but I >>> prefer planning for the long term - also we could start using them >>> very soon via multi-release jars, which would simply imply that users >>> on newer JDKs would see more benefits than other users. >>> But especially, such event instances are passed over and over across >>> many methods; so in terms of maintenance and readability, such methods >>> would need to pass many parameters rather than one: the example made >>> above is oversimplifying our use. Also while I understand it's >>> unlikely, having a "cheap" event objects makes it easier to change the >>> exact types being passed on. >>> BTW stack space is cheap but forcing many references to be passed when >>> one single reference could do might also have some performance >>> implications since these are passed many times - I've never tested >>> this scientifically though :) Inline objects would typically be >>> allocated on the stack as well, but they don't force the JVM to do so. >>> >>> >>> Also while I said that it's unlikely we want to change those types, >>> the very coming of inline types might actually encourage us to make >>> changes in this area, even though these events have been stable for >>> years; for example "String entityName" seems like an excellent >>> candidate to become "EntityName typeIdentifier" - and then allow us to >>> improve the persister maps, which have been a bottleneck in the past. >>> So sure we could remove them and just pass parameters
Re: [hibernate-dev] v6 and load-event
Any thoughts on this "continuation" approach? Or maybe its just not important (yet) to handle "surround" handling? On Wed, May 27, 2020 at 9:27 AM Steve Ebersole wrote: > Inline... > > On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero > wrote: > >> At high level I agree, just have 3 more thoughts: >> >> # Regarding the "swap" of information between listeners - could that >> even work? I might have misunderstood something, but wouldn't we >> require listeners to run in some specific order for such swapping to >> work? >> > > This is why we allow control over the ordering of the registered > listeners. And yes, that is and was a hokey solution. Nothing changes > there really if that is why you are using load listener. > > > >> # The "surround advice" you mention for e.g. timing seems very >> interesting, especially as I'd love us to be able to integrate with >> tracing libraries - but these would need to be able to co-relate the >> pre-load event with some post-load event. How would that work? I'd >> expect these to need having a single listener implementation which >> implements both PreLoadEventListener and PostLoadEventListener, but >> also they'll likely need some capability to store some information >> contextual to the "event". >> > > I was just thinking through this one as well. My initial thought was > exactly what you proposed - some combination of pre/post listener with some > ability to store state between. But that gets ugly. > > Another option I thought about is easier to illustrate, but basically > works on the principle of "continuation" many surround advice solutions are > based on: > https://gist.github.com/sebersole/142765fe2417492061e92726e7cb6bd8 > > I kept the name LoadEventListener there, but since it changes the contract > anyway I'd probably rename this to something like SurroundLoadEventListener > > > >> # To clarify on my previous comment regarding why I'd consider having >> an actual Event class more maintainable: >> Sure we won't have inline classes widely used for a while, but I >> prefer planning for the long term - also we could start using them >> very soon via multi-release jars, which would simply imply that users >> on newer JDKs would see more benefits than other users. >> But especially, such event instances are passed over and over across >> many methods; so in terms of maintenance and readability, such methods >> would need to pass many parameters rather than one: the example made >> above is oversimplifying our use. Also while I understand it's >> unlikely, having a "cheap" event objects makes it easier to change the >> exact types being passed on. >> BTW stack space is cheap but forcing many references to be passed when >> one single reference could do might also have some performance >> implications since these are passed many times - I've never tested >> this scientifically though :) Inline objects would typically be >> allocated on the stack as well, but they don't force the JVM to do so. > > >> Also while I said that it's unlikely we want to change those types, >> the very coming of inline types might actually encourage us to make >> changes in this area, even though these events have been stable for >> years; for example "String entityName" seems like an excellent >> candidate to become "EntityName typeIdentifier" - and then allow us to >> improve the persister maps, which have been a bottleneck in the past. >> So sure we could remove them and just pass parameters, we'd just need >> to change more code if such a situation arises - I'm just highliting >> the drawbacks for our consideration, not recommending against it :) >> > > Maybe its simply a difference of wording, but to me none of this validates > how keeping an event class is more maintainable. If you want to say that > eventually the overhead of having an actual event class will be less, ok, > but that's different. > > For sure though we'd have lots of uses for in-line value types throughout > the code base. Just not sure this really an argument for keeping the event > impl in-and-of-itself. > > ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] v6 and load-event
Inline... On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero wrote: > At high level I agree, just have 3 more thoughts: > > # Regarding the "swap" of information between listeners - could that > even work? I might have misunderstood something, but wouldn't we > require listeners to run in some specific order for such swapping to > work? > This is why we allow control over the ordering of the registered listeners. And yes, that is and was a hokey solution. Nothing changes there really if that is why you are using load listener. > # The "surround advice" you mention for e.g. timing seems very > interesting, especially as I'd love us to be able to integrate with > tracing libraries - but these would need to be able to co-relate the > pre-load event with some post-load event. How would that work? I'd > expect these to need having a single listener implementation which > implements both PreLoadEventListener and PostLoadEventListener, but > also they'll likely need some capability to store some information > contextual to the "event". > I was just thinking through this one as well. My initial thought was exactly what you proposed - some combination of pre/post listener with some ability to store state between. But that gets ugly. Another option I thought about is easier to illustrate, but basically works on the principle of "continuation" many surround advice solutions are based on: https://gist.github.com/sebersole/142765fe2417492061e92726e7cb6bd8 I kept the name LoadEventListener there, but since it changes the contract anyway I'd probably rename this to something like SurroundLoadEventListener > # To clarify on my previous comment regarding why I'd consider having > an actual Event class more maintainable: > Sure we won't have inline classes widely used for a while, but I > prefer planning for the long term - also we could start using them > very soon via multi-release jars, which would simply imply that users > on newer JDKs would see more benefits than other users. > But especially, such event instances are passed over and over across > many methods; so in terms of maintenance and readability, such methods > would need to pass many parameters rather than one: the example made > above is oversimplifying our use. Also while I understand it's > unlikely, having a "cheap" event objects makes it easier to change the > exact types being passed on. > BTW stack space is cheap but forcing many references to be passed when > one single reference could do might also have some performance > implications since these are passed many times - I've never tested > this scientifically though :) Inline objects would typically be > allocated on the stack as well, but they don't force the JVM to do so. > Also while I said that it's unlikely we want to change those types, > the very coming of inline types might actually encourage us to make > changes in this area, even though these events have been stable for > years; for example "String entityName" seems like an excellent > candidate to become "EntityName typeIdentifier" - and then allow us to > improve the persister maps, which have been a bottleneck in the past. > So sure we could remove them and just pass parameters, we'd just need > to change more code if such a situation arises - I'm just highliting > the drawbacks for our consideration, not recommending against it :) > Maybe its simply a difference of wording, but to me none of this validates how keeping an event class is more maintainable. If you want to say that eventually the overhead of having an actual event class will be less, ok, but that's different. For sure though we'd have lots of uses for in-line value types throughout the code base. Just not sure this really an argument for keeping the event impl in-and-of-itself. ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] v6 and load-event
At high level I agree, just have 3 more thoughts: # Regarding the "swap" of information between listeners - could that even work? I might have misunderstood something, but wouldn't we require listeners to run in some specific order for such swapping to work? # The "surround advice" you mention for e.g. timing seems very interesting, especially as I'd love us to be able to integrate with tracing libraries - but these would need to be able to co-relate the pre-load event with some post-load event. How would that work? I'd expect these to need having a single listener implementation which implements both PreLoadEventListener and PostLoadEventListener, but also they'll likely need some capability to store some information contextual to the "event". # To clarify on my previous comment regarding why I'd consider having an actual Event class more maintainable: Sure we won't have inline classes widely used for a while, but I prefer planning for the long term - also we could start using them very soon via multi-release jars, which would simply imply that users on newer JDKs would see more benefits than other users. But especially, such event instances are passed over and over across many methods; so in terms of maintenance and readability, such methods would need to pass many parameters rather than one: the example made above is oversimplifying our use. Also while I understand it's unlikely, having a "cheap" event objects makes it easier to change the exact types being passed on. BTW stack space is cheap but forcing many references to be passed when one single reference could do might also have some performance implications since these are passed many times - I've never tested this scientifically though :) Inline objects would typically be allocated on the stack as well, but they don't force the JVM to do so. Also while I said that it's unlikely we want to change those types, the very coming of inline types might actually encourage us to make changes in this area, even though these events have been stable for years; for example "String entityName" seems like an excellent candidate to become "EntityName typeIdentifier" - and then allow us to improve the persister maps, which have been a bottleneck in the past. So sure we could remove them and just pass parameters, we'd just need to change more code if such a situation arises - I'm just highliting the drawbacks for our consideration, not recommending against it :) In summary I'd go with the suggested change; would be great to explore how timings and tracing could use this. Thanks, Sanne On Wed, 27 May 2020 at 12:40, Steve Ebersole wrote: > > The overall concept with event+listeners initially was that listeners would > "collaborate" which each other with the event as a token for common > information. The "result" of the operation was made part of the event to > facilitate this design - the idea being that one listener might swap the > result, etc. But in practice that does not happen essentially ever that I > have seen. > > But if we split this idea of handler from listener, then LoadEvent is no > longer strictly necessary. > > How we handle PreLoadEvent and PostLoadEvent is another question. Maybe here > it is best to maintain the stability of the API, as Yoann points out, to > minimize the work for upgrading for users who supply custom events. Assuming > we do keep those pre/post events as-is (dropping PreLoadEventListener and > PostLoadEventListener from my gist), then I completely agree about delaying > the creation of those events until we need them... and reusing those created > instances. > > The "swap" use case I mentioned earlier is something important to keep in > mind. Do we feel like we need such a feature? Keeping the pre/post load > stuff working as-is would allow for that. Otherwise we'd need to decide if > that is important and decide how to allow it in the new contract. > > Sounds like we all agree dropping LoadEvent and LoadEventListener in favor of > LoadEventHandler is a good idea. > > How does everyone feel about PreLoadEventListener / PostLoadEventListener > versus the existing pre/post-load event and listeners? I think it's also a > good idea to understand the ways a user might want to use a listener and > advice based on what they want to achieve: > > pre-event notification > post-event notification > pre-event validation > post-event validation > result swapping > "surround advice" - pre/post combo (timing, etc) > others? > > I think those use-cases fit in with either approach. What do y'all think? > > > On Wed, May 27, 2020 at 2:18 AM Yoann Rodiere wrote: >> >> Hi, >> >> > I think that's a great idea >> >> Same here! >> >> Another advantage is we separate the "SPI"/internal interface (Handler) from >> the API that can be implemented by users (listeners). >> That could be a great help moving forward to evolve Hibernate ORM without >> breaking APIs. >> >> > but I wonder about the readability of the cod
Re: [hibernate-dev] v6 and load-event
The overall concept with event+listeners initially was that listeners would
"collaborate" which each other with the event as a token for common
information. The "result" of the operation was made part of the event to
facilitate this design - the idea being that one listener might swap the
result, etc. But in practice that does not happen essentially ever that I
have seen.
But if we split this idea of handler from listener, then LoadEvent is no
longer strictly necessary.
How we handle PreLoadEvent and PostLoadEvent is another question. Maybe
here it is best to maintain the stability of the API, as Yoann points out,
to minimize the work for upgrading for users who supply custom events.
Assuming we do keep those pre/post events as-is (dropping
PreLoadEventListener and PostLoadEventListener from my gist), then I
completely agree about delaying the creation of those events until we need
them... and reusing those created instances.
The "swap" use case I mentioned earlier is something important to keep in
mind. Do we feel like we need such a feature? Keeping the pre/post load
stuff working as-is would allow for that. Otherwise we'd need to decide if
that is important and decide how to allow it in the new contract.
Sounds like we all agree dropping LoadEvent and LoadEventListener in favor
of LoadEventHandler is a good idea.
How does everyone feel about PreLoadEventListener / PostLoadEventListener
versus the existing pre/post-load event and listeners? I think it's also a
good idea to understand the ways a user might want to use a listener and
advice based on what they want to achieve:
- pre-event notification
- post-event notification
- pre-event validation
- post-event validation
- result swapping
- "surround advice" - pre/post combo (timing, etc)
- others?
I think those use-cases fit in with either approach. What do y'all think?
On Wed, May 27, 2020 at 2:18 AM Yoann Rodiere wrote:
> Hi,
>
> > I think that's a great idea
>
> Same here!
>
> Another advantage is we separate the "SPI"/internal interface (Handler)
> from the API that can be implemented by users (listeners).
> That could be a great help moving forward to evolve Hibernate ORM without
> breaking APIs.
>
> > but I wonder about the readability of the code.
>
> I was thinking that it was actually *more* readable. Maybe it's just a
> matter of taste, but this:
>
> Object loaded = handler.load( param1, param2, ... )
>
> Seems considerably more straightforward than this:
>
> LoadEvent event = new LoadEvent( param1, param2, ... )
> for (LoadListener listener: listeners) {
>listener.onLoad( event );
> }
> Object loaded = event.getLoaded();
>
> Especially if you consider that with the handler, the implementation of
> loading is just one CTRL+click away.
> With listeners you have to go through all the listener implementations and
> find which one actually implements loading.
> Not a problem with load since there's only one implementation, but I know
> I've had trouble with other events in the past.
>
> The devil is in the details, though. I admit it could get more complex if
> multiple return values are necessary, and maybe there are other problems
> with this approach that we can't see right away.
> But still, I think it's worth a try.
>
> > With "inline types" coming soon to the JDK, the event object
> types could be great candidates to be converted into inline?
>
> Aren't you talking about JDKs that we won't be able to use as the baseline
> for Hibernate ORM for a few years at least? If so, Steve's change seems
> worthwhile, if only in the meantime.
>
>
>
> Yoann Rodière
> Hibernate Team
> yo...@hibernate.org
>
>
> On Tue, 26 May 2020 at 20:17, Sanne Grinovero wrote:
>
>> Hi Steve,
>>
>> looks like you're getting rid of the "event object"? No more events to
>> be allocated?
>>
>> I think that's a great idea, but I wonder about the readability of the
>> code. With "inline types" coming soon to the JDK, the event object
>> types could be great candidates to be converted into inline?
>>
>> If we used the inline types for such things I'm confident that
>> allocation would improve significantly; it's undeniable that not
>> having anything-at-all would be even better - but perhaps it could be
>> a better tradeoff in consideration of maintainability.
>>
>> I also noticed that it's quite possible that some "list of
>> eventlisteners" for a specific event type could be empty: no listeners
>> at all.
>> So an aspect that could be worth exploring while thinking about the
>> event system design, is to avoid creating en event in such cases;
>> there's an example of this approach already in
>> org.hibernate.internal.SessionImpl#internalClear : the signature I had
>> to use is a bit puzzling, but it manages to avoid allocating the event
>> (unless needed) and also avoids allocating the iterator on the
>> listeners.
>>
>> For example PostLoad events by default have a single listener, but
>> looking more closely I believe we could av
Re: [hibernate-dev] v6 and load-event
Hi,
> I think that's a great idea
Same here!
Another advantage is we separate the "SPI"/internal interface (Handler)
from the API that can be implemented by users (listeners).
That could be a great help moving forward to evolve Hibernate ORM without
breaking APIs.
> but I wonder about the readability of the code.
I was thinking that it was actually *more* readable. Maybe it's just a
matter of taste, but this:
Object loaded = handler.load( param1, param2, ... )
Seems considerably more straightforward than this:
LoadEvent event = new LoadEvent( param1, param2, ... )
for (LoadListener listener: listeners) {
listener.onLoad( event );
}
Object loaded = event.getLoaded();
Especially if you consider that with the handler, the implementation of
loading is just one CTRL+click away.
With listeners you have to go through all the listener implementations and
find which one actually implements loading.
Not a problem with load since there's only one implementation, but I know
I've had trouble with other events in the past.
The devil is in the details, though. I admit it could get more complex if
multiple return values are necessary, and maybe there are other problems
with this approach that we can't see right away.
But still, I think it's worth a try.
> With "inline types" coming soon to the JDK, the event object
types could be great candidates to be converted into inline?
Aren't you talking about JDKs that we won't be able to use as the baseline
for Hibernate ORM for a few years at least? If so, Steve's change seems
worthwhile, if only in the meantime.
Yoann Rodière
Hibernate Team
yo...@hibernate.org
On Tue, 26 May 2020 at 20:17, Sanne Grinovero wrote:
> Hi Steve,
>
> looks like you're getting rid of the "event object"? No more events to
> be allocated?
>
> I think that's a great idea, but I wonder about the readability of the
> code. With "inline types" coming soon to the JDK, the event object
> types could be great candidates to be converted into inline?
>
> If we used the inline types for such things I'm confident that
> allocation would improve significantly; it's undeniable that not
> having anything-at-all would be even better - but perhaps it could be
> a better tradeoff in consideration of maintainability.
>
> I also noticed that it's quite possible that some "list of
> eventlisteners" for a specific event type could be empty: no listeners
> at all.
> So an aspect that could be worth exploring while thinking about the
> event system design, is to avoid creating en event in such cases;
> there's an example of this approach already in
> org.hibernate.internal.SessionImpl#internalClear : the signature I had
> to use is a bit puzzling, but it manages to avoid allocating the event
> (unless needed) and also avoids allocating the iterator on the
> listeners.
>
> For example PostLoad events by default have a single listener, but
> looking more closely I believe we could avoid registering that single
> listener depending on configuration.
>
> Thanks,
> Sanne
>
>
>
> On Tue, 26 May 2020 at 16:00, Steve Ebersole wrote:
> >
> > Historically we made a terrible design mistake with the event system as a
> > whole. This has both a confusing design impact and a very real
> performance
> > impact. The main problem is the smashing together of things that handle
> > events and things that listen to such events.
> >
> > In working on a problem in v6 I have come to a point where fixing this
> > design flaw for load events specifically would be a huge help. So I'm
> > going to make a proposal for how to specifically change this for load
> event
> > handling. The plan at the moment is to not make similar changes to the
> > other event types for now, though I certainly would like to keep them all
> > in mind with regard to the overall design for if/when we can get to the
> > others.
> >
> > So I'd love feedback specifically regarding (1) general design
> considering
> > other event types and (2) issues/concerns specific to load event type.
> >
> > I created a simple example at
> > https://gist.github.com/sebersole/2a1c3ac010a166fc91e62b088179678d
> >
> > Thanks!
> > ___
> > hibernate-dev mailing list
> > hibernate-dev@lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
> >
> ___
> hibernate-dev mailing list
> hibernate-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
>
___
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
Re: [hibernate-dev] v6 and load-event
Hi Steve, looks like you're getting rid of the "event object"? No more events to be allocated? I think that's a great idea, but I wonder about the readability of the code. With "inline types" coming soon to the JDK, the event object types could be great candidates to be converted into inline? If we used the inline types for such things I'm confident that allocation would improve significantly; it's undeniable that not having anything-at-all would be even better - but perhaps it could be a better tradeoff in consideration of maintainability. I also noticed that it's quite possible that some "list of eventlisteners" for a specific event type could be empty: no listeners at all. So an aspect that could be worth exploring while thinking about the event system design, is to avoid creating en event in such cases; there's an example of this approach already in org.hibernate.internal.SessionImpl#internalClear : the signature I had to use is a bit puzzling, but it manages to avoid allocating the event (unless needed) and also avoids allocating the iterator on the listeners. For example PostLoad events by default have a single listener, but looking more closely I believe we could avoid registering that single listener depending on configuration. Thanks, Sanne On Tue, 26 May 2020 at 16:00, Steve Ebersole wrote: > > Historically we made a terrible design mistake with the event system as a > whole. This has both a confusing design impact and a very real performance > impact. The main problem is the smashing together of things that handle > events and things that listen to such events. > > In working on a problem in v6 I have come to a point where fixing this > design flaw for load events specifically would be a huge help. So I'm > going to make a proposal for how to specifically change this for load event > handling. The plan at the moment is to not make similar changes to the > other event types for now, though I certainly would like to keep them all > in mind with regard to the overall design for if/when we can get to the > others. > > So I'd love feedback specifically regarding (1) general design considering > other event types and (2) issues/concerns specific to load event type. > > I created a simple example at > https://gist.github.com/sebersole/2a1c3ac010a166fc91e62b088179678d > > Thanks! > ___ > hibernate-dev mailing list > hibernate-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/hibernate-dev > ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
[hibernate-dev] v6 and load-event
Historically we made a terrible design mistake with the event system as a whole. This has both a confusing design impact and a very real performance impact. The main problem is the smashing together of things that handle events and things that listen to such events. In working on a problem in v6 I have come to a point where fixing this design flaw for load events specifically would be a huge help. So I'm going to make a proposal for how to specifically change this for load event handling. The plan at the moment is to not make similar changes to the other event types for now, though I certainly would like to keep them all in mind with regard to the overall design for if/when we can get to the others. So I'd love feedback specifically regarding (1) general design considering other event types and (2) issues/concerns specific to load event type. I created a simple example at https://gist.github.com/sebersole/2a1c3ac010a166fc91e62b088179678d Thanks! ___ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
