common/RenderTiles.hpp | 12 +++++- wsd/ClientSession.cpp | 5 +- wsd/DocumentBroker.cpp | 16 +++++---- wsd/TileCache.cpp | 85 +++++++++++++++++++++++++++---------------------- wsd/TileCache.hpp | 13 +++---- wsd/TileDesc.hpp | 8 ++++ 6 files changed, 82 insertions(+), 57 deletions(-)
New commits: commit 4c6ba6d85096a223b5c6672ee2f4b51c12ca41c3 Author: Michael Meeks <michael.me...@collabora.com> AuthorDate: Fri Aug 7 18:37:53 2020 +0100 Commit: Michael Meeks <michael.me...@collabora.com> CommitDate: Fri Aug 7 20:01:40 2020 +0200 Notify WSD of tiles which we didn't need to render. When we get a wid match, this helps WSD to cleanup its tile subscriber list effectively. Change-Id: I6517039fb3d8c9ad8f53aef549b8adbb79961ce1 Reviewed-on: https://gerrit.libreoffice.org/c/online/+/100348 Tested-by: Jenkins Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoff...@gmail.com> Reviewed-by: Michael Meeks <michael.me...@collabora.com> diff --git a/common/RenderTiles.hpp b/common/RenderTiles.hpp index 011503f12..c5bc9d03e 100644 --- a/common/RenderTiles.hpp +++ b/common/RenderTiles.hpp @@ -638,6 +638,9 @@ namespace RenderTiles // The tile content is identical to what the client already has, so skip it LOG_TRC("Match for tile #" << tileIndex << " at (" << positionX << ',' << positionY << ") oldhash==hash (" << hash << "), wireId: " << wireId << " skipping"); + // Push a zero byte image to inform WSD we didn't need that. + // This allows WSD side TileCache to free up waiting subscribers. + pushRendered(renderedTiles, tiles[tileIndex], wireId, 0); tileIndex++; continue; } @@ -701,13 +704,16 @@ namespace RenderTiles tileIndex++; } + // empty ones come first + size_t zeroCheckStart = renderedTiles.size(); + pngPool.run(); - for (auto &i : renderedTiles) + for (size_t i = zeroCheckStart; i < renderedTiles.size(); ++i) { - if (i.getImgSize() == 0) + if (renderedTiles[i].getImgSize() == 0) { - LOG_ERR("Encoded 0-sized tile!"); + LOG_TRC("Encoded 0-sized tile in slot !" << i); assert(!"0-sized tile enocded!"); } } diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp index 5120bb099..a5e62fcc4 100644 --- a/wsd/DocumentBroker.cpp +++ b/wsd/DocumentBroker.cpp @@ -2034,7 +2034,7 @@ void DocumentBroker::handleTileCombinedResponse(const std::vector<char>& payload try { const size_t length = payload.size(); - if (firstLine.size() < static_cast<std::string::size_type>(length) - 1) + if (firstLine.size() <= static_cast<std::string::size_type>(length) - 1) { const TileCombined tileCombined = TileCombined::parse(firstLine); const char* buffer = payload.data(); diff --git a/wsd/TileCache.cpp b/wsd/TileCache.cpp index e1cdefe8d..982cd985b 100644 --- a/wsd/TileCache.cpp +++ b/wsd/TileCache.cpp @@ -175,19 +175,24 @@ void TileCache::saveTileAndNotify(const TileDesc& tile, const char *data, const { assertCorrectThread(); - // Save to disk. + if (size > 0) + { + // Save to in-memory cache. - // Ignore if we can't save the tile, things will work anyway, but slower. - // An error indication is supposed to be sent to all users in that case. - saveDataToCache(tile, data, size); - LOG_TRC("Saved cache tile: " << cacheFileName(tile) << " of size " << size << " bytes"); + // Ignore if we can't save the tile, things will work anyway, but slower. + // An error indication is supposed to be sent to all users in that case. + saveDataToCache(tile, data, size); + LOG_TRC("Saved cache tile: " << cacheFileName(tile) << " of size " << size << " bytes"); + } + else + LOG_TRC("Zero sized cache tile: " << cacheFileName(tile)); // Notify subscribers, if any. std::shared_ptr<TileBeingRendered> tileBeingRendered = findTileBeingRendered(tile); if (tileBeingRendered) { const size_t subscriberCount = tileBeingRendered->getSubscribers().size(); - if (subscriberCount > 0) + if (size > 0 && subscriberCount > 0) { std::string response = tile.serialize("tile:"); LOG_DBG("Sending tile message to " << subscriberCount << " subscribers: " << response); @@ -229,10 +234,9 @@ void TileCache::saveTileAndNotify(const TileDesc& tile, const char *data, const } } } - else - { + else if (subscriberCount == 0) LOG_DBG("No subscribers for: " << cacheFileName(tile)); - } + // else zero sized // Remove subscriptions. if (tileBeingRendered->getVersion() <= tile.getVersion()) @@ -243,9 +247,7 @@ void TileCache::saveTileAndNotify(const TileDesc& tile, const char *data, const } } else - { LOG_DBG("No subscribers for: " << cacheFileName(tile)); - } } bool TileCache::getTextStream(StreamType type, const std::string& fileName, std::string& content) commit 1061ac90ce212a6b0376102c42b763ba3d6c75d1 Author: Michael Meeks <michael.me...@collabora.com> AuthorDate: Fri Aug 7 17:36:56 2020 +0100 Commit: Michael Meeks <michael.me...@collabora.com> CommitDate: Fri Aug 7 20:01:30 2020 +0200 TileCache: cleanup debug, propagate now more helpfully & fix staleness. Stale tiles were still being counted, unhelpfully. Avoid doing lots of ::now() calls, and yet detect this. Change-Id: Ib1e4b2f1968c1994849bb23ec54e28f6706230ee Reviewed-on: https://gerrit.libreoffice.org/c/online/+/100347 Tested-by: Jenkins Reviewed-by: Michael Meeks <michael.me...@collabora.com> diff --git a/wsd/ClientSession.cpp b/wsd/ClientSession.cpp index baaab79f7..a3fa8e078 100644 --- a/wsd/ClientSession.cpp +++ b/wsd/ClientSession.cpp @@ -1567,9 +1567,8 @@ bool ClientSession::forwardToClient(const std::shared_ptr<Message>& payload) void ClientSession::enqueueSendMessage(const std::shared_ptr<Message>& data) { const std::shared_ptr<DocumentBroker> docBroker = _docBroker.lock(); - // If in the correct thread - no need for wakeups. - if (docBroker) - docBroker->assertCorrectThread(); + LOG_CHECK_RET(docBroker && "Null DocumentBroker instance", ); + docBroker->assertCorrectThread(); const std::string command = data->firstToken(); std::unique_ptr<TileDesc> tile; diff --git a/wsd/DocumentBroker.cpp b/wsd/DocumentBroker.cpp index 98d2c7614..5120bb099 100644 --- a/wsd/DocumentBroker.cpp +++ b/wsd/DocumentBroker.cpp @@ -1700,6 +1700,7 @@ size_t DocumentBroker::getMemorySize() const _sessions.size() * sizeof(ClientSession); } +// Expected to be legacy, ~all new requests are tilecombinedRequests void DocumentBroker::handleTileRequest(TileDesc& tile, const std::shared_ptr<ClientSession>& session) { @@ -1718,17 +1719,18 @@ void DocumentBroker::handleTileRequest(TileDesc& tile, return; } + auto now = std::chrono::steady_clock::now(); if (tile.getBroadcast()) { for (auto& it: _sessions) { if (!it.second->inWaitDisconnected()) - tileCache().subscribeToTileRendering(tile, it.second); + tileCache().subscribeToTileRendering(tile, it.second, now); } } else { - tileCache().subscribeToTileRendering(tile, session); + tileCache().subscribeToTileRendering(tile, session, now); } // Forward to child to render. @@ -1907,6 +1909,8 @@ void DocumentBroker::sendRequestedTiles(const std::shared_ptr<ClientSession>& se // Drop tiles which we are waiting for too long session->removeOutdatedTilesOnFly(); + auto now = std::chrono::steady_clock::now(); + // All tiles were processed on client side that we sent last time, so we can send // a new batch of tiles which was invalidated / requested in the meantime std::deque<TileDesc>& requestedTiles = session->getRequestedTiles(); @@ -1914,7 +1918,7 @@ void DocumentBroker::sendRequestedTiles(const std::shared_ptr<ClientSession>& se { size_t delayedTiles = 0; std::vector<TileDesc> tilesNeedsRendering; - size_t beingRendered = _tileCache->countTilesBeingRenderedForSession(session); + size_t beingRendered = _tileCache->countTilesBeingRenderedForSession(session, now); while (session->getTilesOnFlyCount() + beingRendered < tilesOnFlyUpperLimit && !requestedTiles.empty() && // If we delayed all tiles we don't send any tile (we will when next tileprocessed message arrives) @@ -1944,14 +1948,14 @@ void DocumentBroker::sendRequestedTiles(const std::shared_ptr<ClientSession>& se else { // Not cached, needs rendering. - if (!tileCache().hasTileBeingRendered(tile) || // There is no in progress rendering of the given tile + if (!tileCache().hasTileBeingRendered(tile, &now) || // There is no in progress rendering of the given tile tileCache().getTileBeingRenderedVersion(tile) < tile.getVersion()) // We need a newer version { tile.setVersion(++_tileVersion); tilesNeedsRendering.push_back(tile); _debugRenderedTileCount++; } - tileCache().subscribeToTileRendering(tile, session); + tileCache().subscribeToTileRendering(tile, session, now); beingRendered++; } requestedTiles.pop_front(); diff --git a/wsd/TileCache.cpp b/wsd/TileCache.cpp index 8be7b9aff..e1cdefe8d 100644 --- a/wsd/TileCache.cpp +++ b/wsd/TileCache.cpp @@ -70,19 +70,19 @@ void TileCache::clear() /// rendering latency. struct TileCache::TileBeingRendered { - explicit TileBeingRendered(const TileDesc& tile) - : _startTime(std::chrono::steady_clock::now()) - , _tile(tile) - { - } + explicit TileBeingRendered(const TileDesc& tile, const std::chrono::steady_clock::time_point &now) + : _startTime(now), _tile(tile) { } const TileDesc& getTile() const { return _tile; } int getVersion() const { return _tile.getVersion(); } void setVersion(int version) { _tile.setVersion(version); } std::chrono::steady_clock::time_point getStartTime() const { return _startTime; } - double getElapsedTimeMs() const { return std::chrono::duration_cast<std::chrono::milliseconds> - (std::chrono::steady_clock::now() - _startTime).count(); } + double getElapsedTimeMs(const std::chrono::steady_clock::time_point *now = nullptr) const + { return std::chrono::duration_cast<std::chrono::milliseconds> + ((now ? *now : std::chrono::steady_clock::now()) - _startTime).count(); } + bool isStale(const std::chrono::steady_clock::time_point *now = nullptr) const + { return getElapsedTimeMs(now) > COMMAND_TIMEOUT_MS; } std::vector<std::weak_ptr<ClientSession>>& getSubscribers() { return _subscribers; } void dumpState(std::ostream& os); @@ -93,11 +93,15 @@ private: TileDesc _tile; }; -size_t TileCache::countTilesBeingRenderedForSession(const std::shared_ptr<ClientSession>& session) +size_t TileCache::countTilesBeingRenderedForSession(const std::shared_ptr<ClientSession>& session, + const std::chrono::steady_clock::time_point &now) { size_t count = 0; for (auto& it : _tilesBeingRendered) { + if (it.second->isStale(&now)) + continue; + for (auto& s : it.second->getSubscribers()) { if (s.lock() == session) @@ -108,6 +112,16 @@ size_t TileCache::countTilesBeingRenderedForSession(const std::shared_ptr<Client return count; } +bool TileCache::hasTileBeingRendered(const TileDesc& tileDesc, const std::chrono::steady_clock::time_point *now) const +{ + const auto it = _tilesBeingRendered.find(tileDesc); + if (it == _tilesBeingRendered.end()) + return false; + + /// did we stall ? if so re-issue. + return !now ? true : !it->second->isStale(now); +} + std::shared_ptr<TileCache::TileBeingRendered> TileCache::findTileBeingRendered(const TileDesc& tileDesc) { assertCorrectThread(); @@ -397,47 +411,40 @@ bool TileCache::intersectsTile(const TileDesc &tileDesc, int part, int x, int y, } // FIXME: to be further simplified when we centralize tile messages. -void TileCache::subscribeToTileRendering(const TileDesc& tile, const std::shared_ptr<ClientSession>& subscriber) +void TileCache::subscribeToTileRendering(const TileDesc& tile, const std::shared_ptr<ClientSession>& subscriber, + const std::chrono::steady_clock::time_point &now) { - std::ostringstream oss; - oss << '(' << tile.getNormalizedViewId() << ',' << tile.getPart() << ',' << tile.getTilePosX() << ',' << tile.getTilePosY() << ')'; - const std::string name = oss.str(); - assertCorrectThread(); std::shared_ptr<TileBeingRendered> tileBeingRendered = findTileBeingRendered(tile); if (tileBeingRendered) { + if (tileBeingRendered->isStale(&now)) + LOG_DBG("Painting stalled; need to re-issue on tile " << tile.debugName()); + for (const auto &s : tileBeingRendered->getSubscribers()) { if (s.lock().get() == subscriber.get()) { - LOG_DBG("Redundant request to subscribe on tile " << name); + LOG_DBG("Redundant request to subscribe on tile " << tile.debugName()); tileBeingRendered->setVersion(tile.getVersion()); return; } } - LOG_DBG("Subscribing " << subscriber->getName() << " to tile " << name << " which has " << + LOG_DBG("Subscribing " << subscriber->getName() << " to tile " << tile.debugName() << " which has " << tileBeingRendered->getSubscribers().size() << " subscribers already."); tileBeingRendered->getSubscribers().push_back(subscriber); - - const auto duration = (std::chrono::steady_clock::now() - tileBeingRendered->getStartTime()); - if (std::chrono::duration_cast<std::chrono::milliseconds>(duration).count() > COMMAND_TIMEOUT_MS) - { - // Tile painting has stalled. Reissue. - tileBeingRendered->setVersion(tile.getVersion()); - } } else { - LOG_DBG("Subscribing " << subscriber->getName() << " to tile " << name << + LOG_DBG("Subscribing " << subscriber->getName() << " to tile " << tile.debugName() << " ver=" << tile.getVersion() << " which has no subscribers " << tile.serialize()); assert(_tilesBeingRendered.find(tile) == _tilesBeingRendered.end()); - tileBeingRendered = std::make_shared<TileBeingRendered>(tile); + tileBeingRendered = std::make_shared<TileBeingRendered>(tile, now); tileBeingRendered->getSubscribers().push_back(subscriber); _tilesBeingRendered[tile] = tileBeingRendered; } @@ -446,10 +453,10 @@ void TileCache::subscribeToTileRendering(const TileDesc& tile, const std::shared void TileCache::registerTileBeingRendered(const TileDesc& tile) { std::shared_ptr<TileBeingRendered> tileBeingRendered = findTileBeingRendered(tile); + auto now = std::chrono::steady_clock::now(); if (tileBeingRendered) { - const auto duration = (std::chrono::steady_clock::now() - tileBeingRendered->getStartTime()); - if (std::chrono::duration_cast<std::chrono::milliseconds>(duration).count() > COMMAND_TIMEOUT_MS) + if (tileBeingRendered->isStale(&now)) { // Tile painting has stalled. Reissue. tileBeingRendered->setVersion(tile.getVersion()); @@ -459,7 +466,7 @@ void TileCache::registerTileBeingRendered(const TileDesc& tile) { assert(_tilesBeingRendered.find(tile) == _tilesBeingRendered.end()); - tileBeingRendered = std::make_shared<TileBeingRendered>(tile); + tileBeingRendered = std::make_shared<TileBeingRendered>(tile, now); _tilesBeingRendered[tile] = tileBeingRendered; } } diff --git a/wsd/TileCache.hpp b/wsd/TileCache.hpp index 04c3b8e17..8044919df 100644 --- a/wsd/TileCache.hpp +++ b/wsd/TileCache.hpp @@ -81,7 +81,8 @@ public: /// Subscribes if no subscription exists and returns the version number. /// Otherwise returns 0 to signify a subscription exists. - void subscribeToTileRendering(const TileDesc& tile, const std::shared_ptr<ClientSession>& subscriber); + void subscribeToTileRendering(const TileDesc& tile, const std::shared_ptr<ClientSession>& subscriber, + const std::chrono::steady_clock::time_point& now); /// Create the TileBeingRendered object for the given tile indicating that the tile was sent to /// the kit for rendering. Note: subscribeToTileRendering calls this internally, so you don't need @@ -126,13 +127,11 @@ public: void forgetTileBeingRendered(const std::shared_ptr<TileCache::TileBeingRendered>& tileBeingRendered); double getTileBeingRenderedElapsedTimeMs(const TileDesc &tileDesc) const; - size_t countTilesBeingRenderedForSession(const std::shared_ptr<ClientSession>& session); - inline bool hasTileBeingRendered(const TileDesc& tileDesc) const - { - return _tilesBeingRendered.find(tileDesc) != _tilesBeingRendered.end(); - } + size_t countTilesBeingRenderedForSession(const std::shared_ptr<ClientSession>& session, + const std::chrono::steady_clock::time_point& now); + bool hasTileBeingRendered(const TileDesc& tileDesc, const std::chrono::steady_clock::time_point *now = nullptr) const; - int getTileBeingRenderedVersion(const TileDesc& tileDesc); + int getTileBeingRenderedVersion(const TileDesc& tileDesc); /// Set the high watermark for tilecache size void setMaxCacheSize(size_t cacheSize); diff --git a/wsd/TileDesc.hpp b/wsd/TileDesc.hpp index cec900d50..28ecc4256 100644 --- a/wsd/TileDesc.hpp +++ b/wsd/TileDesc.hpp @@ -193,6 +193,14 @@ public: return oss.str(); } + /// short name for a tile for debugging. + std::string debugName() const + { + std::ostringstream oss; + oss << '(' << getNormalizedViewId() << ',' << getPart() << ',' << getTilePosX() << ',' << getTilePosY() << ')'; + return oss.str(); + } + /// Deserialize a TileDesc from a tokenized string. static TileDesc parse(const StringVector& tokens) { _______________________________________________ Libreoffice-commits mailing list libreoffice-comm...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/libreoffice-commits