[
https://issues.apache.org/jira/browse/OAK-3146?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14644046#comment-14644046
]
Chetan Mehrotra edited comment on OAK-3146 at 7/28/15 9:01 AM:
---------------------------------------------------------------
bq. you filed this issue with the label 'performance' but fixed the issue
without even verifying i.e. writing a benchmark test that would prove that you
actually found/fixed a performance issue.
For me the issue was logical one as its knows that frequent lookup on
ServiceRegistry is slow and becomes a bottleneck. Reproducing such performance
issue is tricky and hence I went ahead with the proposed fix based on _logical_
understanding of the problem and from my previous experience in such issues
bq. in case of a performance issue this should be covered with benchmarks,
such that we reproduce the issue first and then fix it and in addition verify
later on, that the fix doesn't get broken later on
Ack that benchmark is always better.
However for performance issues many times the fix is done based on logical flow
and following best practices. For e.g. if you have a code which makes same
remote call in a loop. Then it can easily be fixed by moving the remote call
out of the loop and reuse the response. This would be considered by me as a
logical fix and having a benchmark in such cases is not _must_.
In similar way here the code was doing a service lookup on each initialize call
which with proposed fix was done once. There was a backing testcase to further
validate that new approach is being used. Based on that I did not worked upon
adding a benchmark. If thats not acceptable to you then thats also fine with
me. Anyways the changes have been reverted and patch is attached for someone to
folowup later
bq. there is not auto-backport for improvements and i want you to revert that.
Have reverted the various changes now
Unassigning the issue now as I would not have time to create a benchmark for
this issue
was (Author: chetanm):
bq. you filed this issue with the label 'performance' but fixed the issue
without even verifying i.e. writing a benchmark test that would prove that you
actually found/fixed a performance issue.
For me the issue was logical one as its knows that frequent lookup on
ServiceRegistry is slow and becomes a bottleneck. Reproducing such performance
issue is tricky and hence I went ahead with the proposed fix based on _logical_
understanding of the problem and from my previous experience in such issues
bq. there is not auto-backport for improvements and i want you to revert that.
Have reverted the various changes now
Unassigning the issue now as I would not have time to create a benchmark for
this issue
> ExternalLoginModuleFactory should inject SyncManager and
> ExternalIdentityProviderManager
> ----------------------------------------------------------------------------------------
>
> Key: OAK-3146
> URL: https://issues.apache.org/jira/browse/OAK-3146
> Project: Jackrabbit Oak
> Issue Type: Improvement
> Components: security
> Reporter: Chetan Mehrotra
> Priority: Minor
> Labels: performance
> Fix For: 1.3.4
>
> Attachments: OAK-3146.patch
>
>
> {{ExternalLoginModule}} currently performs a lookup of {{SyncManager}} and
> {{ExternalIdentityProviderManager}} in initialize call which performs service
> lookup using ServiceTracker. Doing a service lookup in critical path would
> have adverse performance impact.
> Instead of performing the lookup {{ExternalLoginModuleFactory}} should track
> the two manager instances and pass them to {{ExternalLoginModule}}. This
> would ensure that expensive operation like service lookup is not performed in
> critical path
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
