Some possible performance improvements

[mannih]

Hi Packy!

It's me again. I've spend some time profiling a little CLI app I made to interact with our JIRA instance at work, trying to make it faster. If you're interested, here is what I found so far:

1. This is a big one. Whenever an instance of JIRA::REST::Class::Issue is created, init() will create a method per issue type like is_bug() or is_story(). But to be able to do this, it needs to query JIRA for a list of all issue types (i.e. it will call $jira->issue_types). Although this is done only once, this call will cost some 500 ms which gives me a noticeable delay. Since all those is_$foo methods aren't even documented, I suggest making this either optional or completely getting rid of it.

2. This can be a big one. JIRA::REST::Class::Mixins::JIRA_REST() gets called quite often. In most cases, this will in turn call JIRA::REST->new(). Now, I use JIRA::REST's ability to load my jira credentials from ~/.jira. If I gpg-encrypt that file like a good employee, all the calls to JIRA::REST->new() result in one gpg decryption per call. My little script takes approximately 1s with a plain text .jira file and 18 s(!) with an encrypted one. Hacking JIRA::REST::Class::Mixins::JIRA_REST() so that it caches the JIRA::REST instance in a package variable, encrypting the file gives me almost no performance penalty.

Please let me know if you want a pull request or two. Or if you have better ideas that tackle those problems.

Cheers,
Manni

[packy]

I love both, and I think I can whip both of them up quickly. For the second, I wouldn't want to cache the instance in a package scalar because I could easily see using two instances to talk either to two servers or connect to the same server as two different users. But if I cache the instance in a hash keyed on the concatenation of the server name and the user name passed into the constructor, then I have the ability to have multiple instances.

…

On Sun, Jan 21, 2018, 5:24 AM mannih ***@***.***> wrote: Hi Packy! It's me again. I've spend some time profiling a little CLI app I made to interact with our JIRA instance at work, trying to make it faster. If you're interested, here is what I found so far: 1. This is a big one. Whenever an instance of JIRA::REST::Class::Issue is created, init() will create a method per issue type like is_bug() or is_story(). But to be able to do this, it needs to query JIRA for a list of all issue types (i.e. it will call $jira->issue_types). Although this is done only once, this call will cost some 500 ms which gives me a noticeable delay. Since all those is_$foo methods aren't even documented, I suggest making this either optional or completely getting rid of it. 2. This can be a big one. JIRA::REST::Class::Mixins::JIRA_REST() gets called quite often. In most cases, this will in turn call JIRA::REST->new(). Now, I use JIRA::REST's ability to load my jira credentials from ~/.jira. If I gpg-encrypt that file like a good employee, all the calls to JIRA::REST->new() result in one gpg decryption per call. My little script takes approximately 1s with a plain text .jira file and 18 s(!) with an encrypted one. Hacking JIRA::REST::Class::Mixins::JIRA_REST() so that it caches the JIRA::REST instance in a package variable, encrypting the file gives me almost no performance penalty. Please let me know if you want a pull request or two. Or if you have better ideas that tackle those problems. Cheers, Manni — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#10>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAUZZL0Qz0lFBPK9XxQ5WdJKwyESGokCks5tMxBEgaJpZM4RlxB1> .

[mannih]

Awesome! Yes, good catch: Just because I don't use another instance doesn't mean nobody else does. I had a feeling that you know more about potential use cases than me ;-) Server and user names as cache keys sound perfect!
Please let me know if I can help.

[packy]

Here's how you can help: test these out for me! Caching should work automatically unless you specifically disable it, but for backward-compatibility reasons, I made creating the is_{issue_type} methods a feature you have to explicitly disable through a no_is_issue_type argument to the top-level constructor.

[mannih]

That was fast! Maybe a wee bit too fast. I created a PR with some fixes: #11

[mannih]

Otherwise, this looks brilliant! Thank you! Profiling now yields very nice results, showing that most of the time is spent somewhere in LWP::UserAgent, i.e. SSL-handshake and JIRA itself.

[packy]

I love this teamwork.

A little later I'll write up the documentation for these changes and close out the issue. Is it important for you to get this change via CPAN? If so, I'll roll a new distribution. If not, I'll document this as the first feature of the next CPAN version and wait a little to see if I can get some more features into the next distro.

[mannih]

Nice teamwork, indeed!

Would it be important to have this on cpan soonish? Let’s say it would be very convenient.

[packy]

Then I'm not waiting for any other features. I've written up documentation for the two new arguments, and once I've figured out what testing I need to do for this and gotten those tests implemented, I'll roll a new CPAN release.

The no_cache tests should be pretty easy; I can test to see that I get back the same object when no_cache is false, and that I get back different objects when no_cache is true. It's the test for no_is_issue_type that's got me thinking. I really wish I could get away from spinning up a fake "JIRA" server to do most of my testing, since adds a bunch of test-only dependencies and doesn't really work that well on Windows.

[mannih]

Thank you!

Yes, testing stuff like this isn't always fun. Ever thought about using Test::LWP::UserAgent? I made a little demo test testing issue_types() and things in JIRA::REST::Class::Issue::Type and I think it might be useful: https://github.com/mannih/JIRA-REST-Class/blob/master/t/02-testserver-communication.t