Skip to content

Commit

Permalink
Performance: Minor optimizations
Browse files Browse the repository at this point in the history 
Addressing a few bottlenecks, particularly related to PathTree
lookups and bindings evaluation. The common theme is avoiding
repeated String construction from C string literals.
  • Loading branch information
@skyjake
skyjake committed Jan 9, 2016
1 parent 99b4e5e commit 1b6948f
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 53 deletions.
15 changes: 10 additions & 5 deletions doomsday/apps/client/include/resource/resourcesystem.h
View file
Expand Up @@ -191,10 +191,7 @@ class ResourceSystem : public res::System
*
* @see hasMaterialManifest(), MaterialManifest::materialPtr()
*/
inline Material *materialPtr(de::Uri const &path) {
if(hasMaterialManifest(path)) return materialManifest(path).materialPtr();
return nullptr;
}
Material *materialPtr(de::Uri const &path);

/**
* Determines if a manifest exists for a material on @a path.
Expand All @@ -204,13 +201,21 @@ class ResourceSystem : public res::System
bool hasMaterialManifest(de::Uri const &path) const;

/**
* Lookup a material manifest by it's unique resource @a path.
* Look up a material manifest by its unique resource @a path.
*
* @param path The path to search for.
* @return Found material manifest.
*/
MaterialManifest &materialManifest(de::Uri const &path) const;

/**
* Look up a material manifest by its unique resource @a path.
*
* @param path The path to search for.
* @return Found material manifest, or nullptr if not found.
*/
MaterialManifest *materialManifestPtr(de::Uri const &path) const;

/**
* Lookup a manifest by unique identifier.
*
Expand Down
34 changes: 21 additions & 13 deletions doomsday/apps/client/src/resource/resourcesystem.cpp
View file
Expand Up @@ -2581,29 +2581,39 @@ MaterialManifest &ResourceSystem::toMaterialManifest(materialid_t id) const
throw UnknownMaterialIdError("ResourceSystem::toMaterialManifest", "Invalid material ID " + String::number(id) + ", valid range " + Rangei(1, d->materialManifestCount + 1).asText());
}

Material *ResourceSystem::materialPtr(de::Uri const &path)
{
if(auto *manifest = materialManifestPtr(path)) return manifest->materialPtr();
return nullptr;
}

bool ResourceSystem::hasMaterialManifest(de::Uri const &path) const
{
try
return materialManifestPtr(path) != nullptr;
}

MaterialManifest &ResourceSystem::materialManifest(de::Uri const &uri) const
{
if(auto *mm = materialManifestPtr(uri))
{
materialManifest(path);
return true;
return *mm;
}
catch(MissingResourceManifestError const &)
{} // Ignore this error.
return false;
/// @throw MissingResourceManifestError Failed to locate a matching manifest.
throw MissingResourceManifestError("ResourceSystem::materialManifest",
"Failed to locate a manifest matching \"" + uri.asText() + "\"");
}

MaterialManifest &ResourceSystem::materialManifest(de::Uri const &uri) const
MaterialManifest *ResourceSystem::materialManifestPtr(de::Uri const &uri) const
{
LOG_AS("ResourceSystem::materialManifest");
LOG_AS("ResourceSystem::materialManifestPtr");

// Does the user want a manifest in a specific scheme?
if(!uri.scheme().isEmpty())
{
MaterialScheme &specifiedScheme = materialScheme(uri.scheme());
if(specifiedScheme.has(uri.path()))
{
return specifiedScheme.find(uri.path());
return &specifiedScheme.find(uri.path());
}
}
else
Expand All @@ -2613,13 +2623,11 @@ MaterialManifest &ResourceSystem::materialManifest(de::Uri const &uri) const
{
if(scheme->has(uri.path()))
{
return scheme->find(uri.path());
return &scheme->find(uri.path());
}
}
}

/// @throw MissingResourceManifestError Failed to locate a matching manifest.
throw MissingResourceManifestError("ResourceSystem::materialManifest", "Failed to locate a manifest matching \"" + uri.asText() + "\"");
return nullptr;
}

dint ResourceSystem::materialCount() const
Expand Down
64 changes: 35 additions & 29 deletions doomsday/apps/client/src/ui/bindcontext.cpp
View file
Expand Up @@ -33,6 +33,12 @@

using namespace de;

static String const VAR_ID ("id");
static String const VAR_TYPE ("type");
static String const VAR_TEST ("test");
static String const VAR_DEVICE_ID ("deviceId");
static String const VAR_CONTROL_ID("controlId");

DENG2_PIMPL(BindContext)
{
bool active = false; ///< @c true= Bindings are active.
Expand Down Expand Up @@ -86,12 +92,12 @@ DENG2_PIMPL(BindContext)
{
CommandBinding bind(*rec);

if(matchCmd && matchCmd.geti("id") != rec->geti("id"))
if(matchCmd && matchCmd.geti(VAR_ID) != rec->geti(VAR_ID))
{
if(matchCmd.geti("type") == bind.geti("type") &&
matchCmd.geti("test") == bind.geti("test") &&
matchCmd.geti("deviceId") == bind.geti("deviceId") &&
matchCmd.geti("controlId") == bind.geti("controlId") &&
if(matchCmd.geti(VAR_TYPE) == bind.geti(VAR_TYPE) &&
matchCmd.geti(VAR_TEST) == bind.geti(VAR_TEST) &&
matchCmd.geti(VAR_DEVICE_ID) == bind.geti(VAR_DEVICE_ID) &&
matchCmd.geti(VAR_CONTROL_ID) == bind.geti(VAR_CONTROL_ID) &&
matchCmd.equalConditions(bind))
{
*cmdResult = const_cast<Record *>(rec);
Expand All @@ -100,9 +106,9 @@ DENG2_PIMPL(BindContext)
}
if(matchImp)
{
if(matchImp.geti("type") == bind.geti("type") &&
matchImp.geti("deviceId") == bind.geti("deviceId") &&
matchImp.geti("controlId") == bind.geti("controlId") &&
if(matchImp.geti(VAR_TYPE) == bind.geti(VAR_TYPE) &&
matchImp.geti(VAR_DEVICE_ID) == bind.geti(VAR_DEVICE_ID) &&
matchImp.geti(VAR_CONTROL_ID) == bind.geti(VAR_CONTROL_ID) &&
matchImp.equalConditions(bind))
{
*cmdResult = const_cast<Record *>(rec);
Expand All @@ -118,21 +124,21 @@ DENG2_PIMPL(BindContext)

if(matchCmd)
{
if(matchCmd.geti("type") == bind.geti("type") &&
matchCmd.geti("deviceId") == bind.geti("deviceId") &&
matchCmd.geti("controlId") == bind.geti("controlId") &&
if(matchCmd.geti(VAR_TYPE) == bind.geti(VAR_TYPE) &&
matchCmd.geti(VAR_DEVICE_ID) == bind.geti(VAR_DEVICE_ID) &&
matchCmd.geti(VAR_CONTROL_ID) == bind.geti(VAR_CONTROL_ID) &&
matchCmd.equalConditions(bind))
{
*impResult = const_cast<Record *>(rec);
return true;
}
}

if(matchImp && matchImp.geti("id") != bind.geti("id"))
if(matchImp && matchImp.geti(VAR_ID) != bind.geti(VAR_ID))
{
if(matchImp.geti("type") == bind.geti("type") &&
matchImp.geti("deviceId") == bind.geti("deviceId") &&
matchImp.geti("controlId") == bind.geti("controlId") &&
if(matchImp.geti(VAR_TYPE) == bind.geti(VAR_TYPE) &&
matchImp.geti(VAR_DEVICE_ID) == bind.geti(VAR_DEVICE_ID) &&
matchImp.geti(VAR_CONTROL_ID) == bind.geti(VAR_CONTROL_ID) &&
matchImp.equalConditions(bind))
{
*impResult = const_cast<Record *>(rec);
Expand All @@ -156,11 +162,11 @@ DENG2_PIMPL(BindContext)
while(findMatchingBinding(cmdBinding, impBinding, &foundCmd, &foundImp))
{
// Only either foundCmd or foundImp is returned as non-NULL.
int bindId = (foundCmd? foundCmd->geti("id") : (foundImp? foundImp->geti("id") : 0));
int bindId = (foundCmd? foundCmd->geti(VAR_ID) : (foundImp? foundImp->geti(VAR_ID) : 0));
if(bindId)
{
LOG_INPUT_VERBOSE("Deleting binding %i, it has been overridden by binding %i")
<< bindId << (cmdBinding? cmdBinding->geti("id") : impBinding->geti("id"));
<< bindId << (cmdBinding? cmdBinding->geti(VAR_ID) : impBinding->geti(VAR_ID));
self.deleteBinding(bindId);
}
}
Expand Down Expand Up @@ -288,17 +294,17 @@ void BindContext::clearBindingsForDevice(int deviceId)
QSet<int> ids;
forAllCommandBindings([&ids, &deviceId] (Record const &bind)
{
if(bind.geti("deviceId") == deviceId)
if(bind.geti(VAR_DEVICE_ID) == deviceId)
{
ids.insert(bind.geti("id"));
ids.insert(bind.geti(VAR_ID));
}
return LoopContinue;
});
forAllImpulseBindings([&ids, &deviceId] (Record const &bind)
{
if(bind.geti("deviceId") == deviceId)
if(bind.geti(VAR_DEVICE_ID) == deviceId)
{
ids.insert(bind.geti("id"));
ids.insert(bind.geti(VAR_ID));
}
return LoopContinue;
});
Expand All @@ -323,7 +329,7 @@ Record *BindContext::bindCommand(char const *eventDesc, char const *command)

LOG_INPUT_VERBOSE("Command " _E(b) "\"%s\"" _E(.) " now bound to "
_E(b) "\"%s\"" _E(.) " in " _E(b) "'%s'" _E(.) " (id %i)")
<< command << bind.composeDescriptor() << d->name << bind.geti("id");
<< command << bind.composeDescriptor() << d->name << bind.geti(VAR_ID);

/// @todo: In interactive binding mode, should ask the user if the
/// replacement is ok. For now, just delete the other bindings.
Expand Down Expand Up @@ -354,7 +360,7 @@ Record *BindContext::bindImpulse(char const *ctrlDesc, PlayerImpulse const &impu

LOG_INPUT_VERBOSE("Impulse " _E(b) "'%s'" _E(.) " of player%i now bound to \"%s\" in " _E(b) "'%s'" _E(.)
" (id %i)")
<< impulse.name << (localPlayer + 1) << bind.composeDescriptor() << d->name << bind.geti("id");
<< impulse.name << (localPlayer + 1) << bind.composeDescriptor() << d->name << bind.geti(VAR_ID);

/// @todo: In interactive binding mode, should ask the user if the
/// replacement is ok. For now, just delete the other bindings.
Expand All @@ -379,7 +385,7 @@ Record *BindContext::findCommandBinding(char const *command, int deviceId) const
CommandBinding bind(*rec);
if(bind.gets("command").compareWithoutCase(command)) continue;

if((deviceId < 0 || deviceId >= NUM_INPUT_DEVICES) || bind.geti("deviceId") == deviceId)
if((deviceId < 0 || deviceId >= NUM_INPUT_DEVICES) || bind.geti(VAR_DEVICE_ID) == deviceId)
{
return const_cast<Record *>(rec);
}
Expand All @@ -394,9 +400,9 @@ Record *BindContext::findImpulseBinding(int deviceId, ibcontroltype_t bindType,
for(Record const *rec : d->impulseBinds[i])
{
ImpulseBinding bind(*rec);
if(bind.geti("type") == bindType &&
bind.geti("deviceId") == deviceId &&
bind.geti("controlId") == controlId)
if(bind.geti(VAR_TYPE) == bindType &&
bind.geti(VAR_DEVICE_ID) == deviceId &&
bind.geti(VAR_CONTROL_ID) == controlId)
{
return const_cast<Record *>(rec);
}
Expand All @@ -410,7 +416,7 @@ bool BindContext::deleteBinding(int id)
for(int i = 0; i < d->commandBinds.count(); ++i)
{
Record *rec = d->commandBinds.at(i);
if(rec->geti("id") == id)
if(rec->geti(VAR_ID) == id)
{
d->commandBinds.removeAt(i);
delete rec;
Expand All @@ -423,7 +429,7 @@ bool BindContext::deleteBinding(int id)
for(int k = 0; k < d->impulseBinds[i].count(); ++k)
{
Record *rec = d->impulseBinds[i].at(k);
if(rec->geti("id") == id)
if(rec->geti(VAR_ID) == id)
{
d->impulseBinds[i].removeAt(k);
delete rec;
Expand Down
8 changes: 5 additions & 3 deletions doomsday/apps/client/src/world/p_mobj.cpp
View file
Expand Up @@ -68,6 +68,8 @@

using namespace de;

static String const VAR_MATERIAL("material");

static mobj_t *unusedMobjs;

/*
Expand Down Expand Up @@ -477,7 +479,7 @@ void Mobj_GenerateLumobjs(mobj_t *mo)
if(!sprite.hasView(0)) return;

// Lookup the Material for the Sprite and prepare the animator.
Material *mat = resSys().materialPtr(de::Uri(sprite.view(0).gets("material"), RC_NULL));
Material *mat = resSys().materialPtr(de::Uri(sprite.view(0).gets(VAR_MATERIAL), RC_NULL));
if(!mat) return;
MaterialAnimator &matAnimator = mat->getAnimator(Rend_SpriteMaterialSpec());
matAnimator.prepare(); // Ensure we have up-to-date info.
Expand Down Expand Up @@ -613,7 +615,7 @@ dfloat Mobj_ShadowStrength(mobj_t const &mob)
ambientLightLevel = cluster.lightSourceIntensity();
}
Rend_ApplyLightAdaptation(ambientLightLevel);

// Sprites have their own shadow strength factor.
dfloat strength = .65f; ///< Default.
if(!::useModels || !Mobj_ModelDef(mob))
Expand All @@ -623,7 +625,7 @@ dfloat Mobj_ShadowStrength(mobj_t const &mob)
defn::Sprite sprite(*spriteRec);
if(sprite.hasView(0)) // Always use the front view for lighting.
{
if(Material *mat = resSys().materialPtr(de::Uri(sprite.view(0).gets("material"), RC_NULL)))
if(Material *mat = resSys().materialPtr(de::Uri(sprite.view(0).gets(VAR_MATERIAL), RC_NULL)))
{
MaterialAnimator &matAnimator = mat->getAnimator(Rend_SpriteMaterialSpec());
matAnimator.prepare(); // Ensure we have up-to-date info.
Expand Down
2 changes: 1 addition & 1 deletion doomsday/sdk/libcore/src/data/pathtree.cpp
View file
Expand Up @@ -136,7 +136,7 @@ struct PathTree::Instance
*/
PathTree::Node *buildNodesForPath(Path const &path)
{
bool const hasLeaf = !path.toStringRef().endsWith("/");
bool const hasLeaf = !path.toStringRef().endsWith(QStringLiteral("/"));

PathTree::Node *node = 0, *parent = &rootNode;
for(int i = 0; i < path.segmentCount() - (hasLeaf? 1 : 0); ++i)
Expand Down
13 changes: 11 additions & 2 deletions doomsday/sdk/libcore/src/data/pathtreenode.cpp
View file
Expand Up @@ -36,6 +36,8 @@ DENG2_PIMPL_NOREF(PathTree::Node)
/// Unique identifier for the path fragment this node represents,
/// in the owning PathTree.
PathTree::SegmentId segmentId;

String const *segmentText = nullptr; // owned by the PathTree

Instance(PathTree &_tree, bool isLeaf, PathTree::SegmentId _segmentId,
PathTree::Node *_parent)
Expand Down Expand Up @@ -120,7 +122,14 @@ void PathTree::Node::removeChild(PathTree::Node &node)

String const &PathTree::Node::name() const
{
return tree().segmentName(d->segmentId);
if(!d->segmentText)
{
// Cache the string, because PathTree::segmentName() locks the tree and that has
// performance implications. The segment text string will not change while the
// node exists.
d->segmentText = &tree().segmentName(d->segmentId);
}
return *d->segmentText;
}

Path::hash_type PathTree::Node::hash() const
Expand Down Expand Up @@ -183,7 +192,7 @@ int PathTree::Node::comparePath(de::Path const &searchPattern, ComparisonFlags f
PathTree::Node const *node = this;
for(int i = 0; i < pathNodeCount; ++i)
{
bool const snameIsWild = !snode->toStringRef().compare("*");
bool const snameIsWild = !snode->toStringRef().compare(QStringLiteral("*"));
if(!snameIsWild)
{
// If the hashes don't match it can't possibly be this.
Expand Down

0 comments on commit 1b6948f

Please sign in to comment.