Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Experiment with Qt HTTP caching options #1012

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1237,16 +1237,21 @@ void ModelEntityRenderer::doRenderUpdateAsynchronousTyped(const TypedEntityPoint
render::Transaction transaction;

// Check for removal
if (!_hasModel) {
bool shouldReloadModelURL = entity->shouldForceReloadModelURL();
if (!_hasModel || shouldReloadModelURL) {
if (model) {
model->removeFromScene(scene, transaction);
entity->bumpAncestorChainRenderableVersion();
emit DependencyManager::get<scriptable::ModelProviderFactory>()->
modelRemovedFromScene(entity->getEntityItemID(), NestableType::Entity, model);
withWriteLock([&] { _model.reset(); });
scene->enqueueTransaction(transaction);
}
_didLastVisualGeometryRequestSucceed = false;
setKey(_didLastVisualGeometryRequestSucceed, model);
if (shouldReloadModelURL) {
emit requestRenderUpdate();
}
return;
}

Expand Down
7 changes: 7 additions & 0 deletions libraries/entities/src/EntityScriptingInterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1329,6 +1329,13 @@ ParabolaToEntityIntersectionResult EntityScriptingInterface::evalParabolaInterse
return result;
}

void EntityScriptingInterface::reloadModelURL(const QUuid& entityID) {
if (auto entity = checkForTreeEntityAndTypeMatch(entityID, EntityTypes::Model)) {
auto modelEntity = std::dynamic_pointer_cast<ModelEntityItem>(entity);
modelEntity->forceReloadModelURL();
}
}

bool EntityScriptingInterface::reloadServerScripts(const QUuid& entityID) {
auto client = DependencyManager::get<EntityScriptClient>();
return client->reloadServerScript(entityID);
Expand Down
14 changes: 11 additions & 3 deletions libraries/entities/src/EntityScriptingInterface.h
Original file line number Diff line number Diff line change
Expand Up @@ -837,10 +837,18 @@ public slots:
const QScriptValue& entityIdsToInclude = QScriptValue(), const QScriptValue& entityIdsToDiscard = QScriptValue(),
bool visibleOnly = false, bool collidableOnly = false) const;

/*@jsdoc
* Reloads an entity's server entity script such that the latest version re-downloaded.

/**jsdoc
* Reloads an entity's model URL such that the latest version is re-downloaded.
* @function Entities.reloadModelURL
* @param {Uuid} entityID - The ID of the entity for which to reload the modelURL.
*/
Q_INVOKABLE void reloadModelURL(const QUuid& entityID);

/**jsdoc
* Reloads an entity's server entity script such that the latest version is re-downloaded.
* @function Entities.reloadServerScripts
* @param {Uuid} entityID - The ID of the entity to reload the server entity script of.
* @param {Uuid} entityID - The ID of the entity for which to reload the server entity script.
* @returns {boolean} <code>true</code> if the reload request was successfully sent to the server, otherwise
* <code>false</code>.
*/
Expand Down
16 changes: 16 additions & 0 deletions libraries/entities/src/ModelEntityItem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -754,3 +754,19 @@ bool ModelEntityItem::getUseOriginalPivot() const {
return _useOriginalPivot;
});
}

void ModelEntityItem::forceReloadModelURL() {
withWriteLock([&] {
_forceReloadModelURL = true;
_needsRenderUpdate = true;
});
somethingChangedNotification();
}

