[hibernate-dev] v6 and load-event

[Sanne Grinovero]

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.
>>> >>
>>>
>>