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

fix: list identities tuning #1588

Merged
merged 2 commits into from
Jul 24, 2021
Merged

Conversation

harnash
Copy link
Contributor

@harnash harnash commented Jul 23, 2021

This change should improve the /identities endpoint performance. Right now it uses Eager() method to load identity model associations n times (where n is the number of identities fetched per request, default is 100). This causes 100+1 SQL queries. It is a tradeoff between memory and speed/queries. I found out that on our setup (yes, our DEV DB have significant lag due to some infrastructure setup but it is not crazy high ~100ms which can happen in some low tier setups as well).

Currently queries looks like this:

SELECT * FROM identities LIMIT, OFFSET;
SELECT * FROM identity_verifiable_addresses WHERE identity_id = id1;
SELECT * FROM identity_verifiable_addresses WHERE identity_id = id2;
SELECT * FROM identity_verifiable_addresses WHERE identity_id = id3;
...
SELECT * FROM identity_verifiable_addresses WHERE identity_id = id100;

We propose to use EagerPreload() which will do only one query to fetch associations.
Now the queries would look like this:

SELECT * FROM identities LIMIT, OFFSET;
SELECT * FROM identity_verifiable_addresses WHERE identity_id IN (id1, id2, id3,..., id100);

Performance on master:

Requests      [total, rate, throughput]         1500, 50.03, 0.19
Duration      [total, attack, wait]             42.616s, 29.98s, 12.636s
Latencies     [min, mean, 50, 90, 95, 99, max]  75.896µs, 5.553s, 3.831ms, 21.809s, 26.786s, 30.004s, 30.013s
Bytes In      [total, mean]                     34317, 22.88
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           0.53%
Status Codes  [code:count]                      0:1133  200:8  502:359

performance after the change:

Requests      [total, rate, throughput]         1500, 50.04, 42.72
Duration      [total, attack, wait]             34.269s, 29.978s, 4.291s
Latencies     [min, mean, 50, 90, 95, 99, max]  1.223ms, 3.693s, 2.874s, 6.784s, 8.782s, 16.303s, 23.191s
Bytes In      [total, mean]                     5557366, 3704.91
Bytes Out     [total, mean]                     0, 0.00
Success       [ratio]                           97.60%
Status Codes  [code:count]                      0:34  200:1464  502:2

Memory usage: https://ibb.co/2YS8sgP (those two bumps are tests on the branch with changes so I guess it does require more memory).

docs: https://gobuffalo.io/en/docs/db/relations#loading-associations

That said I'm not 100% sure this is a good change but it should help with high latency DBs and require more memory to operate (still using Argon2Id could mean Kratos will need more memory anyway). Your call!

Related issue(s)

I don't think there is an issue but I'm pretty sure someone mentioned this on Slack one day.

Checklist

@harnash harnash changed the title List identities tuning fix: list identities tuning Jul 23, 2021
@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #1588 (f63ea11) into master (ca15200) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1588   +/-   ##
=======================================
  Coverage   74.17%   74.17%           
=======================================
  Files         257      257           
  Lines       12556    12556           
=======================================
  Hits         9313     9313           
  Misses       2624     2624           
  Partials      619      619           
Impacted Files Coverage Δ
persistence/sql/persister_identity.go 63.29% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca15200...f63ea11. Read the comment docs.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I am a pop maintainer and did not know about this functionality!

@aeneasr aeneasr merged commit de5fb3e into ory:master Jul 24, 2021
@harnash harnash deleted the list_identities_tuning branch July 24, 2021 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants