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

Implement a sqlite-based store for the cache #6023

Merged
merged 6 commits into from Dec 10, 2018

Conversation

Projects
None yet
4 participants
@msullivan
Copy link
Collaborator

msullivan commented Dec 7, 2018

File system operations on OS X are pretty slow, and untarring a large
archive of mypy cache information can get pretty slow. Work around
this by using a sqlite database to store the entire cache in one file.

To do this we introduce a generic interface for storing metadata,
called MetadataStore. It presents an essentially key/value
interface. We provide two implementations, one using the existing file
system backing and one using sqlite.

It is enabled with the option --sqlite-cache, but is not the default
yet.

I'm not sure what the right thing to do about testing is. I've tested
it with the default changed, and everything passes.

Implement a sqlite-based store for the cache
File system operations on OS X are pretty slow, and untarring a large
archive of mypy cache information can get pretty slow. Work around
this by using a sqlite database to store the entire cache in one file.

To do this we introduce a generic interface for storing metadata,
called `MetadataStore`. It presents an essentially key/value
interface. We provide two implementations, one using the existing file
system backing and one using sqlite.

It is enabled with the option `--sqlite-cache`, but is not the default
yet.

I'm not sure what the right thing to do about testing is. I've tested
it with the default changed, and everything passes.

@msullivan msullivan requested review from gvanrossum and ilevkivskyi Dec 7, 2018

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks! This looks very good, I just have some documentation suggestions.

Show resolved Hide resolved misc/convert-cache.py Outdated

for s in input.list_all():
# print("Copying", s)
assert output.write(s, input.read(s), input.getmtime(s))

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 7, 2018

Collaborator

I would give a reasonable error message if this fails. Also, do we need to perform a cleanup for partially written cache to disk in this case?

This comment has been minimized.

@msullivan

msullivan Dec 7, 2018

Author Collaborator

It's just a debugging script, so I dunno if it matters?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 7, 2018

Collaborator

Up to you, but if we expect that someone except us will use it (I assume), then think it is better to do it.

Show resolved Hide resolved misc/convert-cache.py Outdated
structure of files.
* A hokey sqlite backed implementation, which basically simulates
the file system in an effort to work around poor file system performance
on OS X.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 7, 2018

Collaborator

What about Windows? Is it also fast or also slow?

This comment has been minimized.

@msullivan

msullivan Dec 7, 2018

Author Collaborator

I have no idea at all

Show resolved Hide resolved mypy/metastore.py
Show resolved Hide resolved mypy/metastore.py
Show resolved Hide resolved mypy/metastore.py Outdated

class SqliteMetadataStore(MetadataStore):
def __init__(self, cache_dir_prefix: str) -> None:
if cache_dir_prefix.startswith('/dev/null'):

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 7, 2018

Collaborator

Why startswith and not ==? I would also documment this behavior in a docstring.

Show resolved Hide resolved mypy/metastore.py
Show resolved Hide resolved misc/convert-cache.py
Show resolved Hide resolved mypy/build.py Outdated
Show resolved Hide resolved mypy/build.py
Show resolved Hide resolved mypy/build.py
Show resolved Hide resolved mypy/metastore.py Outdated
Show resolved Hide resolved mypy/metastore.py Outdated
Show resolved Hide resolved mypy/metastore.py Outdated
Show resolved Hide resolved mypy/metastore.py Outdated
Show resolved Hide resolved mypy/metastore.py
Show resolved Hide resolved mypy/metastore.py

msullivan added some commits Dec 7, 2018

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented on 66f9389 Dec 7, 2018

The cache_map fix LGTM.

msullivan added some commits Dec 7, 2018

@gvanrossum
Copy link
Member

gvanrossum left a comment

LGTM now.

@msullivan msullivan merged commit b3a7f89 into master Dec 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@msullivan msullivan deleted the sqlite branch Dec 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment