Skip to content

Commit 5aa95f1

Browse files
committed
Address shader compiler review feedback locally
1 parent e545d37 commit 5aa95f1

4 files changed

Lines changed: 28 additions & 101 deletions

File tree

include/nbl/asset/utils/IShaderCompiler.h

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,6 @@
1717
#include "nbl/builtin/hlsl/enums.hlsl"
1818

1919
#include <functional>
20-
#include <mutex>
21-
#include <unordered_map>
22-
#include <unordered_set>
2320

2421
namespace nbl::asset
2522
{
@@ -72,9 +69,6 @@ class NBL_API2 IShaderCompiler : public core::IReferenceCounted
7269

7370
protected:
7471
core::smart_refctd_ptr<system::ISystem> m_system;
75-
mutable std::mutex m_cacheMutex;
76-
mutable std::unordered_map<std::string, IIncludeLoader::found_t> m_cache;
77-
mutable std::unordered_set<std::string> m_missingCache;
7872
};
7973

8074
class NBL_API2 CIncludeFinder : public core::IReferenceCounted

src/nbl/asset/utils/IShaderCompiler.cpp

Lines changed: 3 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// This file is part of the "Nabla Engine".
33
// For conditions of distribution and use, see copyright notice in nabla.h
44
#include "nbl/asset/utils/IShaderCompiler.h"
5-
#include "includeResolutionCommon.h"
65
#include "nbl/asset/utils/shadercUtils.h"
76

87
#include <sstream>
@@ -605,49 +604,16 @@ auto IShaderCompiler::CFileSystemIncludeLoader::getInclude(const system::path& s
605604
{
606605
system::path path = (searchPath / includeName).lexically_normal();
607606

608-
const auto cacheKey = path.generic_string();
609-
if (!cacheKey.empty())
610-
{
611-
std::lock_guard<std::mutex> lock(m_cacheMutex);
612-
const auto found = m_cache.find(cacheKey);
613-
if (found != m_cache.end())
614-
{
615-
if (needHash && found->second.hash == core::blake3_hash_t{})
616-
{
617-
core::blake3_hasher hasher;
618-
hasher.update(reinterpret_cast<uint8_t*>(found->second.contents.data()), found->second.contents.size() * (sizeof(char) / sizeof(uint8_t)));
619-
found->second.hash = static_cast<core::blake3_hash_t>(hasher);
620-
}
621-
return found->second;
622-
}
623-
if (m_missingCache.contains(cacheKey))
624-
return {};
625-
}
626-
627607
core::smart_refctd_ptr<system::IFile> f;
628608
{
629609
system::ISystem::future_t<core::smart_refctd_ptr<system::IFile>> future;
630610
m_system->createFile(future, path.c_str(), system::IFile::ECF_READ);
631611
if (!future.wait())
632-
{
633-
if (!cacheKey.empty())
634-
{
635-
std::lock_guard<std::mutex> lock(m_cacheMutex);
636-
m_missingCache.insert(cacheKey);
637-
}
638612
return {};
639-
}
640613
future.acquire().move_into(f);
641614
}
642615
if (!f)
643-
{
644-
if (!cacheKey.empty())
645-
{
646-
std::lock_guard<std::mutex> lock(m_cacheMutex);
647-
m_missingCache.insert(cacheKey);
648-
}
649616
return {};
650-
}
651617
const size_t size = f->getSize();
652618

653619
std::string contents(size, '\0');
@@ -664,13 +630,6 @@ auto IShaderCompiler::CFileSystemIncludeLoader::getInclude(const system::path& s
664630
retVal.hash = static_cast<core::blake3_hash_t>(hasher);
665631
}
666632

667-
if (!cacheKey.empty())
668-
{
669-
std::lock_guard<std::mutex> lock(m_cacheMutex);
670-
m_missingCache.erase(cacheKey);
671-
m_cache.insert_or_assign(cacheKey, retVal);
672-
}
673-
674633
return retVal;
675634
}
676635

@@ -731,22 +690,9 @@ auto IShaderCompiler::CIncludeFinder::getIncludeRelative(const system::path& req
731690
{
732691
const auto lookupName = normalizeIncludeLookupName(includeName);
733692
IShaderCompiler::IIncludeLoader::found_t retVal;
734-
if (asset::detail::isGloballyResolvedIncludeName(lookupName))
735-
{
736-
if (auto contents = tryIncludeGenerators(lookupName))
737-
retVal = std::move(contents);
738-
else if (auto contents = trySearchPaths(lookupName, needHash))
739-
retVal = std::move(contents);
740-
else retVal = m_defaultFileSystemLoader->getInclude(requestingSourceDir.string(), lookupName, needHash);
741-
}
742-
else
743-
{
744-
if (auto contents = m_defaultFileSystemLoader->getInclude(requestingSourceDir.string(), lookupName, needHash))
745-
retVal = std::move(contents);
746-
else if (auto contents = tryIncludeGenerators(lookupName))
747-
retVal = std::move(contents);
748-
else retVal = std::move(trySearchPaths(lookupName, needHash));
749-
}
693+
if (auto contents = m_defaultFileSystemLoader->getInclude(requestingSourceDir.string(), lookupName, needHash))
694+
retVal = std::move(contents);
695+
else retVal = std::move(trySearchPaths(lookupName, needHash));
750696

751697
if (needHash && retVal && retVal.hash == core::blake3_hash_t{})
752698
{

src/nbl/asset/utils/includeResolutionCommon.h

Lines changed: 0 additions & 36 deletions
This file was deleted.

src/nbl/asset/utils/waveContext.h

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
#include <unordered_set>
1818

1919
#include "nbl/asset/utils/IShaderCompiler.h"
20-
#include "includeResolutionCommon.h"
2120

2221
namespace nbl::wave
2322
{
@@ -27,6 +26,30 @@ using namespace boost::wave::util;
2726

2827
namespace detail
2928
{
29+
inline bool is_globally_resolved_include_name(std::string_view includeName)
30+
{
31+
constexpr std::string_view globalPrefixes[] = {
32+
"nbl/",
33+
"nbl\\",
34+
"boost/",
35+
"boost\\",
36+
"glm/",
37+
"glm\\",
38+
"spirv/",
39+
"spirv\\",
40+
"Imath/",
41+
"Imath\\"
42+
};
43+
44+
for (const auto prefix : globalPrefixes)
45+
{
46+
if (includeName.rfind(prefix, 0ull) == 0ull)
47+
return true;
48+
}
49+
50+
return false;
51+
}
52+
3053
struct PerfStats
3154
{
3255
bool enabled = false;
@@ -740,7 +763,7 @@ class context : private boost::noncopyable
740763
std::string make_include_resolution_key(std::string_view includeName, bool is_system) const
741764
{
742765
std::string key;
743-
const bool globallyResolved = is_system || asset::detail::isGloballyResolvedIncludeName(includeName);
766+
const bool globallyResolved = is_system || detail::is_globally_resolved_include_name(includeName);
744767
if (!globallyResolved)
745768
{
746769
const auto currentDirString = current_dir.generic_string();

0 commit comments

Comments
 (0)