8:09 a.m.
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?
# 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".
# 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 :)
In summary I'd go with the suggested change; would be great to explore
how timings and tracing could use this.
Thanks,
Sanne
On Wed, 27 May 2020 at 12:40, Steve Ebersole <steve(a)hibernate.org> wrote:
The overall concept with event+listeners initially was that listeners would
"collaborate" which each other with the event as a token for common information.
The "result" of the operation was made part of the event to facilitate this
design - the idea being that one listener might swap the result, etc. But in practice
that does not happen essentially ever that I have seen.
But if we split this idea of handler from listener, then LoadEvent is no longer strictly
necessary.
How we handle PreLoadEvent and PostLoadEvent is another question. Maybe here it is best
to maintain the stability of the API, as Yoann points out, to minimize the work for
upgrading for users who supply custom events. Assuming we do keep those pre/post events
as-is (dropping PreLoadEventListener and PostLoadEventListener from my gist), then I
completely agree about delaying the creation of those events until we need them... and
reusing those created instances.
The "swap" use case I mentioned earlier is something important to keep in mind.
Do we feel like we need such a feature? Keeping the pre/post load stuff working as-is
would allow for that. Otherwise we'd need to decide if that is important and decide
how to allow it in the new contract.
Sounds like we all agree dropping LoadEvent and LoadEventListener in favor of
LoadEventHandler is a good idea.
How does everyone feel about PreLoadEventListener / PostLoadEventListener versus the
existing pre/post-load event and listeners? I think it's also a good idea to
understand the ways a user might want to use a listener and advice based on what they want
to achieve:
pre-event notification
post-event notification
pre-event validation
post-event validation
result swapping
"surround advice" - pre/post combo (timing, etc)
others?
I think those use-cases fit in with either approach. What do y'all think?
On Wed, May 27, 2020 at 2:18 AM Yoann Rodiere <yoann(a)hibernate.org> wrote:
>
> Hi,
>
> > I think that's a great idea
>
> Same here!
>
> Another advantage is we separate the "SPI"/internal interface (Handler)
from the API that can be implemented by users (listeners).
> That could be a great help moving forward to evolve Hibernate ORM without breaking
APIs.
>
> > but I wonder about the readability of the code.
>
> I was thinking that it was actually *more* readable. Maybe it's just a matter of
taste, but this:
>
> Object loaded = handler.load( param1, param2, ... )
>
> Seems considerably more straightforward than this:
>
> LoadEvent event = new LoadEvent( param1, param2, ... )
> for (LoadListener listener: listeners) {
> listener.onLoad( event );
> }
> Object loaded = event.getLoaded();
>
> Especially if you consider that with the handler, the implementation of loading is
just one CTRL+click away.
> With listeners you have to go through all the listener implementations and find which
one actually implements loading.
> Not a problem with load since there's only one implementation, but I know
I've had trouble with other events in the past.
>
> The devil is in the details, though. I admit it could get more complex if multiple
return values are necessary, and maybe there are other problems with this approach that we
can't see right away.
> But still, I think it's worth a try.
>
> > With "inline types" coming soon to the JDK, the event object
> types could be great candidates to be converted into inline?
>
> Aren't you talking about JDKs that we won't be able to use as the baseline
for Hibernate ORM for a few years at least? If so, Steve's change seems worthwhile, if
only in the meantime.
>
>
>
> Yoann Rodière
> Hibernate Team
> yoann(a)hibernate.org
>
>
> On Tue, 26 May 2020 at 20:17, Sanne Grinovero <sanne(a)hibernate.org> wrote:
>>
>> Hi Steve,
>>
>> looks like you're getting rid of the "event object"? No more events
to
>> be allocated?
>>
>> I think that's a great idea, but I wonder about the readability of the
>> code. With "inline types" coming soon to the JDK, the event object
>> types could be great candidates to be converted into inline?
>>
>> If we used the inline types for such things I'm confident that
>> allocation would improve significantly; it's undeniable that not
>> having anything-at-all would be even better - but perhaps it could be
>> a better tradeoff in consideration of maintainability.
>>
>> I also noticed that it's quite possible that some "list of
>> eventlisteners" for a specific event type could be empty: no listeners
>> at all.
>> So an aspect that could be worth exploring while thinking about the
>> event system design, is to avoid creating en event in such cases;
>> there's an example of this approach already in
>> org.hibernate.internal.SessionImpl#internalClear : the signature I had
>> to use is a bit puzzling, but it manages to avoid allocating the event
>> (unless needed) and also avoids allocating the iterator on the
>> listeners.
>>
>> For example PostLoad events by default have a single listener, but
>> looking more closely I believe we could avoid registering that single
>> listener depending on configuration.
>>
>> Thanks,
>> Sanne
>>
>>
>>
>> On Tue, 26 May 2020 at 16:00, Steve Ebersole <steve(a)hibernate.org> wrote:
>> >
>> > Historically we made a terrible design mistake with the event system as a
>> > whole. This has both a confusing design impact and a very real performance
>> > impact. The main problem is the smashing together of things that handle
>> > events and things that listen to such events.
>> >
>> > In working on a problem in v6 I have come to a point where fixing this
>> > design flaw for load events specifically would be a huge help. So I'm
>> > going to make a proposal for how to specifically change this for load event
>> > handling. The plan at the moment is to not make similar changes to the
>> > other event types for now, though I certainly would like to keep them all
>> > in mind with regard to the overall design for if/when we can get to the
>> > others.
>> >
>> > So I'd love feedback specifically regarding (1) general design
considering
>> > other event types and (2) issues/concerns specific to load event type.
>> >
>> > I created a simple example at
>> > https://gist.github.com/sebersole/2a1c3ac010a166fc91e62b088179678d
>> >
>> > Thanks!
>> > _______________________________________________
>> > hibernate-dev mailing list
>> > hibernate-dev(a)lists.jboss.org
>> > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>> >
>> _______________________________________________
>> hibernate-dev mailing list
>> hibernate-dev(a)lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>