[hibernate-dev] v6 and load-event

[Steve Ebersole]

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