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

Speed up client schema cache #488

Closed
wants to merge 2 commits into from

Conversation

tiran
Copy link
Member

@tiran tiran commented Feb 20, 2017

It's inefficient to open a zip file over and over again. By loading all
members of the schema cache file at once, the ipa CLI script starts
about 25 to 30% faster for simple cases like help and ping.

Before:

$ time for i in {1..20}; do ./ipa ping >/dev/null; done

real    0m13.608s
user    0m10.316s
sys     0m1.121s

After:

$ time for i in {1..20}; do ./ipa ping >/dev/null; done

real    0m9.330s
user    0m7.635s
sys     0m1.146s

https://fedorahosted.org/freeipa/ticket/6690

Signed-off-by: Christian Heimes cheimes@redhat.com

@ghost ghost self-assigned this Feb 23, 2017
@ghost
Copy link

ghost commented Feb 28, 2017

@tiran Currently the file is first copied into BytesIO and then all reading is done from it. Your modification IMO supersedes the need for the BytesIO copy because everything is read into memory at once. Could you remove it?

It's inefficient to open a zip file over and over again. By loading all
members of the schema cache file at once, the ipa CLI script starts
about 25 to 30% faster for simple cases like help and ping.

Before:

$ time for i in {1..20}; do ./ipa ping >/dev/null; done

real    0m13.608s
user    0m10.316s
sys     0m1.121s

After:

$ time for i in {1..20}; do ./ipa ping >/dev/null; done

real    0m9.330s
user    0m7.635s
sys     0m1.146s

https://fedorahosted.org/freeipa/ticket/6690

Signed-off-by: Christian Heimes <cheimes@redhat.com>
The schema cache used a BytesIO buffer to read/write schema cache before
it got flushed to disk. Since the schema cache is now loaded in one go,
the temporary buffer is no longer needed.

File locking has been replaced with a temporary file and atomic rename.

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@tiran
Copy link
Member Author

tiran commented Feb 28, 2017

@dkupka Makes sense, I dropped the temporary buffer and replaced the file locking logic with tempfile + os.rename.

@ghost
Copy link

ghost commented Mar 1, 2017

The speedup I see is smaller (10-15%) [1] than what you're reporting but that might be caused by the fact that I store the cache on really slow file system (NFS mount). Anyway the changes makes sense, thanks.

[1] https://da.gd/7r75A

@ghost ghost added the ack Pull Request approved, can be merged label Mar 1, 2017
@tiran
Copy link
Member Author

tiran commented Mar 1, 2017

It looks like your IPA server is about half as fast (26sec / 13sec for 20 pings). In absolute numbers, it's still ~2.5 sec faster. In your case, performance probably dominated by server latency.

@MartinBasti
Copy link
Contributor

master:

  • 332dbab Speed up client schema cache
  • 3be696c Drop in-memory copy of schema zip file

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Mar 1, 2017
@MartinBasti MartinBasti closed this Mar 1, 2017
@tiran tiran deleted the cli_fast_schema_cache branch March 2, 2017 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
2 participants