search

brutasse / graphite-api

Graphite-web, without the interface. Just the rendering HTTP API.
https://graphite-api.readthedocs.io
Apache License 2.0
492 stars 131 forks source link

default whisper reader - number of os.stat >> stat syscalls #213

Closed olevchyk closed 7 years ago

olevchyk commented 7 years ago

super small fix reduces the number of stat syscalls by using scandir libray: https://github.com/benhoyt/scandir scandir has been included in the Python 3.5 standard library as os.scandir()

context: by tracing graphite-api workers on a system with slow disk sub-system, found out that end ts of whisper database query we get by calling os.stat for database, so even if get your data from local carbon-cache instance, you generate abnormal amount of stats calls. In our case graphite workers spend 90% of time on stat calls on the storage subsystem with limited number of IOPS.
After fix, we spend only 25% of time

https://www.python.org/dev/peps/pep-0471/ This new function adds useful functionality and increases the speed of os.walk() by 2-20 times (depending on the platform and file system) by avoiding calls to os.stat() in most cases.

changed function calls only in most expensive places, without trying to change func _find_paths() not be recursive

olevchyk commented 7 years ago

Thank you very much for your comments and review. I have modified the code according to your comments. Please check last build log there are 3 issues:

    • cairocffi is failing to install without specifying version
    • app.py linting failed when I put original version
brutasse commented 7 years ago

Thanks a lot!

Regarding app.py, I believe it's still slightly different than current master. This passes the lint build: https://github.com/brutasse/graphite-api/blob/master/graphite_api/app.py#L463-L466

As for the cairocffi failure, it seems to be isolated to the use of Pypy. I'd rather investigate it separately than forcing the use of an old version.

Happy to merge this once the lint build is green and the additional app.py is removed :)

olevchyk commented 7 years ago

Thank you. sorry it is my fault, I put app.py to wrong place by mistake and didn't even notice that :( recovered original version. Please review.

brutasse commented 7 years ago

Thanks @olevchyk, finally merged!