[#2121] Move _key() after reference/null checks in JPABase.equals()

[davidcostanzo]

The pull request moves _key() and adds unit tests for JPABase.equals() to give confidence that there were no behavioral changes.

[asolntsev]

@davidcostanzo Can you describe what problem you are trying to solve? Why the check should be moved? Why are you confident that it will not break existing applications?

[davidcostanzo]

Thanks for considering the pull request. Answers to your questions follow:

Can you describe what problem you are trying to solve?

Briefly, the problem I'm trying to solve is to improve performance of my application. The problem is described abstractly in the lighthouse ticket, but essentially I have an inner loop within a long-running Job which needs to test GenericModel objects for equality and in most cases the objects are equal (if they're not, then some additional work is done). Due to the entity manager's persistence context, objects are not duplicated within the job, so references to objects that are equal are references to the same object. That is, the object references are equal. So the change improves performance.

The application could test for reference equality before calling equals() (which is what I effectively did to work around the problem) but that's ugly because it's the job of equals() to encapsulate equality logic, not the caller.

Why the check should be moved?

The check should be moved because improves in the case where the object references are equal without hurting performance in the other cases. Because of Play's and Hibernate's architecture, object references will usually be equal whenever the objects are equal.

Why are you confident that it will not break existing applications?

I don't see how it could. It's a simple optimization that delays computing a value until it's used and follows the pattern that was recommended in Effective Java. It doesn't change the computed result. I added unit tests to be sure of that.

[asolntsev]

Oh, yes, you are right. I didn't catch that you actually didn't change the behavior. Thank you for the explanation. *@xael-fry recommended to merge this PR.*

…

On May 24, 2017 2:40 AM, "David Costanzo" ***@***.***> wrote: Thanks for considering the pull request. Answers to your questions follow: Can you describe what problem you are trying to solve? Briefly, the problem I'm trying to solve is to improve performance of my application. The problem is described abstractly in the lighthouse ticket <https://play.lighthouseapp.com/projects/57987-play-framework/tickets/2121-jpabaseequals-invokes-_key-before-checking-nullref-equality>, but essentially I have an inner loop within a long-running Job which needs to test GenericModel objects for equality and in most cases the objects are equal (if they're not, then some additional work is done). Due to the entity manager's persistence context, objects are not duplicated within the job, so references to objects that are equal are references to the same object. That is, the object references are equal. So the change improves performance. The application could test for reference equality before calling equals() (which is what I effectively did to work around the problem) but that's ugly because it's the job of equals() to encapsulate equality logic, not the caller. Why the check should be moved? The check should be moved because improves in the case where the object references are equal without hurting performance in the other cases. Because of Play's and Hibernate's architecture, object references will usually be equal whenever the objects are equal. Why are you confident that it will not break existing applications? I don't see how it could. It's a simple optimization that delays computing a value until it's used and follows the pattern that was recommended in Effective Java. It doesn't change the computed result. I added unit tests to be sure of that. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1141 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARE3V-TFbvFqLk-jUfflpgoqnpzx6eJks5r825_gaJpZM4Nc1A-> .