Re: [hibernate-dev] v6 and load-event

On Thu, 28 May 2020 at 21:27, Steve Ebersole <steve(a)hibernate.org&gt; 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 <steve(a)hibernate.org&gt; wrote: >> >> Inline... >> >> On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero <sanne(a)hibernate.org&gt; 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. >>