+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 <sa...@hibernate.org> wrote: > Yes, I agree. > > On Thu, 28 May 2020, 22:11 Steve Ebersole, <st...@hibernate.org> 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 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 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