Add build artifact caching #67

vmrob · 2016-07-09T08:38:33Z

This patch introduces caching that can be made available with needy cache-path set [path] and acts as a key/value store to archive the contents of the build directory after builds. When the cache contains artifacts for a particular build, they are used instead of going through the build process.

vmrob · 2016-07-09T08:39:12Z

Looking for some comments. I still need to write some tests to cover the new stuff, but this implementation currently works on my machine.

vmrob · 2016-07-09T08:42:39Z

needy/__main__.py

+from .caches.directorycache import DirectoryCache
+
+
+def __invoke_with_configured_needy(func, parameters):


Wanted to deduplicate repeated code here, but ugh. I'm not sure of a clean way to do this using the with blocks.

The cleaner way would be with another context manager. This is fine though. Not a big deal.

vmrob · 2016-07-10T23:42:30Z

This PR is still a little bit WIP, but I think the structure is mostly good. RE garbage collection in the base class: I don't think it's applicable here unless we generalize it further with methods for enumerating keys and deleting keys. If that's the case, then GC in the base class essentially becomes something like...

def collect_garbage(self, max_age=timedelta(weeks=2)):
    for key in [k for k in self.keys() if k.age() > max_age]:
        self.expire_key(key)

I'm not saying that's a nice thing to have, I'm just not sure it's within the scope of this first iteration without knowing what we want out of other cache implementations.

So TODO:

Use transactions for keys
Tests
Better garbage collection?

ccbrown · 2016-07-10T23:44:56Z

needy/__main__.py

+  ldflags          emits the linker flags required to use the satisfied needs
+  builddir         emits the build directory for a need
+  sourcedir        emits the source directory for a need
+  pkg-config-path  emits the pkg-config path for a need


Hmm. Why is this pkg-config-path stuff showing up in this diff?

Oh, and you forgot to add that to the epilog before.

vmrob · 2016-07-11T02:09:40Z

Tests and better garbage collection are what remain. I'm not sure if the file locking works or not--about to write some tests for it.

ccbrown · 2016-07-11T03:14:43Z

Notes as discussed in person:

Garbage collection can be built externally if transactions and an unset method are provided by the cache implementation. One possible implementation would be to maintain an index or manifest, and write last-use times to it.
Some potential cache configurables such as garbage collection policy should be set per-cache. Thus their configuration would need to be stored in the cache itself. This emphasizes the need for transactions.

ccbrown · 2016-07-16T10:12:08Z

needy/build_cache.py

+        logging.info('Storing build artifacts in cache')
+        try:
+            with self.__load_manifest() as m:
+                self.__cache.store_directory(directory, key, timeout=self.__lock_timeout)


you could reduce contention if you update the manifest, then release the lock before the store

with self.__load_manifest() as m: m.touch(key) self.__collect_garbage(m) self.__cache.store_directory(directory, key, timeout=self.__lock_timeout)

ccbrown · 2016-07-16T10:20:07Z

needy/build_cache.py

+        for key in [k for k in manifest if manifest[k].use_time < min_time]:
+            try:
+                with self.__cache.lease(key, timeout=0):
+                    self.__cache.unset_key(key)


what happens if you invoke self.__cache.unset_key(key) without a lease?

ccbrown · 2016-07-17T04:19:43Z

needy/__main__.py

+
+    def set_cache_path(args=[]):
+        needs_dir = Needy.resolve_needs_directory('.')
+        default_cache_path = os.path.join(needs_dir, '.cache') if needs_dir else None


we don't currently have a use-case for a default cache in the needs directory, which is already a build cache. i'd leave off the default for now

ccbrown · 2016-07-17T05:15:42Z

Okay so I'm gonna put two hard merge requirements on this:

Make Directory take normal arguments instead of a dictionary. I would consider ways to refactor the serialization / deserialization as well, but it's good enough for now.
I'd like to agree on a set of commands that takes into account future support for other cache types.

vmrob · 2016-07-17T09:27:24Z

I'd like to agree on a set of commands that takes into account future support for other cache types.

I'm all ears. Best I can think of is what you've already suggested or just requiring a modification by hand if you want something other than a directory to be default. I'm totally ok requiring hand-crafted configuration files too.

I'll go ahead and make a needy.caching module tomorrow with cache, build_cache, and manifest. Are you sure about breaking the convention of sources.*, projects.*, and caches.* with needy.caching.directory_cache?

This patch introduces caching that can be made available with `needy cache set [path]` and acts as a key/value store to archive the contents of the build directory after builds. When the cache contains artifacts for a particular build, they are used instead of going through the build process.

vmrob · 2016-07-17T20:51:28Z

Fixed the name issues in main, moved needy.manifest.Manifest to needy.cache.Manifest and moved the tests accordingly.

ccbrown · 2016-07-17T21:30:07Z

Alright, let's do this.

vmrob force-pushed the directory-cache branch from 827fc28 to 980724b Compare July 9, 2016 08:41

vmrob reviewed Jul 9, 2016
View reviewed changes

ccbrown reviewed Jul 10, 2016
View reviewed changes

vmrob changed the title ~~Add build artifact caching~~ WIP: Add build artifact caching Jul 11, 2016

vmrob mentioned this pull request Jul 14, 2016

Refactor file locking code for testing #75

Merged

vmrob force-pushed the directory-cache branch from fb2d91a to 4996142 Compare July 16, 2016 07:50

vmrob changed the title ~~WIP: Add build artifact caching~~ Add build artifact caching Jul 16, 2016

vmrob force-pushed the directory-cache branch 5 times, most recently from 9dea642 to c69ab23 Compare July 16, 2016 10:05

ccbrown reviewed Jul 16, 2016
View reviewed changes

vmrob force-pushed the directory-cache branch from c69ab23 to 169abc7 Compare July 16, 2016 10:12

ccbrown reviewed Jul 16, 2016
View reviewed changes

vmrob force-pushed the directory-cache branch from 169abc7 to cf057a6 Compare July 16, 2016 10:36

vmrob force-pushed the directory-cache branch 3 times, most recently from 7ea22f0 to 0d48fa2 Compare July 16, 2016 23:58

ccbrown reviewed Jul 17, 2016
View reviewed changes

vmrob force-pushed the directory-cache branch from 0d48fa2 to 889f19e Compare July 17, 2016 09:25

vmrob force-pushed the directory-cache branch from 889f19e to b6a42e3 Compare July 17, 2016 20:46

vmrob force-pushed the directory-cache branch from b6a42e3 to e8b559e Compare July 17, 2016 20:50

ccbrown merged commit 752f411 into ccbrown:master Jul 17, 2016

vmrob deleted the directory-cache branch July 18, 2016 03:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add build artifact caching #67

Add build artifact caching #67

vmrob commented Jul 9, 2016

vmrob commented Jul 9, 2016

vmrob Jul 9, 2016

ccbrown Jul 9, 2016

vmrob commented Jul 10, 2016 •

edited

Loading

ccbrown Jul 10, 2016

vmrob Jul 10, 2016

vmrob Jul 10, 2016

vmrob commented Jul 11, 2016

ccbrown commented Jul 11, 2016

ccbrown Jul 16, 2016 •

edited

Loading

ccbrown Jul 16, 2016

ccbrown Jul 17, 2016

ccbrown commented Jul 17, 2016

vmrob commented Jul 17, 2016 •

edited

Loading

vmrob commented Jul 17, 2016

ccbrown commented Jul 17, 2016

		from .caches.directorycache import DirectoryCache


		def __invoke_with_configured_needy(func, parameters):

Add build artifact caching #67

Add build artifact caching #67

Conversation

vmrob commented Jul 9, 2016

vmrob commented Jul 9, 2016

vmrob Jul 9, 2016

Choose a reason for hiding this comment

ccbrown Jul 9, 2016

Choose a reason for hiding this comment

vmrob commented Jul 10, 2016 • edited Loading

ccbrown Jul 10, 2016

Choose a reason for hiding this comment

vmrob Jul 10, 2016

Choose a reason for hiding this comment

vmrob Jul 10, 2016

Choose a reason for hiding this comment

vmrob commented Jul 11, 2016

ccbrown commented Jul 11, 2016

ccbrown Jul 16, 2016 • edited Loading

Choose a reason for hiding this comment

ccbrown Jul 16, 2016

Choose a reason for hiding this comment

ccbrown Jul 17, 2016

Choose a reason for hiding this comment

ccbrown commented Jul 17, 2016

vmrob commented Jul 17, 2016 • edited Loading

vmrob commented Jul 17, 2016

ccbrown commented Jul 17, 2016

vmrob commented Jul 10, 2016 •

edited

Loading

ccbrown Jul 16, 2016 •

edited

Loading

vmrob commented Jul 17, 2016 •

edited

Loading