[hibernate-dev] v6 and load-event

[Yoann Rodiere]

+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
yoann at hibernate.org


On Fri, 29 May 2020 at 07:25, Sanne Grinovero <sanne at hibernate.org> wrote:

> Yes, I agree.
>
> On Thu, 28 May 2020, 22:11 Steve Ebersole, <steve at 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 <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.
>>>> >>
>>>>
>>>