From d86260ba2d459b39befbd60d6bc4d048a6648737 Mon Sep 17 00:00:00 2001 From: emeric Date: Fri, 20 Oct 2023 15:22:00 +0200 Subject: [PATCH 1/7] Replaced Json parser with a custom one (optims+compact output) --- src/libs/subsonic/impl/SubsonicResource.cpp | 3 +- src/libs/subsonic/impl/SubsonicResponse.cpp | 270 +++++++++++------- src/libs/subsonic/impl/SubsonicResponse.hpp | 62 ++-- .../impl/entrypoints/AlbumSongLists.cpp | 4 +- .../subsonic/impl/entrypoints/Browsing.cpp | 4 +- .../impl/entrypoints/MediaLibraryScanning.cpp | 3 +- src/libs/subsonic/impl/responses/Album.cpp | 6 +- src/libs/subsonic/impl/responses/Song.cpp | 4 +- src/libs/utils/impl/StreamLogger.cpp | 13 +- src/libs/utils/impl/String.cpp | 24 +- src/libs/utils/impl/WtLogger.cpp | 17 +- src/libs/utils/include/utils/Logger.hpp | 84 +++--- src/libs/utils/include/utils/String.hpp | 3 +- 13 files changed, 316 insertions(+), 181 deletions(-) diff --git a/src/libs/subsonic/impl/SubsonicResource.cpp b/src/libs/subsonic/impl/SubsonicResource.cpp index fe432a81..c38bc0d0 100644 --- a/src/libs/subsonic/impl/SubsonicResource.cpp +++ b/src/libs/subsonic/impl/SubsonicResource.cpp @@ -290,10 +290,11 @@ namespace API::Subsonic Response resp{ (itEntryPoint->second.func)(requestContext) }; + LMS_LOG(API_SUBSONIC, DEBUG) << "Request " << requestId << " '" << requestPath << "' handled!"; resp.write(response.out(), format); response.setMimeType(std::string{ ResponseFormatToMimeType(format) }); + LMS_LOG(API_SUBSONIC, DEBUG) << "Request " << requestId << " '" << requestPath << "' written!"; - LMS_LOG(API_SUBSONIC, DEBUG) << "Request " << requestId << " '" << requestPath << "' handled!"; return; } diff --git a/src/libs/subsonic/impl/SubsonicResponse.cpp b/src/libs/subsonic/impl/SubsonicResponse.cpp index 3f177a6d..ec6e6505 100644 --- a/src/libs/subsonic/impl/SubsonicResponse.cpp +++ b/src/libs/subsonic/impl/SubsonicResponse.cpp @@ -20,12 +20,8 @@ #include "SubsonicResponse.hpp" #include -#include -#include -#include -#include - -#include +#include +#include #include #include "utils/Exception.hpp" @@ -57,59 +53,66 @@ namespace API::Subsonic _value = value; } - void Response::Node::setAttribute(std::string_view key, std::string_view value) + void Response::Node::setAttribute(Key key, std::string_view value) { - _attributes[std::string{ key }] = std::string{ value }; + _attributes[key] = std::string{ value }; } - void Response::Node::addChild(const std::string& key, Node node) + void Response::Node::addChild(Key key, Node node) { assert(!_value); - _children[key].emplace_back(std::move(node)); + assert(_children.find(key) == std::cend(_children)); + _children[key] = std::move(node); } - void Response::Node::createEmptyArrayChild(std::string_view key) + void Response::Node::createEmptyArrayChild(Key key) { assert(!_value); + assert(_children.find(key) == std::cend(_children)); _childrenArrays.emplace(key, std::vector{}); } - void Response::Node::addArrayChild(std::string_view key, Node node) + void Response::Node::addArrayChild(Key key, Node node) { assert(!_value); - _childrenArrays[std::string{ key }].emplace_back(std::move(node)); + assert(_children.find(key) == std::cend(_children)); + _childrenArrays[key].emplace_back(std::move(node)); } - void Response::Node::createEmptyArrayValue(std::string_view key) + void Response::Node::createEmptyArrayValue(Key key) { assert(!_value); + assert(_children.find(key) == std::cend(_children)); _childrenValues.emplace(key, ValuesType{}); } - void Response::Node::addArrayValue(std::string_view key, std::string_view value) + void Response::Node::addArrayValue(Key key, std::string_view value) { assert(!_value); - auto& values{ _childrenValues[std::string{ key }] }; + assert(_children.find(key) == std::cend(_children)); + auto& values{ _childrenValues[key] }; values.push_back(std::string{ value }); assert(std::all_of(std::cbegin(values) + 1, std::cend(values), [&](const ValueType& value) {return value.index() == values.front().index();})); } - void Response::Node::addArrayValue(std::string_view key, long long value) + void Response::Node::addArrayValue(Key key, long long value) { assert(!_value); - auto& values{ _childrenValues[std::string{ key }] }; + auto& values{ _childrenValues[key] }; values.push_back(value); assert(std::all_of(std::cbegin(values) + 1, std::cend(values), [&](const ValueType& value) {return value.index() == values.front().index();})); } - Response::Node& Response::Node::createChild(const std::string& key) + Response::Node& Response::Node::createChild(Key key) { - _children[key].emplace_back(); - return _children[key].back(); + assert(!_value); + return _children[key]; } - Response::Node& Response::Node::createArrayChild(const std::string& key) + Response::Node& Response::Node::createArrayChild(Key key) { + assert(!_value); + assert(_children.find(key) == std::cend(_children)); _childrenArrays[key].emplace_back(); return _childrenArrays[key].back(); } @@ -152,19 +155,19 @@ namespace API::Subsonic return response; } - void Response::addNode(const std::string& key, Node node) + void Response::addNode(Node::Key key, Node node) { - return _root._children["subsonic-response"].front().addChild(key, std::move(node)); + return _root._children["subsonic-response"].addChild(key, std::move(node)); } - Response::Node& Response::createNode(const std::string& key) + Response::Node& Response::createNode(Node::Key key) { - return _root._children["subsonic-response"].front().createChild(key); + return _root._children["subsonic-response"].createChild(key); } - Response::Node& Response::createArrayNode(const std::string& key) + Response::Node& Response::createArrayNode(Node::Key key) { - return _root._children["subsonic-response"].front().createArrayChild(key); + return _root._children["subsonic-response"].createArrayChild(key); } void Response::write(std::ostream& os, ResponseFormat format) @@ -186,16 +189,16 @@ namespace API::Subsonic { boost::property_tree::ptree res; - for (auto itAttribute : node._attributes) + for (const auto& [key, value] : node._attributes) { - if (std::holds_alternative(itAttribute.second)) - res.put("." + itAttribute.first, std::get(itAttribute.second)); - else if (std::holds_alternative(itAttribute.second)) - res.put("." + itAttribute.first, std::get(itAttribute.second)); - else if (std::holds_alternative(itAttribute.second)) - res.put("." + itAttribute.first, std::get(itAttribute.second)); - else if (std::holds_alternative(itAttribute.second)) - res.put("." + itAttribute.first, std::get(itAttribute.second)); + if (std::holds_alternative(value)) + res.put("." + std::string{ key.get() }, std::get(value)); + else if (std::holds_alternative(value)) + res.put("." + std::string{ key.get() }, std::get(value)); + else if (std::holds_alternative(value)) + res.put("." + std::string{ key.get() }, std::get(value)); + else if (std::holds_alternative(value)) + res.put("." + std::string{ key.get() }, std::get(value)); } auto valueToPropertyTree = [](const Node::ValueType& value) @@ -215,22 +218,21 @@ namespace API::Subsonic } else { - for (const auto& [key, childNodes] : node._children) + for (const auto& [key, childNode] : node._children) { - for (const Node& childNode : childNodes) - res.add_child(key, nodeToPropertyTree(childNode)); + res.add_child(std::string{ key.get() }, nodeToPropertyTree(childNode)); } for (const auto& [key, childArrayNodes] : node._childrenArrays) { for (const Node& childNode : childArrayNodes) - res.add_child(key, nodeToPropertyTree(childNode)); + res.add_child(std::string{ key.get() }, nodeToPropertyTree(childNode)); } for (const auto& [key, childArrayValues] : node._childrenValues) { for (const Response::Node::ValueType& value : childArrayValues) - res.add_child(key, valueToPropertyTree(value)); + res.add_child(std::string{ key.get() }, valueToPropertyTree(value)); } } @@ -241,63 +243,137 @@ namespace API::Subsonic boost::property_tree::write_xml(os, root); } + void Response::JsonSerializer::serializeNode(std::ostream& os, const Response::Node& node) + { + os << '{'; + + bool first{ true }; + + for (const auto& [key, value] : node._attributes) + { + if (!first) + os << ','; + + serializeEscapedString(os, key.get()); + os << ':'; + serializeValue(os, value); + + first = false; + } + + if (node._value) + { + if (!first) + os << ','; + + os << "value:"; + serializeValue(os, *node._value); + + first = false; + } + else + { + for (const auto& [key, childNode] : node._children) + { + if (!first) + os << ','; + + serializeEscapedString(os, key.get()); + os << ':'; + serializeNode(os, childNode); + + first = false; + } + + + for (const auto& [key, childArrayNodes] : node._childrenArrays) + { + if (!first) + os << ','; + + serializeEscapedString(os, key.get()); + os << ":["; + + bool firstChild{ true }; + for (const Response::Node& childNode : childArrayNodes) + { + if (!firstChild) + os << ","; + + serializeNode(os, childNode); + firstChild = false; + } + os << ']'; + + first = false; + } + + for (const auto& [key, childValues] : node._childrenValues) + { + if (!first) + os << ','; + + serializeEscapedString(os, key.get()); + os << ":["; + + bool firstChild{ true }; + for (const Node::ValueType& childValue : childValues) + { + if (!firstChild) + os << ","; + + serializeValue(os, childValue); + + firstChild = false; + } + os << ']'; + + first = false; + } + } + + os << '}'; + } + + void Response::JsonSerializer::serializeValue(std::ostream& os, const Node::ValueType& value) + { + if (std::holds_alternative(value)) + { + serializeEscapedString(os, std::get(value)); + } + else if (std::holds_alternative(value)) + { + os << (std::get(value) ? "true" : "false"); + } + else if (std::holds_alternative(value)) + { + const float d{ std::get(value) }; + if (std::isnan(d) || std::fabs(d) == std::numeric_limits::infinity()) + os << "null"; + else + os << d; + } + else if (std::holds_alternative(value)) + { + os << std::get(value); + } + else + { + assert(false); + } + } + + void Response::JsonSerializer::serializeEscapedString(std::ostream& os, std::string_view str) + { + os << '\"'; + StringUtils::writeJSEscapedString(os, str); + os << '\"'; + } + void Response::writeJSON(std::ostream& os) { - namespace Json = Wt::Json; - - std::function nodeToJsonObject = [&](const Response::Node& node) - { - Json::Object res; - - auto valueToJsonValue{ [](const Node::ValueType& value) -> Json::Value - { - Json::Value res; - std::visit([&](const auto& rawValue) - { - res = Json::Value{ rawValue }; - }, value); - return res; - } }; - - for (auto itAttribute : node._attributes) - res[itAttribute.first] = valueToJsonValue(itAttribute.second); - - if (node._value) - { - res["value"] = valueToJsonValue(*node._value); - } - else - { - for (const auto& [key, childNodes] : node._children) - { - for (const Response::Node& childNode : childNodes) - res[key] = nodeToJsonObject(childNode); - } - - for (const auto& [key, childArrayNodes] : node._childrenArrays) - { - Json::Array array; - for (const Response::Node& childNode : childArrayNodes) - array.emplace_back(nodeToJsonObject(childNode)); - - res[key] = std::move(array); - } - - for (const auto& [key, childValues] : node._childrenValues) - { - Json::Array array; - for (const Node::ValueType& childValue : childValues) - array.emplace_back(valueToJsonValue(childValue)); - - res[key] = std::move(array); - } - } - - return res; - }; - - Json::Object root{ nodeToJsonObject(_root) }; - os << Json::serialize(root); + JsonSerializer serializer; + serializer.serializeNode(os, _root); } } // namespace diff --git a/src/libs/subsonic/impl/SubsonicResponse.hpp b/src/libs/subsonic/impl/SubsonicResponse.hpp index 64046ac0..119630ce 100644 --- a/src/libs/subsonic/impl/SubsonicResponse.hpp +++ b/src/libs/subsonic/impl/SubsonicResponse.hpp @@ -191,17 +191,30 @@ namespace API::Subsonic class Node { public: - void setAttribute(std::string_view key, std::string_view value); + class Key + { + public: + template + constexpr Key(const char (&str)[N]) : _str{ str } {} + constexpr std::string_view get() const { return _str; } + + bool constexpr operator<(const Key& other) const { return _str < other._str; } + + private: + const std::string_view _str; + }; + + void setAttribute(Key key, std::string_view value); template ::value>* = nullptr> - void setAttribute(std::string_view key, T value) + void setAttribute(Key key, T value) { if constexpr (std::is_same::value) - _attributes[std::string{ key }] = value; + _attributes[key] = value; else if constexpr (std::is_floating_point::value) - _attributes[std::string{ key }] = static_cast(value); + _attributes[key] = static_cast(value); else if constexpr (std::is_integral::value) - _attributes[std::string{ key }] = static_cast(value); + _attributes[key] = static_cast(value); else static_assert("Unhandled type"); } @@ -209,28 +222,28 @@ namespace API::Subsonic // A Node has either a single value or an array of values or some children void setValue(std::string_view value); void setValue(long long value); - Node& createChild(const std::string& key); - Node& createArrayChild(const std::string& key); + Node& createChild(Key key); + Node& createArrayChild(Key key); - void addChild(const std::string& key, Node node); - void createEmptyArrayChild(std::string_view key); - void addArrayChild(std::string_view key, Node node); - void createEmptyArrayValue(std::string_view key); - void addArrayValue(std::string_view key, std::string_view value); - void addArrayValue(std::string_view key, long long value); + void addChild(Key key, Node node); + void createEmptyArrayChild(Key key); + void addArrayChild(Key key, Node node); + void createEmptyArrayValue(Key key); + void addArrayValue(Key key, std::string_view value); + void addArrayValue(Key key, long long value); private: void setVersionAttribute(ProtocolVersion version); friend class Response; using ValueType = std::variant; - std::map _attributes; + std::map _attributes; std::optional _value; - std::map> _children; - std::map> _childrenArrays; + std::map _children; + std::map> _childrenArrays; using ValuesType = std::vector; - std::map _childrenValues; + std::map _childrenValues; }; static Response createOkResponse(ProtocolVersion protocolVersion); @@ -242,14 +255,23 @@ namespace API::Subsonic Response(Response&&) = default; Response& operator=(Response&&) = default; - void addNode(const std::string& key, Node node); - Node& createNode(const std::string& key); - Node& createArrayNode(const std::string& key); + void addNode(Node::Key key, Node node); + Node& createNode(Node::Key key); + Node& createArrayNode(Node::Key key); void write(std::ostream& os, ResponseFormat format); private: static Response createResponseCommon(ProtocolVersion protocolVersion, const Error* error = nullptr); + + class JsonSerializer + { + public: + void serializeNode(std::ostream& os, const Node& node); + void serializeValue(std::ostream& os, const Node::ValueType& node); + void serializeEscapedString(std::ostream&, std::string_view str); + }; + void writeJSON(std::ostream& os); void writeXML(std::ostream& os); diff --git a/src/libs/subsonic/impl/entrypoints/AlbumSongLists.cpp b/src/libs/subsonic/impl/entrypoints/AlbumSongLists.cpp index e0b096db..2cb08ca0 100644 --- a/src/libs/subsonic/impl/entrypoints/AlbumSongLists.cpp +++ b/src/libs/subsonic/impl/entrypoints/AlbumSongLists.cpp @@ -133,7 +133,7 @@ namespace API::Subsonic throw NotImplementedGenericError{}; Response response{ Response::createOkResponse(context.serverProtocolVersion) }; - Response::Node& albumListNode{ response.createNode(id3 ? "albumList2" : "albumList") }; + Response::Node& albumListNode{ response.createNode(id3 ? Response::Node::Key{ "albumList2" } : Response::Node::Key{ "albumList" }) }; for (const ReleaseId releaseId : releases.results) { @@ -153,7 +153,7 @@ namespace API::Subsonic throw UserNotAuthorizedError{}; Response response{ Response::createOkResponse(context.serverProtocolVersion) }; - Response::Node& starredNode{ response.createNode(id3 ? "starred2" : "starred") }; + Response::Node& starredNode{ response.createNode(id3 ? Response::Node::Key{ "starred2" } : Response::Node::Key{ "starred" }) }; Scrobbling::IScrobblingService& scrobbling{ *Service::get() }; diff --git a/src/libs/subsonic/impl/entrypoints/Browsing.cpp b/src/libs/subsonic/impl/entrypoints/Browsing.cpp index 44202630..ad525f4e 100644 --- a/src/libs/subsonic/impl/entrypoints/Browsing.cpp +++ b/src/libs/subsonic/impl/entrypoints/Browsing.cpp @@ -54,7 +54,7 @@ namespace API::Subsonic std::size_t count{ getParameterAs(context.parameters, "count").value_or(20) }; Response response{ Response::createOkResponse(context.serverProtocolVersion) }; - Response::Node& artistInfoNode{ response.createNode(id3 ? "artistInfo2" : "artistInfo") }; + Response::Node& artistInfoNode{ response.createNode(id3 ? Response::Node::Key{ "artistInfo2" } : Response::Node::Key{ "artistInfo" }) }; { auto transaction{ context.dbSession.createSharedTransaction() }; @@ -236,7 +236,7 @@ namespace API::Subsonic throw UserNotAuthorizedError{}; Response response{ Response::createOkResponse(context.serverProtocolVersion) }; - Response::Node& similarSongsNode{ response.createNode(id3 ? "similarSongs2" : "similarSongs") }; + Response::Node& similarSongsNode{ response.createNode(id3 ? Response::Node::Key{ "similarSongs2" } : Response::Node::Key{ "similarSongs" }) }; for (const TrackId trackId : tracks) { const Track::pointer track{ Track::find(context.dbSession, trackId) }; diff --git a/src/libs/subsonic/impl/entrypoints/MediaLibraryScanning.cpp b/src/libs/subsonic/impl/entrypoints/MediaLibraryScanning.cpp index b328553e..78f2ea25 100644 --- a/src/libs/subsonic/impl/entrypoints/MediaLibraryScanning.cpp +++ b/src/libs/subsonic/impl/entrypoints/MediaLibraryScanning.cpp @@ -28,8 +28,7 @@ namespace API::Subsonic::Scan namespace { - Response::Node - createStatusResponseNode() + Response::Node createStatusResponseNode() { Response::Node statusResponse; diff --git a/src/libs/subsonic/impl/responses/Album.cpp b/src/libs/subsonic/impl/responses/Album.cpp index effd21f4..0ec6dfb0 100644 --- a/src/libs/subsonic/impl/responses/Album.cpp +++ b/src/libs/subsonic/impl/responses/Album.cpp @@ -114,7 +114,7 @@ namespace API::Subsonic if (artists.size() == 1) { - albumNode.setAttribute(id3 ? "artistId" : "parent", idToString(artists.front()->getId())); + albumNode.setAttribute(id3 ? Response::Node::Key{ "artistId" } : Response::Node::Key{ "parent" }, idToString(artists.front()->getId())); } else { @@ -140,7 +140,7 @@ namespace API::Subsonic { const Wt::WDateTime dateTime{ Service::get()->getLastListenDateTime(user->getId(), release->getId()) }; - albumNode.setAttribute("played", dateTime.isValid() ? StringUtils::toISO8601String(dateTime) : ""); + albumNode.setAttribute("played", dateTime.isValid() ? StringUtils::toISO8601String(dateTime) : std::string{ "" }); } { @@ -148,7 +148,7 @@ namespace API::Subsonic albumNode.setAttribute("musicBrainzId", mbid ? mbid->getAsString() : ""); } - auto addClusters{ [&](std::string_view field, std::string_view clusterTypeName) + auto addClusters{ [&](Response::Node::Key field, std::string_view clusterTypeName) { albumNode.createEmptyArrayValue(field); diff --git a/src/libs/subsonic/impl/responses/Song.cpp b/src/libs/subsonic/impl/responses/Song.cpp index d3ef6a79..c2a0fbc7 100644 --- a/src/libs/subsonic/impl/responses/Song.cpp +++ b/src/libs/subsonic/impl/responses/Song.cpp @@ -186,7 +186,7 @@ namespace API::Subsonic } } - auto addArtistLinks{ [&](std::string_view nodeName, TrackArtistLinkType type) + auto addArtistLinks{ [&](Response::Node::Key nodeName, TrackArtistLinkType type) { trackResponse.createEmptyArrayChild(nodeName); @@ -209,7 +209,7 @@ namespace API::Subsonic if (release) trackResponse.setAttribute("displayAlbumArtist", release->getArtistDisplayName()); - auto addClusters{ [&](std::string_view field, std::string_view clusterTypeName) + auto addClusters{ [&](Response::Node::Key field, std::string_view clusterTypeName) { trackResponse.createEmptyArrayValue(field); diff --git a/src/libs/utils/impl/StreamLogger.cpp b/src/libs/utils/impl/StreamLogger.cpp index 4b1cebd1..cc8efbe6 100644 --- a/src/libs/utils/impl/StreamLogger.cpp +++ b/src/libs/utils/impl/StreamLogger.cpp @@ -17,18 +17,19 @@ * along with LMS. If not, see . */ +#include + #include "utils/StreamLogger.hpp" StreamLogger::StreamLogger(std::ostream& os, EnumSet severities) -: _os {os} -, _severities {severities} + : _os{ os } + , _severities{ severities } { } -void -StreamLogger::processLog(const Log& log) +void StreamLogger::processLog(const Log& log) { - if (_severities.contains(log.getSeverity())) - _os << "[" << getSeverityName(log.getSeverity()) << "] [" << getModuleName(log.getModule()) << "] " << log.getMessage() << std::endl; + if (_severities.contains(log.getSeverity())) + _os << std::this_thread::get_id() << " [" << getSeverityName(log.getSeverity()) << "] [" << getModuleName(log.getModule()) << "] " << log.getMessage() << std::endl; } diff --git a/src/libs/utils/impl/String.cpp b/src/libs/utils/impl/String.cpp index cae5c406..807ec72b 100644 --- a/src/libs/utils/impl/String.cpp +++ b/src/libs/utils/impl/String.cpp @@ -219,7 +219,7 @@ namespace StringUtils return res; } - std::string jsEscape(const std::string& str) + std::string jsEscape(std::string_view str) { static const std::unordered_map escapeMap { @@ -249,6 +249,28 @@ namespace StringUtils return escaped; } + void writeJSEscapedString(std::ostream& os, std::string_view str) + { + static constexpr std::pair charsToEscape[] + { + {'\\', "\\\\" }, + { '\n', "\\n" }, + { '\r', "\\r" }, + { '\t', "\\t" }, + { '"', "\\\"" }, + { '\'', "\\\'" }, + }; + + for (const char c : str) + { + auto itEntry{ std::find_if(std::cbegin(charsToEscape), std::cend(charsToEscape), [=](const auto& entry) { return entry.first == c;}) }; + if (itEntry != std::cend(charsToEscape)) + os << itEntry->second; + else + os << c; + } + } + std::string escapeString(std::string_view str, std::string_view charsToEscape, char escapeChar) { std::string res; diff --git a/src/libs/utils/impl/WtLogger.cpp b/src/libs/utils/impl/WtLogger.cpp index 126af923..ded7522e 100644 --- a/src/libs/utils/impl/WtLogger.cpp +++ b/src/libs/utils/impl/WtLogger.cpp @@ -19,14 +19,25 @@ #include "utils/WtLogger.hpp" +#include +#include #include #include #include "utils/Logger.hpp" -void -WtLogger::processLog(const Log& log) +namespace { - Wt::log(getSeverityName(log.getSeverity())) << Wt::WLogger::sep << "[" << getModuleName(log.getModule()) << "]" << Wt::WLogger::sep << log.getMessage(); + std::string to_string(std::thread::id id) + { + std::ostringstream oss; + oss << id; + return oss.str(); + } +} + +void WtLogger::processLog(const Log& log) +{ + Wt::log(getSeverityName(log.getSeverity())) << Wt::WLogger::sep << to_string(std::this_thread::get_id()) << Wt::WLogger::sep << "[" << getModuleName(log.getModule()) << "]" << Wt::WLogger::sep << log.getMessage(); } diff --git a/src/libs/utils/include/utils/Logger.hpp b/src/libs/utils/include/utils/Logger.hpp index d7faf078..ba36f4ea 100644 --- a/src/libs/utils/include/utils/Logger.hpp +++ b/src/libs/utils/include/utils/Logger.hpp @@ -26,33 +26,33 @@ enum class Severity { - FATAL, - ERROR, - WARNING, - INFO, - DEBUG, + FATAL, + ERROR, + WARNING, + INFO, + DEBUG, }; enum class Module { - API_SUBSONIC, - AUTH, - AV, - CHILDPROCESS, - COVER, - DB, - DBUPDATER, - FEATURE, - HTTP, - MAIN, - METADATA, - REMOTE, - SCROBBLING, - SERVICE, - RECOMMENDATION, - TRANSCODE, - UI, - UTILS, + API_SUBSONIC, + AUTH, + AV, + CHILDPROCESS, + COVER, + DB, + DBUPDATER, + FEATURE, + HTTP, + MAIN, + METADATA, + REMOTE, + SCROBBLING, + SERVICE, + RECOMMENDATION, + TRANSCODE, + UI, + UTILS, }; const char* getModuleName(Module mod); @@ -61,30 +61,32 @@ const char* getSeverityName(Severity sev); class Logger; class Log { - public: - Log(Logger* logger, Module module, Severity severity); - ~Log(); +public: + Log(Logger* logger, Module module, Severity severity); + ~Log(); - Module getModule() const { return _module; } - Severity getSeverity() const { return _severity; } - std::string getMessage() const; + Module getModule() const { return _module; } + Severity getSeverity() const { return _severity; } + std::string getMessage() const; - std::ostringstream& getOstream() { return _oss; } + std::ostringstream& getOstream() { return _oss; } - private: - Module _module; - Severity _severity; - std::ostringstream _oss; - Logger* _logger {}; +private: + Log(const Log&) = delete; + Log& operator=(const Log&) = delete; + + Module _module; + Severity _severity; + std::ostringstream _oss; + Logger* _logger{}; }; class Logger { - public: - virtual ~Logger() = default; - virtual void processLog(const Log& log) = 0; +public: + virtual ~Logger() = default; + virtual void processLog(const Log& log) = 0; }; -#define LMS_LOG(module, severity) Log(Service::get(), Module::module, Severity::severity).getOstream() -#define LMS_LOG_EX(module, severity) Log(Service::get(), module, severity).getOstream() - +#define LMS_LOG(module, severity) Log{Service::get(), Module::module, Severity::severity}.getOstream() +#define LMS_LOG_EX(module, severity) Log{Service::get(), module, severity}.getOstream() diff --git a/src/libs/utils/include/utils/String.hpp b/src/libs/utils/include/utils/String.hpp index 70addee0..03709ac1 100644 --- a/src/libs/utils/include/utils/String.hpp +++ b/src/libs/utils/include/utils/String.hpp @@ -83,7 +83,8 @@ namespace StringUtils { [[nodiscard]] std::string replaceInString(std::string_view str, const std::string& from, const std::string& to); - [[nodiscard]] std::string jsEscape(const std::string& str); + [[nodiscard]] std::string jsEscape(std::string_view str); + void writeJSEscapedString(std::ostream& os, std::string_view str); [[nodiscard]] std::string escapeString(std::string_view str, std::string_view charsToEscape, char escapeChar); From 07f58b3d100d51282981ee8cd385db4c390ab00a Mon Sep 17 00:00:00 2001 From: emeric Date: Sat, 21 Oct 2023 00:00:36 +0200 Subject: [PATCH 2/7] Avoid a useless request --- src/libs/subsonic/impl/responses/Album.cpp | 11 +++++------ src/libs/subsonic/impl/responses/Song.cpp | 8 ++++---- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/libs/subsonic/impl/responses/Album.cpp b/src/libs/subsonic/impl/responses/Album.cpp index 0ec6dfb0..0394e46f 100644 --- a/src/libs/subsonic/impl/responses/Album.cpp +++ b/src/libs/subsonic/impl/responses/Album.cpp @@ -124,9 +124,10 @@ namespace API::Subsonic } // Report the first GENRE for this track - if (ClusterType::pointer clusterType{ ClusterType::find(dbSession, "GENRE") }) + const ClusterType::pointer genreClusterType{ ClusterType::find(dbSession, "GENRE") }; + if (genreClusterType) { - auto clusters{ release->getClusterGroups({clusterType}, 1) }; + auto clusters{ release->getClusterGroups({genreClusterType}, 1) }; if (!clusters.empty() && !clusters.front().empty()) albumNode.setAttribute("genre", clusters.front().front()->getName()); } @@ -173,13 +174,11 @@ namespace API::Subsonic // Genres { albumNode.createEmptyArrayChild("genres"); - - ClusterType::pointer clusterType{ ClusterType::find(dbSession, "GENRE") }; - if (clusterType) + if (genreClusterType) { Cluster::FindParameters params; params.setRelease(release->getId()); - params.setClusterType(clusterType->getId()); + params.setClusterType(genreClusterType->getId()); for (const ClusterId clusterId : Cluster::find(dbSession, params).results) { diff --git a/src/libs/subsonic/impl/responses/Song.cpp b/src/libs/subsonic/impl/responses/Song.cpp index c2a0fbc7..8870d8a8 100644 --- a/src/libs/subsonic/impl/responses/Song.cpp +++ b/src/libs/subsonic/impl/responses/Song.cpp @@ -152,7 +152,8 @@ namespace API::Subsonic trackResponse.setAttribute("starred", StringUtils::toISO8601String(dateTime)); // Report the first GENRE for this track - if (ClusterType::pointer genreClusterType{ ClusterType::find(dbSession, "GENRE") }) + const ClusterType::pointer genreClusterType{ ClusterType::find(dbSession, "GENRE") }; + if (genreClusterType) { auto clusters{ track->getClusterGroups({genreClusterType}, 1) }; if (!clusters.empty() && !clusters.front().empty()) @@ -235,12 +236,11 @@ namespace API::Subsonic { trackResponse.createEmptyArrayChild("genres"); - ClusterType::pointer clusterType{ ClusterType::find(dbSession, "GENRE") }; - if (clusterType) + if (genreClusterType) { Cluster::FindParameters params; params.setTrack(track->getId()); - params.setClusterType(clusterType->getId()); + params.setClusterType(genreClusterType->getId()); for (const ClusterId clusterId : Cluster::find(dbSession, params).results) { From 61e0931a827eabd0834e69ce08ec91ab58065964 Mon Sep 17 00:00:00 2001 From: emeric Date: Sat, 21 Oct 2023 14:45:39 +0200 Subject: [PATCH 3/7] Fixed output bug for values, added some optims --- src/libs/services/database/impl/Cluster.cpp | 6 ++-- src/libs/services/database/impl/Migration.cpp | 6 ++-- .../include/services/database/Cluster.hpp | 11 ++++--- src/libs/services/database/test/Cluster.cpp | 24 ++++++++------- src/libs/subsonic/impl/SubsonicResponse.cpp | 8 ++--- src/libs/subsonic/impl/SubsonicResponse.hpp | 6 ++-- .../subsonic/impl/entrypoints/Playlists.cpp | 2 +- src/libs/subsonic/impl/responses/Album.cpp | 30 +++++++------------ .../subsonic/impl/responses/ItemGenre.cpp | 4 +-- .../subsonic/impl/responses/ItemGenre.hpp | 4 +-- src/libs/subsonic/impl/responses/Song.cpp | 29 ++++++------------ 11 files changed, 58 insertions(+), 72 deletions(-) diff --git a/src/libs/services/database/impl/Cluster.cpp b/src/libs/services/database/impl/Cluster.cpp index 6acc874a..1dcbf1b7 100644 --- a/src/libs/services/database/impl/Cluster.cpp +++ b/src/libs/services/database/impl/Cluster.cpp @@ -32,11 +32,11 @@ namespace Database { namespace { - Wt::Dbo::Query createQuery(Session& session, const Cluster::FindParameters& params) + Wt::Dbo::Query createQuery(Session& session, const Cluster::FindParameters& params) { session.checkSharedLocked(); - auto query{ session.getDboSession().query("SELECT DISTINCT c.id FROM cluster c") }; + auto query{ session.getDboSession().query("SELECT DISTINCT c.id,c.name FROM cluster c") }; if (params.track.isValid() || params.release.isValid()) { @@ -74,7 +74,7 @@ namespace Database return session.getDboSession().query("SELECT COUNT(*) FROM cluster"); } - RangeResults Cluster::find(Session& session, const FindParameters& params) + RangeResults Cluster::find(Session& session, const FindParameters& params) { session.checkSharedLocked(); auto query{ createQuery(session, params) }; diff --git a/src/libs/services/database/impl/Migration.cpp b/src/libs/services/database/impl/Migration.cpp index 728caf4e..5785cce6 100644 --- a/src/libs/services/database/impl/Migration.cpp +++ b/src/libs/services/database/impl/Migration.cpp @@ -672,7 +672,7 @@ CREATE TABLE IF NOT EXISTS "track_backup" ( if (version == LMS_DATABASE_VERSION) { - LMS_LOG(DB, DEBUG) << "Lms database version " << LMS_DATABASE_VERSION << ": up to date!"; + LMS_LOG(DB, INFO) << "Lms database version " << LMS_DATABASE_VERSION << ": up to date!"; return; } else if (version > LMS_DATABASE_VERSION) @@ -683,13 +683,15 @@ CREATE TABLE IF NOT EXISTS "track_backup" ( if (version < migrationFunctions.begin()->first) throw LmsException{ outdatedMsg }; - LMS_LOG(DB, INFO) << "Migrating database from version " << version << "..."; + LMS_LOG(DB, INFO) << "Migrating database from version " << version << " to " << version + 1 << "..."; auto itMigrationFunc{ migrationFunctions.find(version) }; assert(itMigrationFunc != std::cend(migrationFunctions)); itMigrationFunc->second(session); VersionInfo::get(session).modify()->setVersion(++version); + + LMS_LOG(DB, INFO) << "Migration complete to version " << version; } } } diff --git a/src/libs/services/database/include/services/database/Cluster.hpp b/src/libs/services/database/include/services/database/Cluster.hpp index 4f416e7f..521727e6 100644 --- a/src/libs/services/database/include/services/database/Cluster.hpp +++ b/src/libs/services/database/include/services/database/Cluster.hpp @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -58,10 +59,12 @@ namespace Database { Cluster() = default; // Find utility - static std::size_t getCount(Session& session); - static RangeResults find(Session& session, const FindParameters& range); - static pointer find(Session& session, ClusterId id); - static RangeResults findOrphans(Session& session, Range range); + // As clusters only have a name, this is an optim to directly get the cluster names + using ClusterFindResult = std::tuple; + static std::size_t getCount(Session& session); + static RangeResults find(Session& session, const FindParameters& range); + static pointer find(Session& session, ClusterId id); + static RangeResults findOrphans(Session& session, Range range); // Accessors const std::string& getName() const { return _name; } diff --git a/src/libs/services/database/test/Cluster.cpp b/src/libs/services/database/test/Cluster.cpp index d9caa140..0b045fd5 100644 --- a/src/libs/services/database/test/Cluster.cpp +++ b/src/libs/services/database/test/Cluster.cpp @@ -48,13 +48,17 @@ TEST_F(DatabaseFixture, Cluster) EXPECT_EQ(Cluster::getCount(session), 1); EXPECT_EQ(cluster->getType()->getId(), clusterType.getId()); - auto clusters{ Cluster::find(session, Cluster::FindParameters {}) }; - ASSERT_EQ(clusters.results.size(), 1); - EXPECT_EQ(clusters.results.front(), cluster.getId()); + { + const auto clusters{ Cluster::find(session, Cluster::FindParameters {}) }; + ASSERT_EQ(clusters.results.size(), 1); + EXPECT_EQ(std::get(clusters.results.front()), cluster.getId()); + } - clusters = Cluster::findOrphans(session, Range{}); - ASSERT_EQ(clusters.results.size(), 1); - EXPECT_EQ(clusters.results.front(), cluster.getId()); + { + const auto clusters{ Cluster::findOrphans(session, Range{}) }; + ASSERT_EQ(clusters.results.size(), 1); + EXPECT_EQ(clusters.results.front(), cluster.getId()); + } auto clusterTypes{ ClusterType::find(session, Range {}) }; ASSERT_EQ(clusterTypes.results.size(), 1); @@ -114,7 +118,7 @@ TEST_F(DatabaseFixture, Cluster_singleTrack) auto transaction{ session.createSharedTransaction() }; auto clusters{ Cluster::find(session, Cluster::FindParameters {}.setTrack(track.getId())) }; ASSERT_EQ(clusters.results.size(), 1); - EXPECT_EQ(clusters.results.front(), cluster1.getId()); + EXPECT_EQ(std::get(clusters.results.front()), cluster1.getId()); } { @@ -317,9 +321,9 @@ TEST_F(DatabaseFixture, Cluster_singleTrackSingleReleaseSingleCluster) { auto transaction{ session.createSharedTransaction() }; - auto clusters{ Cluster::find(session, Cluster::FindParameters{}.setRelease(release.getId())) }; + const auto clusters{ Cluster::find(session, Cluster::FindParameters{}.setRelease(release.getId())) }; ASSERT_EQ(clusters.results.size(), 1); - EXPECT_EQ(clusters.results.front(), cluster.getId()); + EXPECT_EQ(std::get(clusters.results.front()), cluster.getId()); } { @@ -1106,5 +1110,3 @@ TEST_F(DatabaseFixture, MultipleTracksMultipleReleasesMultiClusters) } } } - - diff --git a/src/libs/subsonic/impl/SubsonicResponse.cpp b/src/libs/subsonic/impl/SubsonicResponse.cpp index ec6e6505..7450792e 100644 --- a/src/libs/subsonic/impl/SubsonicResponse.cpp +++ b/src/libs/subsonic/impl/SubsonicResponse.cpp @@ -58,7 +58,7 @@ namespace API::Subsonic _attributes[key] = std::string{ value }; } - void Response::Node::addChild(Key key, Node node) + void Response::Node::addChild(Key key, Node&& node) { assert(!_value); assert(_children.find(key) == std::cend(_children)); @@ -72,7 +72,7 @@ namespace API::Subsonic _childrenArrays.emplace(key, std::vector{}); } - void Response::Node::addArrayChild(Key key, Node node) + void Response::Node::addArrayChild(Key key, Node&& node) { assert(!_value); assert(_children.find(key) == std::cend(_children)); @@ -155,7 +155,7 @@ namespace API::Subsonic return response; } - void Response::addNode(Node::Key key, Node node) + void Response::addNode(Node::Key key, Node&& node) { return _root._children["subsonic-response"].addChild(key, std::move(node)); } @@ -266,7 +266,7 @@ namespace API::Subsonic if (!first) os << ','; - os << "value:"; + os << "\"value\":"; serializeValue(os, *node._value); first = false; diff --git a/src/libs/subsonic/impl/SubsonicResponse.hpp b/src/libs/subsonic/impl/SubsonicResponse.hpp index 119630ce..9fe51a5b 100644 --- a/src/libs/subsonic/impl/SubsonicResponse.hpp +++ b/src/libs/subsonic/impl/SubsonicResponse.hpp @@ -225,9 +225,9 @@ namespace API::Subsonic Node& createChild(Key key); Node& createArrayChild(Key key); - void addChild(Key key, Node node); + void addChild(Key key, Node&& node); void createEmptyArrayChild(Key key); - void addArrayChild(Key key, Node node); + void addArrayChild(Key key, Node&& node); void createEmptyArrayValue(Key key); void addArrayValue(Key key, std::string_view value); void addArrayValue(Key key, long long value); @@ -255,7 +255,7 @@ namespace API::Subsonic Response(Response&&) = default; Response& operator=(Response&&) = default; - void addNode(Node::Key key, Node node); + void addNode(Node::Key key, Node&& node); Node& createNode(Node::Key key); Node& createArrayNode(Node::Key key); diff --git a/src/libs/subsonic/impl/entrypoints/Playlists.cpp b/src/libs/subsonic/impl/entrypoints/Playlists.cpp index 049a8dd7..63b896a3 100644 --- a/src/libs/subsonic/impl/entrypoints/Playlists.cpp +++ b/src/libs/subsonic/impl/entrypoints/Playlists.cpp @@ -75,7 +75,7 @@ namespace API::Subsonic for (const TrackListEntry::pointer& entry : entries) playlistNode.addArrayChild("entry", createSongNode(entry->getTrack(), context.dbSession, user)); - response.addNode("playlist", playlistNode); + response.addNode("playlist", std::move(playlistNode)); return response; } diff --git a/src/libs/subsonic/impl/responses/Album.cpp b/src/libs/subsonic/impl/responses/Album.cpp index 0394e46f..0abb0b88 100644 --- a/src/libs/subsonic/impl/responses/Album.cpp +++ b/src/libs/subsonic/impl/responses/Album.cpp @@ -133,7 +133,7 @@ namespace API::Subsonic } if (const Wt::WDateTime dateTime{ Service::get()->getStarredDateTime(user->getId(), release->getId()) }; dateTime.isValid()) - albumNode.setAttribute("starred", StringUtils::toISO8601String(dateTime)); // TODO report correct date/time + albumNode.setAttribute("starred", StringUtils::toISO8601String(dateTime)); // OpenSubsonic specific fields (must always be set) if (!id3) @@ -160,33 +160,23 @@ namespace API::Subsonic params.setRelease(release->getId()); params.setClusterType(clusterType->getId()); - for (const ClusterId clusterId : Cluster::find(dbSession, params).results) - { - Cluster::pointer cluster{ Cluster::find(dbSession, clusterId) }; - if (cluster) - albumNode.addArrayValue(field, cluster->getName()); - } + for (const auto& cluster : Cluster::find(dbSession, params).results) + albumNode.addArrayValue(field, std::get(cluster)); } } }; addClusters("moods", "MOOD"); // Genres + albumNode.createEmptyArrayChild("genres"); + if (genreClusterType) { - albumNode.createEmptyArrayChild("genres"); - if (genreClusterType) - { - Cluster::FindParameters params; - params.setRelease(release->getId()); - params.setClusterType(genreClusterType->getId()); + Cluster::FindParameters params; + params.setRelease(release->getId()); + params.setClusterType(genreClusterType->getId()); - for (const ClusterId clusterId : Cluster::find(dbSession, params).results) - { - Cluster::pointer cluster{ Cluster::find(dbSession, clusterId) }; - if (cluster) - albumNode.addArrayChild("genres", createItemGenreNode(cluster)); - } - } + for (const auto& cluster : Cluster::find(dbSession, params).results) + albumNode.addArrayChild("genres", createItemGenreNode(std::get(cluster))); } albumNode.createEmptyArrayChild("artists"); diff --git a/src/libs/subsonic/impl/responses/ItemGenre.cpp b/src/libs/subsonic/impl/responses/ItemGenre.cpp index a5b3387d..70ab2b9e 100644 --- a/src/libs/subsonic/impl/responses/ItemGenre.cpp +++ b/src/libs/subsonic/impl/responses/ItemGenre.cpp @@ -23,11 +23,11 @@ namespace API::Subsonic { - Response::Node createItemGenreNode(const Database::Cluster::pointer& cluster) + Response::Node createItemGenreNode(std::string_view name) { Response::Node genreNode; - genreNode.setAttribute("name", cluster->getName()); + genreNode.setAttribute("name", name); return genreNode; } diff --git a/src/libs/subsonic/impl/responses/ItemGenre.hpp b/src/libs/subsonic/impl/responses/ItemGenre.hpp index f4c1dda9..c15a4674 100644 --- a/src/libs/subsonic/impl/responses/ItemGenre.hpp +++ b/src/libs/subsonic/impl/responses/ItemGenre.hpp @@ -19,7 +19,7 @@ #pragma once -#include "services/database/Object.hpp" +#include #include "SubsonicResponse.hpp" namespace Database @@ -29,5 +29,5 @@ namespace Database namespace API::Subsonic { - Response::Node createItemGenreNode(const Database::ObjectPtr& cluster); + Response::Node createItemGenreNode(std::string_view name); } diff --git a/src/libs/subsonic/impl/responses/Song.cpp b/src/libs/subsonic/impl/responses/Song.cpp index 8870d8a8..543737f4 100644 --- a/src/libs/subsonic/impl/responses/Song.cpp +++ b/src/libs/subsonic/impl/responses/Song.cpp @@ -221,34 +221,23 @@ namespace API::Subsonic params.setTrack(track->getId()); params.setClusterType(clusterType->getId()); - for (const ClusterId clusterId : Cluster::find(dbSession, params).results) - { - Cluster::pointer cluster {Cluster::find(dbSession, clusterId)}; - if (cluster) - trackResponse.addArrayValue(field, cluster->getName()); - } + for (const auto& cluster : Cluster::find(dbSession, params).results) + trackResponse.addArrayValue(field, std::get(cluster)); } } }; addClusters("moods", "MOOD"); // Genres + trackResponse.createEmptyArrayChild("genres"); + if (genreClusterType) { - trackResponse.createEmptyArrayChild("genres"); + Cluster::FindParameters params; + params.setTrack(track->getId()); + params.setClusterType(genreClusterType->getId()); - if (genreClusterType) - { - Cluster::FindParameters params; - params.setTrack(track->getId()); - params.setClusterType(genreClusterType->getId()); - - for (const ClusterId clusterId : Cluster::find(dbSession, params).results) - { - Cluster::pointer cluster{ Cluster::find(dbSession, clusterId) }; - if (cluster) - trackResponse.addArrayChild("genres", createItemGenreNode(cluster)); - } - } + for (const auto& cluster : Cluster::find(dbSession, params).results) + trackResponse.addArrayChild("genres", createItemGenreNode(std::get(cluster))); } trackResponse.addChild("replayGain", createReplayGainNode(track)); From 7d9b5a708461a1640d948ef3d8b5044b92a1cf01 Mon Sep 17 00:00:00 2001 From: emeric Date: Sun, 22 Oct 2023 00:19:03 +0200 Subject: [PATCH 4/7] More constness --- src/libs/subsonic/impl/SubsonicResource.cpp | 5 ++--- src/libs/subsonic/impl/SubsonicResponse.cpp | 7 +++---- src/libs/subsonic/impl/SubsonicResponse.hpp | 6 +++--- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/libs/subsonic/impl/SubsonicResource.cpp b/src/libs/subsonic/impl/SubsonicResource.cpp index c38bc0d0..4b62014c 100644 --- a/src/libs/subsonic/impl/SubsonicResource.cpp +++ b/src/libs/subsonic/impl/SubsonicResource.cpp @@ -288,12 +288,11 @@ namespace API::Subsonic checkUserTypeIsAllowed(requestContext, itEntryPoint->second.allowedUserTypes); - Response resp{ (itEntryPoint->second.func)(requestContext) }; + const Response resp{ (itEntryPoint->second.func)(requestContext) }; - LMS_LOG(API_SUBSONIC, DEBUG) << "Request " << requestId << " '" << requestPath << "' handled!"; resp.write(response.out(), format); response.setMimeType(std::string{ ResponseFormatToMimeType(format) }); - LMS_LOG(API_SUBSONIC, DEBUG) << "Request " << requestId << " '" << requestPath << "' written!"; + LMS_LOG(API_SUBSONIC, DEBUG) << "Request " << requestId << " '" << requestPath << "' handled!"; return; } diff --git a/src/libs/subsonic/impl/SubsonicResponse.cpp b/src/libs/subsonic/impl/SubsonicResponse.cpp index 7450792e..ed28c49c 100644 --- a/src/libs/subsonic/impl/SubsonicResponse.cpp +++ b/src/libs/subsonic/impl/SubsonicResponse.cpp @@ -170,7 +170,7 @@ namespace API::Subsonic return _root._children["subsonic-response"].createArrayChild(key); } - void Response::write(std::ostream& os, ResponseFormat format) + void Response::write(std::ostream& os, ResponseFormat format) const { switch (format) { @@ -183,7 +183,7 @@ namespace API::Subsonic } } - void Response::writeXML(std::ostream& os) + void Response::writeXML(std::ostream& os) const { std::function nodeToPropertyTree = [&](const Node& node) { @@ -285,7 +285,6 @@ namespace API::Subsonic first = false; } - for (const auto& [key, childArrayNodes] : node._childrenArrays) { if (!first) @@ -370,7 +369,7 @@ namespace API::Subsonic os << '\"'; } - void Response::writeJSON(std::ostream& os) + void Response::writeJSON(std::ostream& os) const { JsonSerializer serializer; serializer.serializeNode(os, _root); diff --git a/src/libs/subsonic/impl/SubsonicResponse.hpp b/src/libs/subsonic/impl/SubsonicResponse.hpp index 9fe51a5b..4414d4d1 100644 --- a/src/libs/subsonic/impl/SubsonicResponse.hpp +++ b/src/libs/subsonic/impl/SubsonicResponse.hpp @@ -259,7 +259,7 @@ namespace API::Subsonic Node& createNode(Node::Key key); Node& createArrayNode(Node::Key key); - void write(std::ostream& os, ResponseFormat format); + void write(std::ostream& os, ResponseFormat format) const; private: static Response createResponseCommon(ProtocolVersion protocolVersion, const Error* error = nullptr); @@ -272,8 +272,8 @@ namespace API::Subsonic void serializeEscapedString(std::ostream&, std::string_view str); }; - void writeJSON(std::ostream& os); - void writeXML(std::ostream& os); + void writeJSON(std::ostream& os) const; + void writeXML(std::ostream& os) const; Response() = default; Node _root; From 17efc2759a7e7437cf3bb4324e08ea5605d6c363 Mon Sep 17 00:00:00 2001 From: emeric Date: Sun, 22 Oct 2023 00:26:11 +0200 Subject: [PATCH 5/7] Made database optimization faster using pragma analysis_limit --- src/libs/services/database/impl/Db.cpp | 98 ++++--- src/libs/services/database/impl/Migration.cpp | 25 +- src/libs/services/database/impl/Session.cpp | 252 +++++++++--------- .../database/include/services/database/Db.hpp | 65 +++-- .../include/services/database/Session.hpp | 93 ++++--- src/libs/services/database/test/Common.cpp | 2 +- .../services/scanner/impl/ScannerService.cpp | 3 +- src/lms/main.cpp | 5 +- 8 files changed, 263 insertions(+), 280 deletions(-) diff --git a/src/libs/services/database/impl/Db.cpp b/src/libs/services/database/impl/Db.cpp index 7eb7bf7a..117d18c5 100644 --- a/src/libs/services/database/impl/Db.cpp +++ b/src/libs/services/database/impl/Db.cpp @@ -28,70 +28,62 @@ namespace Database { -// Session living class handling the database and the login -Db::Db(const std::filesystem::path& dbPath, std::size_t connectionCount) -{ - LMS_LOG(DB, INFO) << "Creating connection pool on file " << dbPath.string(); + // Session living class handling the database and the login + Db::Db(const std::filesystem::path& dbPath, std::size_t connectionCount) + { + LMS_LOG(DB, INFO) << "Creating connection pool on file " << dbPath.string(); - std::unique_ptr connection {std::make_unique(dbPath.string())}; -// connection->setProperty("show-queries", "true"); - connection->executeSql("pragma journal_mode=WAL"); - connection->executeSql("pragma synchronous=normal"); + auto connection{ std::make_unique(dbPath.string()) }; + // connection->setProperty("show-queries", "true"); + connection->executeSql("pragma journal_mode=WAL"); + connection->executeSql("pragma synchronous=normal"); + connection->executeSql("pragma analysis_limit=1000"); // to help make analyze command faster - auto connectionPool = std::make_unique(std::move(connection), connectionCount); - connectionPool->setTimeout(std::chrono::seconds(10)); + auto connectionPool{ std::make_unique(std::move(connection), connectionCount) }; + connectionPool->setTimeout(std::chrono::seconds{ 10 }); - _connectionPool = std::move(connectionPool); -} + _connectionPool = std::move(connectionPool); + } -Db::~Db() -{ - LMS_LOG(DB, DEBUG) << "Optimizing db..."; - executeSql("pragma optimize"); - LMS_LOG(DB, DEBUG) << "Optimizing db DONE"; -} + void Db::executeSql(const std::string& sql) + { + ScopedConnection connection{ *_connectionPool }; + connection->executeSql(sql); + } -void -Db::executeSql(const std::string& sql) -{ - ScopedConnection connection {*_connectionPool}; - connection->executeSql(sql); -} + Session& Db::getTLSSession() + { + static thread_local Session* tlsSession{}; -Session& -Db::getTLSSession() -{ - static thread_local Session* tlsSession {}; + if (!tlsSession) + { + auto newSession{ std::make_unique(*this) }; + tlsSession = newSession.get(); - if (!tlsSession) - { - auto newSession {std::make_unique(*this)}; - tlsSession = newSession.get(); + { + std::scoped_lock lock{ _tlsSessionsMutex }; + _tlsSessions.push_back(std::move(newSession)); + } + } - { - std::scoped_lock lock {_tlsSessionsMutex}; - _tlsSessions.push_back(std::move(newSession)); - } - } + return *tlsSession; + } - return *tlsSession; -} + Db::ScopedConnection::ScopedConnection(Wt::Dbo::SqlConnectionPool& pool) + : _connectionPool{ pool } + , _connection{ _connectionPool.getConnection() } + { + } -Db::ScopedConnection::ScopedConnection(Wt::Dbo::SqlConnectionPool& pool) -: _connectionPool {pool} -, _connection {_connectionPool.getConnection()} -{ -} + Db::ScopedConnection::~ScopedConnection() + { + _connectionPool.returnConnection(std::move(_connection)); + } -Db::ScopedConnection::~ScopedConnection() -{ - _connectionPool.returnConnection(std::move(_connection)); -} - -Wt::Dbo::SqlConnection* Db::ScopedConnection::operator->() const -{ - return _connection.get(); -} + Wt::Dbo::SqlConnection* Db::ScopedConnection::operator->() const + { + return _connection.get(); + } } // namespace Database diff --git a/src/libs/services/database/impl/Migration.cpp b/src/libs/services/database/impl/Migration.cpp index 5785cce6..6c11b887 100644 --- a/src/libs/services/database/impl/Migration.cpp +++ b/src/libs/services/database/impl/Migration.cpp @@ -654,7 +654,6 @@ CREATE TABLE IF NOT EXISTS "track_backup" ( {41, migrateFromV41}, }; - while (1) { auto uniqueTransaction{ session.createUniqueTransaction() }; @@ -670,28 +669,24 @@ CREATE TABLE IF NOT EXISTS "track_backup" ( throw LmsException{ outdatedMsg }; } - if (version == LMS_DATABASE_VERSION) - { - LMS_LOG(DB, INFO) << "Lms database version " << LMS_DATABASE_VERSION << ": up to date!"; - return; - } - else if (version > LMS_DATABASE_VERSION) - { + if (version > LMS_DATABASE_VERSION) throw LmsException{ "Server binary outdated, please upgrade it to handle this database" }; - } if (version < migrationFunctions.begin()->first) throw LmsException{ outdatedMsg }; - LMS_LOG(DB, INFO) << "Migrating database from version " << version << " to " << version + 1 << "..."; + while (version < LMS_DATABASE_VERSION) + { + LMS_LOG(DB, INFO) << "Migrating database from version " << version << " to " << version + 1 << "..."; - auto itMigrationFunc{ migrationFunctions.find(version) }; - assert(itMigrationFunc != std::cend(migrationFunctions)); - itMigrationFunc->second(session); + auto itMigrationFunc{ migrationFunctions.find(version) }; + assert(itMigrationFunc != std::cend(migrationFunctions)); + itMigrationFunc->second(session); - VersionInfo::get(session).modify()->setVersion(++version); + VersionInfo::get(session).modify()->setVersion(++version); - LMS_LOG(DB, INFO) << "Migration complete to version " << version; + LMS_LOG(DB, INFO) << "Migration complete to version " << version; + } } } } diff --git a/src/libs/services/database/impl/Session.cpp b/src/libs/services/database/impl/Session.cpp index 0aa5d551..90387aaa 100644 --- a/src/libs/services/database/impl/Session.cpp +++ b/src/libs/services/database/impl/Session.cpp @@ -46,145 +46,145 @@ namespace Database { -Session::Session(Db& db) -: _db {db} -{ - _session.setConnectionPool(_db.getConnectionPool()); + Session::Session(Db& db) + : _db{ db } + { + _session.setConnectionPool(_db.getConnectionPool()); - _session.mapClass("version_info"); - _session.mapClass("artist"); - _session.mapClass("auth_token"); - _session.mapClass("cluster"); - _session.mapClass("cluster_type"); - _session.mapClass("listen"); - _session.mapClass("release"); - _session.mapClass("scan_settings"); - _session.mapClass("starred_artist"); - _session.mapClass("starred_release"); - _session.mapClass("starred_track"); - _session.mapClass("track"); - _session.mapClass("track_bookmark"); - _session.mapClass("track_artist_link"); - _session.mapClass("track_features"); - _session.mapClass("tracklist"); - _session.mapClass("tracklist_entry"); - _session.mapClass("user"); -} + _session.mapClass("version_info"); + _session.mapClass("artist"); + _session.mapClass("auth_token"); + _session.mapClass("cluster"); + _session.mapClass("cluster_type"); + _session.mapClass("listen"); + _session.mapClass("release"); + _session.mapClass("scan_settings"); + _session.mapClass("starred_artist"); + _session.mapClass("starred_release"); + _session.mapClass("starred_track"); + _session.mapClass("track"); + _session.mapClass("track_bookmark"); + _session.mapClass("track_artist_link"); + _session.mapClass("track_features"); + _session.mapClass("tracklist"); + _session.mapClass("tracklist_entry"); + _session.mapClass("user"); + } -UniqueTransaction::UniqueTransaction(RecursiveSharedMutex& mutex, Wt::Dbo::Session& session) -: _lock {mutex}, - _transaction {session} -{ -} + UniqueTransaction::UniqueTransaction(RecursiveSharedMutex& mutex, Wt::Dbo::Session& session) + : _lock{ mutex }, + _transaction{ session } + { + } -SharedTransaction::SharedTransaction(RecursiveSharedMutex& mutex, Wt::Dbo::Session& session) -: _lock {mutex}, - _transaction {session} -{ -} + SharedTransaction::SharedTransaction(RecursiveSharedMutex& mutex, Wt::Dbo::Session& session) + : _lock{ mutex }, + _transaction{ session } + { + } -void -Session::checkUniqueLocked() -{ - assert(_db.getMutex().isUniqueLocked()); -} + void Session::checkUniqueLocked() + { + assert(_db.getMutex().isUniqueLocked()); + } -void -Session::checkSharedLocked() -{ - assert(_db.getMutex().isSharedLocked()); -} + void Session::checkSharedLocked() + { + assert(_db.getMutex().isSharedLocked()); + } -UniqueTransaction -Session::createUniqueTransaction() -{ - return UniqueTransaction {_db.getMutex(), _session}; -} + UniqueTransaction Session::createUniqueTransaction() + { + return UniqueTransaction{ _db.getMutex(), _session }; + } -SharedTransaction -Session::createSharedTransaction() -{ - return SharedTransaction {_db.getMutex(), _session}; -} + SharedTransaction Session::createSharedTransaction() + { + return SharedTransaction{ _db.getMutex(), _session }; + } -void -Session::prepareTables() -{ - // Creation case - try - { - _session.createTables(); + void Session::prepareTables() + { + LMS_LOG(DB, INFO) << "Preparing tables..."; - LMS_LOG(DB, INFO) << "Tables created"; - } - catch (Wt::Dbo::Exception& e) - { - LMS_LOG(DB, ERROR) << "Cannot create tables: " << e.what(); - } + // Initial creation case + try + { + _session.createTables(); + LMS_LOG(DB, INFO) << "Tables created"; + } + catch (Wt::Dbo::Exception& e) + { + LMS_LOG(DB, DEBUG) << "Cannot create tables: " << e.what(); + if (std::string_view{ e.what() }.find("already exists") == std::string_view::npos) + { + LMS_LOG(DB, ERROR) << "Cannot create tables: " << e.what(); + throw e; + } + } - Migration::doDbMigration(*this); + Migration::doDbMigration(*this); - // Indexes - { - auto uniqueTransaction {createUniqueTransaction()}; - _session.execute("CREATE INDEX IF NOT EXISTS artist_name_idx ON artist(name)"); - _session.execute("CREATE INDEX IF NOT EXISTS artist_sort_name_nocase_idx ON artist(sort_name COLLATE NOCASE)"); - _session.execute("CREATE INDEX IF NOT EXISTS artist_mbid_idx ON artist(mbid)"); - _session.execute("CREATE INDEX IF NOT EXISTS auth_token_user_idx ON auth_token(user_id)"); - _session.execute("CREATE INDEX IF NOT EXISTS auth_token_expiry_idx ON auth_token(expiry)"); - _session.execute("CREATE INDEX IF NOT EXISTS auth_token_value_idx ON auth_token(value)"); - _session.execute("CREATE INDEX IF NOT EXISTS cluster_name_idx ON cluster(name)"); - _session.execute("CREATE INDEX IF NOT EXISTS cluster_cluster_type_idx ON cluster(cluster_type_id)"); - _session.execute("CREATE INDEX IF NOT EXISTS cluster_type_name_idx ON cluster_type(name)"); - _session.execute("CREATE INDEX IF NOT EXISTS release_name_idx ON release(name)"); - _session.execute("CREATE INDEX IF NOT EXISTS release_name_nocase_idx ON release(name COLLATE NOCASE)"); - _session.execute("CREATE INDEX IF NOT EXISTS release_mbid_idx ON release(mbid)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_file_last_write_idx ON track(file_last_write)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_path_idx ON track(file_path)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_name_idx ON track(name)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_name_nocase_idx ON track(name COLLATE NOCASE)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_mbid_idx ON track(mbid)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_recording_mbid_idx ON track(recording_mbid)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_release_idx ON track(release_id)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_date_idx ON track(date)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_original_date_idx ON track(original_date)"); - _session.execute("CREATE INDEX IF NOT EXISTS tracklist_name_idx ON tracklist(name)"); - _session.execute("CREATE INDEX IF NOT EXISTS tracklist_user_idx ON tracklist(user_id)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_features_track_idx ON track_features(track_id)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_artist_link_artist_idx ON track_artist_link(artist_id)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_artist_link_track_idx ON track_artist_link(track_id)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_artist_link_type_idx ON track_artist_link(type)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_artist_link_artist_type_idx ON track_artist_link(artist_id,type)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_bookmark_user_idx ON track_bookmark(user_id)"); - _session.execute("CREATE INDEX IF NOT EXISTS track_bookmark_user_track_idx ON track_bookmark(user_id,track_id)"); - _session.execute("CREATE INDEX IF NOT EXISTS listen_scrobbler_idx ON listen(scrobbler)"); - _session.execute("CREATE INDEX IF NOT EXISTS listen_user_scrobbler_idx ON listen(user_id,scrobbler)"); - _session.execute("CREATE INDEX IF NOT EXISTS listen_user_track_scrobbler_date_time_idx ON listen(user_id,track_id,scrobbler,date_time)"); - _session.execute("CREATE INDEX IF NOT EXISTS starred_artist_user_scrobbler_idx ON starred_artist(user_id,scrobbler)"); - _session.execute("CREATE INDEX IF NOT EXISTS starred_artist_artist_user_scrobbler_idx ON starred_artist(artist_id,user_id,scrobbler)"); - _session.execute("CREATE INDEX IF NOT EXISTS starred_release_user_scrobbler_idx ON starred_release(user_id,scrobbler)"); - _session.execute("CREATE INDEX IF NOT EXISTS starred_release_release_user_scrobbler_idx ON starred_release(release_id,user_id,scrobbler)"); - _session.execute("CREATE INDEX IF NOT EXISTS starred_track_user_scrobbler_idx ON starred_track(user_id,scrobbler)"); - _session.execute("CREATE INDEX IF NOT EXISTS starred_track_track_user_scrobbler_idx ON starred_track(track_id,user_id,scrobbler)"); - } + // Indexes + { + auto uniqueTransaction{ createUniqueTransaction() }; + _session.execute("CREATE INDEX IF NOT EXISTS artist_name_idx ON artist(name)"); + _session.execute("CREATE INDEX IF NOT EXISTS artist_sort_name_nocase_idx ON artist(sort_name COLLATE NOCASE)"); + _session.execute("CREATE INDEX IF NOT EXISTS artist_mbid_idx ON artist(mbid)"); + _session.execute("CREATE INDEX IF NOT EXISTS auth_token_user_idx ON auth_token(user_id)"); + _session.execute("CREATE INDEX IF NOT EXISTS auth_token_expiry_idx ON auth_token(expiry)"); + _session.execute("CREATE INDEX IF NOT EXISTS auth_token_value_idx ON auth_token(value)"); + _session.execute("CREATE INDEX IF NOT EXISTS cluster_name_idx ON cluster(name)"); + _session.execute("CREATE INDEX IF NOT EXISTS cluster_cluster_type_idx ON cluster(cluster_type_id)"); + _session.execute("CREATE INDEX IF NOT EXISTS cluster_type_name_idx ON cluster_type(name)"); + _session.execute("CREATE INDEX IF NOT EXISTS release_name_idx ON release(name)"); + _session.execute("CREATE INDEX IF NOT EXISTS release_name_nocase_idx ON release(name COLLATE NOCASE)"); + _session.execute("CREATE INDEX IF NOT EXISTS release_mbid_idx ON release(mbid)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_file_last_write_idx ON track(file_last_write)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_path_idx ON track(file_path)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_name_idx ON track(name)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_name_nocase_idx ON track(name COLLATE NOCASE)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_mbid_idx ON track(mbid)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_recording_mbid_idx ON track(recording_mbid)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_release_idx ON track(release_id)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_date_idx ON track(date)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_original_date_idx ON track(original_date)"); + _session.execute("CREATE INDEX IF NOT EXISTS tracklist_name_idx ON tracklist(name)"); + _session.execute("CREATE INDEX IF NOT EXISTS tracklist_user_idx ON tracklist(user_id)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_features_track_idx ON track_features(track_id)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_artist_link_artist_idx ON track_artist_link(artist_id)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_artist_link_track_idx ON track_artist_link(track_id)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_artist_link_type_idx ON track_artist_link(type)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_artist_link_artist_type_idx ON track_artist_link(artist_id,type)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_bookmark_user_idx ON track_bookmark(user_id)"); + _session.execute("CREATE INDEX IF NOT EXISTS track_bookmark_user_track_idx ON track_bookmark(user_id,track_id)"); + _session.execute("CREATE INDEX IF NOT EXISTS listen_scrobbler_idx ON listen(scrobbler)"); + _session.execute("CREATE INDEX IF NOT EXISTS listen_user_scrobbler_idx ON listen(user_id,scrobbler)"); + _session.execute("CREATE INDEX IF NOT EXISTS listen_user_track_scrobbler_date_time_idx ON listen(user_id,track_id,scrobbler,date_time)"); + _session.execute("CREATE INDEX IF NOT EXISTS starred_artist_user_scrobbler_idx ON starred_artist(user_id,scrobbler)"); + _session.execute("CREATE INDEX IF NOT EXISTS starred_artist_artist_user_scrobbler_idx ON starred_artist(artist_id,user_id,scrobbler)"); + _session.execute("CREATE INDEX IF NOT EXISTS starred_release_user_scrobbler_idx ON starred_release(user_id,scrobbler)"); + _session.execute("CREATE INDEX IF NOT EXISTS starred_release_release_user_scrobbler_idx ON starred_release(release_id,user_id,scrobbler)"); + _session.execute("CREATE INDEX IF NOT EXISTS starred_track_user_scrobbler_idx ON starred_track(user_id,scrobbler)"); + _session.execute("CREATE INDEX IF NOT EXISTS starred_track_track_user_scrobbler_idx ON starred_track(track_id,user_id,scrobbler)"); + } - // Initial settings tables - { - auto uniqueTransaction {createUniqueTransaction()}; + // Initial settings tables + { + auto uniqueTransaction{ createUniqueTransaction() }; - ScanSettings::init(*this); - } -} + ScanSettings::init(*this); + } + } -void -Session::optimize() -{ - LMS_LOG(DB, DEBUG) << "Optimizing db..."; - { - auto uniqueTransaction {createUniqueTransaction()}; - _session.execute("ANALYZE"); - } - LMS_LOG(DB, DEBUG) << "Optimized db!"; -} + void Session::analyze() + { + LMS_LOG(DB, INFO) << "Analyzing database..."; + { + auto uniqueTransaction{ createUniqueTransaction() }; + _session.execute("ANALYZE"); + } + LMS_LOG(DB, INFO) << "Database Analyze complete"; + } } // namespace Database diff --git a/src/libs/services/database/include/services/database/Db.hpp b/src/libs/services/database/include/services/database/Db.hpp index b58077e8..a0298eb1 100644 --- a/src/libs/services/database/include/services/database/Db.hpp +++ b/src/libs/services/database/include/services/database/Db.hpp @@ -27,52 +27,47 @@ namespace Database { -class Session; -class Db -{ - public: - Db(const std::filesystem::path& dbPath, std::size_t connectionCount = 10); - ~Db(); + class Session; + class Db + { + public: + Db(const std::filesystem::path& dbPath, std::size_t connectionCount = 10); - Db(const Db&) = delete; - Db(Db&&) = delete; - Db& operator=(const Db&) = delete; - Db& operator=(Db&&) = delete; + Session& getTLSSession(); - Session& getTLSSession(); + void executeSql(const std::string& sql); - void executeSql(const std::string& sql); + private: + Db(const Db&) = delete; + Db& operator=(const Db&) = delete; - private: - friend class Session; + friend class Session; - RecursiveSharedMutex& getMutex() { return _sharedMutex; } - Wt::Dbo::SqlConnectionPool& getConnectionPool() { return *_connectionPool; } + RecursiveSharedMutex& getMutex() { return _sharedMutex; } + Wt::Dbo::SqlConnectionPool& getConnectionPool() { return *_connectionPool; } - class ScopedConnection - { - public: - ScopedConnection(Wt::Dbo::SqlConnectionPool& pool); - ~ScopedConnection(); + class ScopedConnection + { + public: + ScopedConnection(Wt::Dbo::SqlConnectionPool& pool); + ~ScopedConnection(); - ScopedConnection(const ScopedConnection& ) = delete; - ScopedConnection(ScopedConnection&& ) = delete; - ScopedConnection& operator=(const ScopedConnection& ) = delete; - ScopedConnection& operator=(ScopedConnection&& ) = delete; + Wt::Dbo::SqlConnection* operator->() const; - Wt::Dbo::SqlConnection* operator->() const; + private: + ScopedConnection(const ScopedConnection&) = delete; + ScopedConnection& operator=(const ScopedConnection&) = delete; - private: - Wt::Dbo::SqlConnectionPool& _connectionPool; - std::unique_ptr _connection; - }; + Wt::Dbo::SqlConnectionPool& _connectionPool; + std::unique_ptr _connection; + }; - RecursiveSharedMutex _sharedMutex; - std::unique_ptr _connectionPool; + RecursiveSharedMutex _sharedMutex; + std::unique_ptr _connectionPool; - std::mutex _tlsSessionsMutex; - std::vector> _tlsSessions; -}; + std::mutex _tlsSessionsMutex; + std::vector> _tlsSessions; + }; } // namespace Database diff --git a/src/libs/services/database/include/services/database/Session.hpp b/src/libs/services/database/include/services/database/Session.hpp index d12b6cfb..0edba936 100644 --- a/src/libs/services/database/include/services/database/Session.hpp +++ b/src/libs/services/database/include/services/database/Session.hpp @@ -28,68 +28,67 @@ namespace Database { - class UniqueTransaction - { - private: - friend class Session; - UniqueTransaction(RecursiveSharedMutex& mutex, Wt::Dbo::Session& session); + class UniqueTransaction + { + private: + friend class Session; + UniqueTransaction(RecursiveSharedMutex& mutex, Wt::Dbo::Session& session); - std::unique_lock _lock; - Wt::Dbo::Transaction _transaction; - }; + std::unique_lock _lock; + Wt::Dbo::Transaction _transaction; + }; - class SharedTransaction - { - private: - friend class Session; - SharedTransaction(RecursiveSharedMutex& mutex, Wt::Dbo::Session& session); + class SharedTransaction + { + private: + friend class Session; + SharedTransaction(RecursiveSharedMutex& mutex, Wt::Dbo::Session& session); - std::shared_lock _lock; - Wt::Dbo::Transaction _transaction; - }; + std::shared_lock _lock; + Wt::Dbo::Transaction _transaction; + }; - class Db; - class Session - { - public: - Session (Db& database); + class Db; + class Session + { + public: + Session(Db& database); + ~Session(); - Session(const Session&) = delete; - Session(Session&&) = delete; - Session& operator=(const Session&) = delete; - Session& operator=(Session&&) = delete; + [[nodiscard]] UniqueTransaction createUniqueTransaction(); + [[nodiscard]] SharedTransaction createSharedTransaction(); - [[nodiscard]] UniqueTransaction createUniqueTransaction(); - [[nodiscard]] SharedTransaction createSharedTransaction(); + void checkUniqueLocked(); + void checkSharedLocked(); - void checkUniqueLocked(); - void checkSharedLocked(); + void analyze(); - void optimize(); + void prepareTables(); // need to run only once at startup - void prepareTables(); // need to run only once at startup + Wt::Dbo::Session& getDboSession() { return _session; } + Db& getDb() { return _db; } - Wt::Dbo::Session& getDboSession() { return _session; } - Db& getDb() { return _db; } + template + typename Object::pointer create(Args&&... args) + { + checkUniqueLocked(); - template - typename Object::pointer create(Args&&... args) - { - checkUniqueLocked(); + typename Object::pointer res{ Object::create(*this, std::forward(args)...) }; + getDboSession().flush(); - typename Object::pointer res {Object::create(*this, std::forward(args)...)}; - getDboSession().flush(); + if (res->hasOnPostCreated()) + res.modify()->onPostCreated(); - if (res->hasOnPostCreated()) - res.modify()->onPostCreated(); + return res; + } - return res; - } + private: + Session(const Session&) = delete; + Session& operator=(const Session&) = delete; - private: - Db& _db; - Wt::Dbo::Session _session; - }; + Db& _db; + Wt::Dbo::Session _session; + }; } // namespace Database diff --git a/src/libs/services/database/test/Common.cpp b/src/libs/services/database/test/Common.cpp index 89a43da8..c32d8932 100644 --- a/src/libs/services/database/test/Common.cpp +++ b/src/libs/services/database/test/Common.cpp @@ -60,7 +60,7 @@ DatabaseFixture::SetUpTestCase() { Database::Session s {_tmpDb->getDb()}; s.prepareTables(); - s.optimize(); + s.analyze(); // remove default created entries { diff --git a/src/libs/services/scanner/impl/ScannerService.cpp b/src/libs/services/scanner/impl/ScannerService.cpp index e9b4cffb..4c4c1b95 100644 --- a/src/libs/services/scanner/impl/ScannerService.cpp +++ b/src/libs/services/scanner/impl/ScannerService.cpp @@ -299,8 +299,7 @@ ScannerService::scan(bool forceScan) LMS_LOG(DBUPDATER, INFO) << "Scan " << (_abortScan ? "aborted" : "complete") << ". Changes = " << stats.nbChanges() << " (added = " << stats.additions << ", removed = " << stats.deletions << ", updated = " << stats.updates << "), Not changed = " << stats.skips << ", Scanned = " << stats.scans << " (errors = " << stats.errors.size() << "), features fetched = " << stats.featuresFetched << ", duplicates = " << stats.duplicates.size(); - // TODO make it a scan step - _dbSession.optimize(); + _dbSession.analyze(); if (!_abortScan) { diff --git a/src/lms/main.cpp b/src/lms/main.cpp index 4cb86c98..0dd1fe41 100644 --- a/src/lms/main.cpp +++ b/src/lms/main.cpp @@ -238,7 +238,10 @@ int main(int argc, char* argv[]) { Database::Session session {database}; session.prepareTables(); - session.optimize(); + + // force optimize in case scanner aborted during a large import: + // queries may be too slow to even be able to relaunch a scan sing the web interface + session.analyze(); } UserInterface::LmsApplicationManager appManager; From 73f7a75aa6d40c4401ec3dabe91b60ec10b5da70 Mon Sep 17 00:00:00 2001 From: emeric Date: Sun, 22 Oct 2023 00:32:50 +0200 Subject: [PATCH 6/7] fixed build --- src/libs/services/database/include/services/database/Session.hpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libs/services/database/include/services/database/Session.hpp b/src/libs/services/database/include/services/database/Session.hpp index 0edba936..2b36e97d 100644 --- a/src/libs/services/database/include/services/database/Session.hpp +++ b/src/libs/services/database/include/services/database/Session.hpp @@ -53,7 +53,6 @@ namespace Database { public: Session(Db& database); - ~Session(); [[nodiscard]] UniqueTransaction createUniqueTransaction(); [[nodiscard]] SharedTransaction createSharedTransaction(); From 43131d391e205272cadf9cce5e9991c3fc74c005 Mon Sep 17 00:00:00 2001 From: emeric Date: Sun, 22 Oct 2023 11:49:42 +0200 Subject: [PATCH 7/7] Set pragma analysis_limit per connection --- src/libs/services/database/impl/Db.cpp | 43 +++++++++++++++++++++----- 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/src/libs/services/database/impl/Db.cpp b/src/libs/services/database/impl/Db.cpp index 117d18c5..bcac0eaa 100644 --- a/src/libs/services/database/impl/Db.cpp +++ b/src/libs/services/database/impl/Db.cpp @@ -26,18 +26,49 @@ #include "services/database/User.hpp" #include "utils/Logger.hpp" -namespace Database { +namespace Database +{ + namespace + { + class Connection : public Wt::Dbo::backend::Sqlite3 + { + public: + Connection(const std::filesystem::path& dbPath) + : Wt::Dbo::backend::Sqlite3{ dbPath.string() } + , _dbPath{ dbPath } + { + prepare(); + } + + private: + Connection(const Connection&) = delete; + Connection& operator=(const Connection&) = delete; + + std::unique_ptr clone() const override + { + return std::make_unique(_dbPath); + } + + void prepare() + { + LMS_LOG(DB, DEBUG) << "Setting per-connection settings..."; + executeSql("pragma journal_mode=WAL"); + executeSql("pragma synchronous=normal"); + executeSql("pragma analysis_limit=1000"); // to help make analyze command faster + LMS_LOG(DB, DEBUG) << "Setting per-connection settings done!"; + } + + std::filesystem::path _dbPath; + }; + } // Session living class handling the database and the login Db::Db(const std::filesystem::path& dbPath, std::size_t connectionCount) { LMS_LOG(DB, INFO) << "Creating connection pool on file " << dbPath.string(); - auto connection{ std::make_unique(dbPath.string()) }; + auto connection{ std::make_unique(dbPath.string()) }; // connection->setProperty("show-queries", "true"); - connection->executeSql("pragma journal_mode=WAL"); - connection->executeSql("pragma synchronous=normal"); - connection->executeSql("pragma analysis_limit=1000"); // to help make analyze command faster auto connectionPool{ std::make_unique(std::move(connection), connectionCount) }; connectionPool->setTimeout(std::chrono::seconds{ 10 }); @@ -86,5 +117,3 @@ namespace Database { } } // namespace Database - -