Skip to content

[SYCL] persistent cache fix - directory creation and reporting improvements #13019

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Mar 28, 2024
Merged

[SYCL] persistent cache fix - directory creation and reporting improvements #13019

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
99c54b9
minor improvement to reporting system err on cache file creation
cperkinsintel Mar 13, 2024
9febda6
interim commit, need to switch
cperkinsintel Mar 13, 2024
bbee794
overly cautious error reporting
cperkinsintel Mar 14, 2024
5ff82ea
clang-format
cperkinsintel Mar 14, 2024
4e55d59
experimental
cperkinsintel Mar 16, 2024
a234292
inclusion
cperkinsintel Mar 16, 2024
fc6095c
adding arbitrary traces to try and understand what's going on.
cperkinsintel Mar 16, 2024
79d2e28
working and tested
cperkinsintel Mar 16, 2024
94eca4f
ensure no abi break
cperkinsintel Mar 16, 2024
ecffb6b
rework makeDir fix, but leaving old version intact for GCC < 8
cperkinsintel Mar 26, 2024
4bc8642
resolve merge conflicts
cperkinsintel Mar 26, 2024
533cbd4
dupes in merge. hmm
cperkinsintel Mar 26, 2024
d53d95d
another dupe from the merge
cperkinsintel Mar 27, 2024
912e565
reviewer feedback
cperkinsintel Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sycl/include/sycl/detail/os_util.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ class __SYCL_EXPORT OSUtil {
/// Deallocates the memory referenced by \p Ptr.
static void alignedFree(void *Ptr);

/// Make directory recursively and returns zero code on success
/// Make all directories on the path, throws on error.
static int makeDir(const char *Dir);

/// Checks if specified path is present
Expand Down
23 changes: 17 additions & 6 deletions sycl/source/detail/os_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
#include <cassert>
#include <limits>

#if __GNUC__ && __GNUC__ < 8
// Don't include <filesystem> for GCC versions less than 8
#else
#include <filesystem> // C++ 17 std::create_directories
#endif

#if defined(__SYCL_RT_OS_LINUX)

#ifndef _GNU_SOURCE
Expand All @@ -32,7 +38,6 @@
#include <detail/windows_os_utils.hpp>

#include <Windows.h>
#include <direct.h>
#include <malloc.h>
#include <shlwapi.h>

Expand Down Expand Up @@ -233,15 +238,14 @@ void OSUtil::alignedFree(void *Ptr) {
#endif
}

/* This is temporary solution until std::filesystem is available when SYCL RT
* is moved to c++17 standard*/

/* Create directory recursively and return non zero code on success*/
// Make all directories on the path, throws on error.
int OSUtil::makeDir(const char *Dir) {
assert((Dir != nullptr) && "Passed null-pointer as directory name.");
if (isPathPresent(Dir))
return 0;

// older GCC doesn't have full C++ 17 support.
#if __GNUC__ && __GNUC__ < 8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we throw in this case as well, to align the two branches?

Copy link
Contributor Author

@cperkinsintel cperkinsintel Mar 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing the signature of this function leads to an ABI break. Since we need to keep this old stuff anyway, I was thinking to keep it as is.
My preference would be to change the signature, throw always, and drop GCC < 8 support, but that decision isn't mine to make.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can keep the signature as is and still throw when we would have returned a non-zero code. I don't think we're handling non-zero codes returned from this anyway, are we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::fileystem::create_directories throws a std::filesystem::filesystem_error but the "old" code doesn't have std::filesystem so can't throw that. I'm modifying it to throw std::runtime_error

std::string Path{Dir}, CurPath;
size_t pos = 0;

Expand All @@ -254,8 +258,15 @@ int OSUtil::makeDir(const char *Dir) {
auto Res = _mkdir(CurPath.c_str());
#endif
if (Res && errno != EEXIST)
return Res;
throw std::runtime_error("Failed to mkdir: " + CurPath + " (" +
std::strerror(errno) + ")");

} while (pos != std::string::npos);
#else
// using filesystem is simpler, more reliable, works better on Win
std::filesystem::path path(Dir);
std::filesystem::create_directories(path.make_preferred());
#endif
return 0;
}

Expand Down
28 changes: 20 additions & 8 deletions sycl/source/detail/persistent_device_code_cache.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include <detail/plugin.hpp>
#include <detail/program_manager/program_manager.hpp>

#include <cerrno>
#include <cstdio>
#include <fstream>
#include <optional>
Expand Down Expand Up @@ -38,7 +39,10 @@ LockCacheItem::LockCacheItem(const std::string &Path)
close(fd);
Owned = true;
} else {
PersistentDeviceCodeCache::trace("Failed to aquire lock file: " + FileName);
PersistentDeviceCodeCache::trace("Failed to acquire lock file: " +
FileName + " " + std::strerror(errno));
PersistentDeviceCodeCache::trace("Failed to acquire lock file: " +
FileName + " " + std::strerror(errno));
}
}

Expand Down Expand Up @@ -100,12 +104,6 @@ void PersistentDeviceCodeCache::putItemToDisc(

auto Plugin = detail::getSyclObjImpl(Device)->getPlugin();

size_t i = 0;
std::string FileName;
do {
FileName = DirName + "/" + std::to_string(i++);
} while (OSUtil::isPathPresent(FileName + ".bin"));

unsigned int DeviceNum = 0;

Plugin->call<PiApiKind::piProgramGetInfo>(
Expand All @@ -127,6 +125,12 @@ void PersistentDeviceCodeCache::putItemToDisc(
Plugin->call<PiApiKind::piProgramGetInfo>(NativePrg, PI_PROGRAM_INFO_BINARIES,
sizeof(char *) * Pointers.size(),
Pointers.data(), nullptr);
size_t i = 0;
std::string FileName;
do {
FileName = DirName + "/" + std::to_string(i++);
} while (OSUtil::isPathPresent(FileName + ".bin") ||
OSUtil::isPathPresent(FileName + ".lock"));

try {
OSUtil::makeDir(DirName.c_str());
Expand All @@ -137,9 +141,17 @@ void PersistentDeviceCodeCache::putItemToDisc(
trace("device binary has been cached: " + FullFileName);
writeSourceItem(FileName + ".src", Device, Img, SpecConsts,
BuildOptionsString);
} else {
PersistentDeviceCodeCache::trace("cache lock not owned " + FileName);
}
} catch (std::exception &e) {
PersistentDeviceCodeCache::trace(
std::string("exception encountered making persistent cache: ") +
e.what());
} catch (...) {
// If a problem happens on storing cache item, do nothing
PersistentDeviceCodeCache::trace(
std::string("error outputting persistent cache: ") +
std::strerror(errno));
}
}

Expand Down