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

If/when it comes to needing that, I kind of like that continuation design.
But I agree, Yoann is right - let's leave it off until needed or until we
have specific requirements.

Thanks everyone!

On Fri, May 29, 2020 at 2:19 AM Sanne Grinovero <[email protected]> wrote:

> On Fri, 29 May 2020 at 07:22, Yoann Rodiere <[email protected]> wrote:
> >
> > +1 not to add surround capability initially. Sounds better to start
> simple and make things more complex when we actually need it :)
>
> Right. I didn't mean to raise additional requirements without having
> investigated those tracing libraries - what I meant really is just to
> raise awareness that we'll likely need to evolve it further when it
> comes to finally implement such things.
>
> >
> > Yoann Rodière
> > Hibernate Team
> > [email protected]
> >
> >
> > On Fri, 29 May 2020 at 07:25, Sanne Grinovero <[email protected]>
> wrote:
> >>
> >> Yes, I agree.
> >>
> >> On Thu, 28 May 2020, 22:11 Steve Ebersole, <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> wrote:
> >>>>>
> >>>>> On Thu, 28 May 2020 at 21:27, Steve Ebersole <[email protected]>
> 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 <
> [email protected]> wrote:
> >>>>> >>
> >>>>> >> Inline...
> >>>>> >>
> >>>>> >> On Wed, May 27, 2020 at 8:10 AM Sanne Grinovero <
> [email protected]> 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.
> >>>>> >>
>
_______________________________________________
hibernate-dev mailing list
[email protected]
https://lists.jboss.org/mailman/listinfo/hibernate-dev