bool ModelEntityItem::shouldForceReloadModelURL() {
return resultWithWriteLock<bool>([&] {
bool toReturn = _forceReloadModelURL;
_forceReloadModelURL = false; // we also reset this here so we don't need to lock + unlock multiple times
return toReturn;
});
}
4 changes: 4 additions & 0 deletions libraries/entities/src/ModelEntityItem.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ class ModelEntityItem : public EntityItem {
bool getUseOriginalPivot() const;
void setUseOriginalPivot(bool useOriginalPivot);

void forceReloadModelURL();
bool shouldForceReloadModelURL();

private:
void setAnimationSettings(const QString& value); // only called for old bitstream format
bool applyNewAnimationProperties(AnimationPropertyGroup newProperties);
Expand Down Expand Up @@ -157,6 +160,7 @@ class ModelEntityItem : public EntityItem {
bool _groupCulled { false };
QVariantMap _blendshapeCoefficientsMap;
bool _useOriginalPivot { false };
bool _forceReloadModelURL { false };

ThreadSafeValueCache<QString> _compoundShapeURL;

Expand Down
30 changes: 0 additions & 30 deletions libraries/networking/src/AssetClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#include <QtScript/QScriptEngine>
#include <QtNetwork/QNetworkDiskCache>

#include <shared/GlobalAppProperties.h>
#include <shared/MiniPromises.h>

#include "AssetRequest.h"
Expand All @@ -35,7 +34,6 @@
MessageID AssetClient::_currentID = 0;

AssetClient::AssetClient() {
_cacheDir = qApp->property(hifi::properties::APP_LOCAL_DATA_PATH).toString();
setCustomDeleter([](Dependency* dependency){
static_cast<AssetClient*>(dependency)->deleteLater();
});
Expand All @@ -57,34 +55,6 @@ AssetClient::AssetClient() {
this, &AssetClient::handleNodeClientConnectionReset);
}

void AssetClient::initCaching() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

more info below, but for some reason, it seems like we were only setting up caching for this AssetClient, not for all other requests? this code moved to NetworkAccessManager::getInstance()

Q_ASSERT(QThread::currentThread() == thread());

// Setup disk cache if not already
auto& networkAccessManager = NetworkAccessManager::getInstance();
if (!networkAccessManager.cache()) {
if (_cacheDir.isEmpty()) {
#ifdef Q_OS_ANDROID
QString cachePath = QStandardPaths::writableLocation(QStandardPaths::CacheLocation);
#else
QString cachePath = QStandardPaths::writableLocation(QStandardPaths::DataLocation);
#endif
_cacheDir = !cachePath.isEmpty() ? cachePath : "interfaceCache";
}
QNetworkDiskCache* cache = new QNetworkDiskCache();
cache->setMaximumCacheSize(MAXIMUM_CACHE_SIZE);
cache->setCacheDirectory(_cacheDir);
networkAccessManager.setCache(cache);
qInfo() << "ResourceManager disk cache setup at" << _cacheDir
<< "(size:" << MAXIMUM_CACHE_SIZE / BYTES_PER_GIGABYTES << "GB)";
} else {
auto cache = qobject_cast<QNetworkDiskCache*>(networkAccessManager.cache());
qInfo() << "ResourceManager disk cache already setup at" << cache->cacheDirectory()
<< "(size:" << cache->maximumCacheSize() / BYTES_PER_GIGABYTES << "GB)";
}

}

namespace {
const QString& CACHE_ERROR_MESSAGE{ "AssetClient::Error: %1 %2" };
}
Expand Down
4 changes: 0 additions & 4 deletions libraries/networking/src/AssetClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ class AssetClient : public QObject, public Dependency {
Q_INVOKABLE AssetUpload* createUpload(const QByteArray& data);

public slots:
void initCaching();

void cacheInfoRequest(QObject* reciever, QString slot);
MiniPromise::Promise cacheInfoRequestAsync(MiniPromise::Promise deferred = nullptr);
MiniPromise::Promise queryCacheMetaAsync(const QUrl& url, MiniPromise::Promise deferred = nullptr);
Expand Down Expand Up @@ -117,8 +115,6 @@ private slots:
std::unordered_map<SharedNodePointer, std::unordered_map<MessageID, GetInfoCallback>> _pendingInfoRequests;
std::unordered_map<SharedNodePointer, std::unordered_map<MessageID, UploadResultCallback>> _pendingUploads;

QString _cacheDir;

friend class AssetRequest;
friend class AssetUpload;
friend class MappingRequest;
Expand Down
3 changes: 0 additions & 3 deletions libraries/networking/src/BaseAssetScriptingInterface.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,6 @@ bool BaseAssetScriptingInterface::initializeCache() {
return true; // cache is ready
}

// attempt to initialize the cache
QMetaObject::invokeMethod(assetClient().data(), "initCaching");

Promise deferred = makePromise("BaseAssetScriptingInterface--queryCacheStatus");
deferred->then([this](QVariantMap result) {
_cacheReady = !result.value("cacheDirectory").toString().isEmpty();
Expand Down
2 changes: 1 addition & 1 deletion libraries/networking/src/HTTPResourceRequest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ void HTTPResourceRequest::doSend() {
networkRequest.setHeader(QNetworkRequest::UserAgentHeader, NetworkingConstants::VIRCADIA_USER_AGENT);

if (_cacheEnabled) {
networkRequest.setAttribute(QNetworkRequest::CacheLoadControlAttribute, QNetworkRequest::PreferCache);
networkRequest.setAttribute(QNetworkRequest::CacheLoadControlAttribute, QNetworkRequest::PreferNetwork);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this should theoretically fix the cachebusting issue, see the docs for more info: https://doc.qt.io/qt-5/qnetworkrequest.html#CacheLoadControl-enum

} else {
networkRequest.setAttribute(QNetworkRequest::CacheLoadControlAttribute, QNetworkRequest::AlwaysNetwork);
}
Expand Down
32 changes: 28 additions & 4 deletions libraries/networking/src/NetworkAccessManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,42 @@

#include "NetworkAccessManager.h"

#include <QThreadStorage>

#include "AtpReply.h"
#include "ResourceCache.h"

#include <QStandardPaths>
#include <QThreadStorage>
#include <QtNetwork/QNetworkProxy>
#include <QtNetwork/QNetworkDiskCache>

#include <shared/GlobalAppProperties.h>

QThreadStorage<QNetworkAccessManager*> networkAccessManagers;

QNetworkAccessManager& NetworkAccessManager::getInstance() {
if (!networkAccessManagers.hasLocalData()) {
networkAccessManagers.setLocalData(new QNetworkAccessManager());
auto networkAccessManager = new QNetworkAccessManager();
// Setup disk cache if not already
if (!networkAccessManager->cache()) {
QString cacheDir = qApp->property(hifi::properties::APP_LOCAL_DATA_PATH).toString();
if (cacheDir.isEmpty()) {
#ifdef Q_OS_ANDROID
QString cachePath = QStandardPaths::writableLocation(QStandardPaths::CacheLocation);
#else
QString cachePath = QStandardPaths::writableLocation(QStandardPaths::DataLocation);
#endif
cacheDir = !cachePath.isEmpty() ? cachePath : "interfaceCache";
}
QNetworkDiskCache* cache = new QNetworkDiskCache();
cache->setMaximumCacheSize(MAXIMUM_CACHE_SIZE);
cache->setCacheDirectory(cacheDir);
networkAccessManager->setCache(cache);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

according to the docs: https://doc.qt.io/qt-5/qnetworkaccessmanager.html#setCache

"QNetworkAccessManager by default does not have a set cache."

so...I think we were forcing PreferCache above, but then...not actually setting up a cache? hopefully this new code is more correct and more performant

qInfo() << "NetworkAccessManager disk cache set up at" << cacheDir
<< "(size:" << MAXIMUM_CACHE_SIZE / BYTES_PER_GIGABYTES << "GB)";
}
networkAccessManagers.setLocalData(networkAccessManager);
}

return *networkAccessManagers.localData();
}

Expand Down
4 changes: 0 additions & 4 deletions libraries/networking/src/ResourceManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@

#include <mutex>

#include <QNetworkDiskCache>
#include <QStandardPaths>
#include <QThread>
#include <QFileInfo>

#include <SharedUtil.h>
Expand All @@ -37,7 +34,6 @@ ResourceManager::ResourceManager(bool atpSupportEnabled) : _atpSupportEnabled(at
assetClient->moveToThread(&_thread);
QObject::connect(&_thread, &QThread::started, assetClient.data(), [assetClient, name] {
setThreadName(name.toStdString());
assetClient->initCaching();
});
}

Expand Down
6 changes: 6 additions & 0 deletions scripts/system/create/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -2601,6 +2601,12 @@ var PropertiesTool = function (opts) {
pushCommandForSelections();
selectionManager._update(false, this);
}
} else if (data.action === "reloadModelURL") {
if (selectionManager.hasSelection()) {
for (i = 0; i < selectionManager.selections.length; i++) {
Entities.reloadModelURL(selectionManager.selections[i]);
}
}
} else if (data.action === "reloadClientScripts") {
if (selectionManager.hasSelection()) {
var timestamp = Date.now();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -612,6 +612,7 @@ const GROUPS = [
type: "string",
placeholder: "URL",
propertyID: "modelURL",
buttons: [ { id: "reload", label: "F", className: "glyph", onClick: reloadModelURL } ],
hideIfCertified: true,
},
{
Expand Down Expand Up @@ -3224,6 +3225,13 @@ function resetToNaturalDimensions() {
}));
}

function reloadModelURL() {
EventBridge.emitWebEvent(JSON.stringify({
type: "action",
action: "reloadModelURL"
}));
}

function reloadScripts() {
EventBridge.emitWebEvent(JSON.stringify({
type: "action",
Expand Down
3 changes: 0 additions & 3 deletions tools/atp-client/src/ATPClientApp.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,6 @@ ATPClientApp::ATPClientApp(int argc, char* argv[]) :
accountManager->requestAccessToken(_username, _password);
}

auto assetClient = DependencyManager::set<AssetClient>();
assetClient->initCaching();

if (_verbose) {
qDebug() << "domain-server address is" << _domainServerAddress;
}
Expand Down