Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Very bad performance since commit 30fab4f9696378c7eac4f439f87e721ae21e784c #114

Closed
derwok opened this issue Feb 6, 2019 · 5 comments
Closed

Comments

@derwok
Copy link

derwok commented Feb 6, 2019

With the current HEAD of master I experience a HUGE drop in performance.
While it normally (last few weeks) took ~30 seconds to download/upload all my 500 vCards. With the current HEAD of master/ branch (commit 22c4512) it now takes like 7 times as long (3:30 minutes) to perform the same task.

I did a git bisect start on commit 729ed75 and then walked through 3 to 4 commits. It boils down to this commit: 30fab4f (commit comment "Fix contacts download empty") - this is the first commit that inceases running time by factor 7.

I did look into the code and the suspicious line for me is this one:

return new Client($this->getClientOptions());

The old code behaved like a Singleton and recycled the client object:

if (!$this->client) {
            $this->client = new Client($this->getClientOptions());
        }

        return $this->client;

The new code creates a new Client object for every vCard we want to download. Seems this is a "costly" operation. I'm insecure if we can just roll back the few lines of "getClient", because I don't completely understand the "mergeOptions" part...

So, no PR from me here... @andig can you take over with the analysis above?

@andig
Copy link
Owner

andig commented Feb 7, 2019

Reason is as you‘ve found the removed cache. Had to do this as it was not obvious that a cached client didn‘t get updated client options. I‘ll readd the cache wirh a twist. Still funny that performance drops so badly as I couldn‘t find anything regarding keep alive or connection reuse in the guzzle docs.

@andig
Copy link
Owner

andig commented Feb 7, 2019

Could you check if #116 works for you? I'm a little cautious as I don't see any difference.

@derwok
Copy link
Author

derwok commented Feb 7, 2019

As already commented on the PR: 👍

@andig andig added the bug label Feb 8, 2019
@andig
Copy link
Owner

andig commented Feb 8, 2019

I'll keep this open as I have another PR in the pipe that implements actual caching of the contacts data. Again, I don't see any difference but it might work for @derwok

@andig
Copy link
Owner

andig commented Feb 12, 2019

Closing, we can revisit caching later...

@andig andig closed this as completed Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants