Cycles: for dynamic kernel compilation, cache only up to 5 kernels of each type
Needs ReviewPublic
Actions

Authored by Brecht Van Lommel (brecht) on Apr 25 2022, 8:25 PM.

Details

Reviewers

Sergey Sharybin (sergey)
Lukas Stockner (lukasstockner97)
Michael Jones (michael_jones)

Summary

To avoid using too much disk space. Whenever a kernel from the cache is used its
lasted modified time is updated. Then when a new kernel is added, the oldest
kernel of the same type is removed.

The number should be high enough that there is little risk of race conditions
with multiple Cycles instances running at the same time.

After OpenCL was removed, this didn't really affect end users much since all
kernels are precompiled not cached. But if we enable the shader cache for Metal
in D14645, it's good to keep disk usage under control. Especially if scene
specialization gets added also.

Diff Detail

Repository

rB Blender

Branch

cache-clear (branched from master)

Build Status

Buildable 21804
Build 21804: arc lint + arc unit

Event Timeline

Brecht Van Lommel (brecht) requested review of this revision.Apr 25 2022, 8:25 PM

Brecht Van Lommel (brecht) created this revision.

Harbormaster completed remote builds in B21804: Diff 50802.Apr 25 2022, 8:25 PM

Wade Lees (wlees) added a subscriber: Wade Lees (wlees).Apr 25 2022, 8:39 PM

Generally seems reasonable. Using the modified date is not ideal since it might e.g. cause unnecessary work for backup tools, but access time is not universal across platforms and file systems so it's probably the best option we got.

intern/cycles/device/cuda/device_impl.cpp
288	It looks like we're adding the `cycles_` prefix for CUDA and HIP, but not Metal. Do we want to handle that consistently? If yes, it might make sense to move it into `path_cache_kernel_get`.
intern/cycles/device/metal/kernel.mm
65	The underscore is already added by `path_cache_kernel_get`. Also, is removing `desc.pso_index` intentional?
70	This line seems outdated, `path_cache_mark_file_used` doesn't exist and `path_cache_kernel_exists_and_mark_used` already marks the file.

This revision now requires changes to proceed.Apr 25 2022, 10:22 PM

Address comments

Harbormaster completed remote builds in B21819: Diff 50845.Apr 26 2022, 4:48 PM

Brecht Van Lommel (brecht) marked 3 inline comments as done.Apr 26 2022, 4:49 PM

Michael Jones (michael_jones) added inline comments.Apr 26 2022, 5:59 PM

intern/cycles/util/path.cpp
941	An alternative to splitting by `_` would be to use nested folders, one per kernel, like in `MetalKernelPipeline::compile` from D14763 which may be neater if we're looking at further specializations.
intern/cycles/util/path.h
67	I think caching up to 5 kernels is reasonable for now, but could be limiting depending on which strategy we use for specialisation. Working backwards from a user controlled cache size is an alternative approach we might want to consider.

Sergey Sharybin (sergey) accepted this revision.Apr 26 2022, 6:08 PM

I'll merge this after D14763: Cycles: Enable inlining on Apple Silicon for 1.1x speedup lands, and resolve any merge conflicts myself.

intern/cycles/util/path.cpp
941	Agree it would be neater, I'm not sure not sure it makes a big practical difference. We already have these caches and it would be nice to clean up existing caches which would be harder when switching to a different convention.
intern/cycles/util/path.h
67	Right, with scene specialization this might need to change. It depends also if we consider specializations to be different kernel types or not.