[hibernate-dev] v6 and load-event

[Gail Badner]

I *think* so.

On Fri, Jun 5, 2020 at 12:45 PM Steve Ebersole <steve at hibernate.org> wrote:

> Load event handling does not have "anything" parameters.  Am I
> understanding you correctly?
>
> On Fri, Jun 5, 2020, 11:42 AM Gail Badner <gbadner at redhat.com> wrote:
>
>> Hi Steve,
>>
>> Sorry, I have not read the entire thread carefully, so please disregard
>> if not relevant.
>>
>> Would this provide functionality like what we discussed for an
>> OperationContext?
>>
>> https://hibernate.atlassian.net/browse/HHH-10478
>>
>> Thanks,
>> Gail
>>
>> On Fri, May 29, 2020 at 4:21 AM Steve Ebersole <steve at hibernate.org>
>> wrote:
>>
>>> 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 <sanne at hibernate.org>
>>> wrote:
>>>
>>> > On Fri, 29 May 2020 at 07:22, Yoann Rodiere <yoann at hibernate.org>
>>> 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
>>> > > 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.
>>> > >>>>> >>
>>> >
>>> _______________________________________________
>>> hibernate-dev mailing list
>>> hibernate-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>
>>