Skip to content

Commit

Permalink
WebRequestRulesRegistry: Get rid of GlobalID as a map key + the local…
Browse files Browse the repository at this point in the history 
… map in AddRulesImpl.

Improving speed for adding new rules:
 * use vector instead of map where we know the size in advance (vector can pre-allocate, map cannot), and don't care too much about searching by key
 * use a map of maps for storing WR rules --> top level by extension ID, second by rules ID (all the time we care about a fixed extension ID, so this makes things easier)

In my benchmark, this reduced the time spent on AddRules by approx 8%.

BUG=236368
R=battre@chromium.org

Review URL: https://codereview.chromium.org/15018010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@198880 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
vabr@chromium.org committed May 8, 2013
1 parent a4ccba0 commit b467810
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 39 deletions.
101 changes: 64 additions & 37 deletions chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ std::list<LinkedPtrEventResponseDelta> WebRequestRulesRegistry::CreateDeltas(
const WebRequestRule::Priority priority_of_rule = i->first;
const WebRequestRule::GlobalRuleId& rule_id = i->second;
const ExtensionId& extension_id = rule_id.first;
const WebRequestRule* rule = webrequest_rules_[rule_id].get();
const WebRequestRule* rule =
webrequest_rules_[rule_id.first][rule_id.second].get();
CHECK(rule);

// Skip rule if a previous rule of this extension instructed to ignore
Expand Down Expand Up @@ -155,18 +156,24 @@ std::list<LinkedPtrEventResponseDelta> WebRequestRulesRegistry::CreateDeltas(
std::string WebRequestRulesRegistry::AddRulesImpl(
const std::string& extension_id,
const std::vector<linked_ptr<RulesRegistry::Rule> >& rules) {
typedef std::pair<WebRequestRule::RuleId, linked_ptr<WebRequestRule> >
IdRulePair;
typedef std::vector<IdRulePair> RulesVector;

base::Time extension_installation_time =
GetExtensionInstallationTime(extension_id);

std::string error;
RulesMap new_webrequest_rules;
RulesVector new_webrequest_rules;
new_webrequest_rules.reserve(rules.size());
const Extension* extension =
extension_info_map_->extensions().GetByID(extension_id);
RulesMap& registered_rules = webrequest_rules_[extension_id];

for (std::vector<linked_ptr<RulesRegistry::Rule> >::const_iterator rule =
rules.begin(); rule != rules.end(); ++rule) {
WebRequestRule::GlobalRuleId rule_id(extension_id, *(*rule)->id);
DCHECK(webrequest_rules_.find(rule_id) == webrequest_rules_.end());
const WebRequestRule::RuleId& rule_id(*(*rule)->id);
DCHECK(registered_rules.find(rule_id) == registered_rules.end());

scoped_ptr<WebRequestRule> webrequest_rule(WebRequestRule::Create(
url_matcher_.condition_factory(),
Expand All @@ -179,7 +186,8 @@ std::string WebRequestRulesRegistry::AddRulesImpl(
break;
}

new_webrequest_rules[rule_id] = make_linked_ptr(webrequest_rule.release());
new_webrequest_rules.push_back(
IdRulePair(rule_id, make_linked_ptr(webrequest_rule.release())));
}

if (!error.empty()) {
Expand All @@ -189,11 +197,11 @@ std::string WebRequestRulesRegistry::AddRulesImpl(
}

// Wohoo, everything worked fine.
webrequest_rules_.insert(new_webrequest_rules.begin(),
new_webrequest_rules.end());
registered_rules.insert(new_webrequest_rules.begin(),
new_webrequest_rules.end());

// Create the triggers.
for (RulesMap::iterator i = new_webrequest_rules.begin();
for (RulesVector::const_iterator i = new_webrequest_rules.begin();
i != new_webrequest_rules.end(); ++i) {
URLMatcherConditionSet::Vector url_condition_sets;
const WebRequestConditionSet& conditions = i->second->conditions();
Expand All @@ -207,7 +215,7 @@ std::string WebRequestRulesRegistry::AddRulesImpl(
// Register url patterns in |url_matcher_| and
// |rules_with_untriggered_conditions_|.
URLMatcherConditionSet::Vector all_new_condition_sets;
for (RulesMap::iterator i = new_webrequest_rules.begin();
for (RulesVector::const_iterator i = new_webrequest_rules.begin();
i != new_webrequest_rules.end(); ++i) {
i->second->conditions().GetURLMatcherConditionSets(&all_new_condition_sets);
if (i->second->conditions().HasConditionsWithoutUrls())
Expand All @@ -217,7 +225,7 @@ std::string WebRequestRulesRegistry::AddRulesImpl(

ClearCacheOnNavigation();

if (profile_id_ && !webrequest_rules_.empty()) {
if (profile_id_ && !registered_rules.empty()) {
BrowserThread::PostTask(
BrowserThread::UI, FROM_HERE,
base::Bind(&extension_web_request_api_helpers::NotifyWebRequestAPIUsed,
Expand All @@ -232,31 +240,24 @@ std::string WebRequestRulesRegistry::RemoveRulesImpl(
const std::vector<std::string>& rule_identifiers) {
// URLMatcherConditionSet IDs that can be removed from URLMatcher.
std::vector<URLMatcherConditionSet::ID> remove_from_url_matcher;
RulesMap& registered_rules = webrequest_rules_[extension_id];

for (std::vector<std::string>::const_iterator i = rule_identifiers.begin();
i != rule_identifiers.end(); ++i) {
WebRequestRule::GlobalRuleId rule_id(extension_id, *i);

// Skip unknown rules.
RulesMap::iterator webrequest_rules_entry = webrequest_rules_.find(rule_id);
if (webrequest_rules_entry == webrequest_rules_.end())
RulesMap::iterator webrequest_rules_entry = registered_rules.find(*i);
if (webrequest_rules_entry == registered_rules.end())
continue;

// Remove all triggers but collect their IDs.
URLMatcherConditionSet::Vector condition_sets;
WebRequestRule* rule = webrequest_rules_entry->second.get();
rule->conditions().GetURLMatcherConditionSets(&condition_sets);
for (URLMatcherConditionSet::Vector::iterator j = condition_sets.begin();
j != condition_sets.end(); ++j) {
remove_from_url_matcher.push_back((*j)->id());
rule_triggers_.erase((*j)->id());
}

rules_with_untriggered_conditions_.erase(rule);
CleanUpAfterRule(webrequest_rules_entry->second.get(),
&remove_from_url_matcher);

// Removes the owning references to (and thus deletes) the rule.
webrequest_rules_.erase(webrequest_rules_entry);
registered_rules.erase(webrequest_rules_entry);
}
if (registered_rules.empty())
webrequest_rules_.erase(extension_id);

// Clear URLMatcher based on condition_set_ids that are not needed any more.
url_matcher_.RemoveConditionSets(remove_from_url_matcher);
Expand All @@ -268,23 +269,49 @@ std::string WebRequestRulesRegistry::RemoveRulesImpl(

std::string WebRequestRulesRegistry::RemoveAllRulesImpl(
const std::string& extension_id) {
// Search all identifiers of rules that belong to extension |extension_id|.
std::vector<std::string> rule_identifiers;
for (RulesMap::iterator i = webrequest_rules_.begin();
i != webrequest_rules_.end(); ++i) {
const WebRequestRule::GlobalRuleId& global_rule_id = i->first;
if (global_rule_id.first == extension_id)
rule_identifiers.push_back(global_rule_id.second);
// First we get out all URLMatcherConditionSets and remove the rule references
// from |rules_with_untriggered_conditions_|.
std::vector<URLMatcherConditionSet::ID> remove_from_url_matcher;
for (RulesMap::const_iterator it = webrequest_rules_[extension_id].begin();
it != webrequest_rules_[extension_id].end();
++it) {
CleanUpAfterRule(it->second.get(), &remove_from_url_matcher);
}
url_matcher_.RemoveConditionSets(remove_from_url_matcher);

webrequest_rules_.erase(extension_id);
ClearCacheOnNavigation();
return std::string();
}

// No need to call ClearCacheOnNavigation() here because RemoveRulesImpl
// takes care of that.
return RemoveRulesImpl(extension_id, rule_identifiers);
void WebRequestRulesRegistry::CleanUpAfterRule(
const WebRequestRule* rule,
std::vector<URLMatcherConditionSet::ID>* remove_from_url_matcher) {
URLMatcherConditionSet::Vector condition_sets;
rule->conditions().GetURLMatcherConditionSets(&condition_sets);
for (URLMatcherConditionSet::Vector::iterator j = condition_sets.begin();
j != condition_sets.end();
++j) {
remove_from_url_matcher->push_back((*j)->id());
rule_triggers_.erase((*j)->id());
}
rules_with_untriggered_conditions_.erase(rule);
}

bool WebRequestRulesRegistry::IsEmpty() const {
return rule_triggers_.empty() && webrequest_rules_.empty() &&
url_matcher_.IsEmpty();
// Easy first.
if (!rule_triggers_.empty() && url_matcher_.IsEmpty())
return false;

// Now all the registered rules for each extensions.
for (std::map<WebRequestRule::ExtensionId, RulesMap>::const_iterator it =
webrequest_rules_.begin();
it != webrequest_rules_.end();
++it) {
if (!it->second.empty())
return false;
}
return true;
}

WebRequestRulesRegistry::~WebRequestRulesRegistry() {}
Expand Down
14 changes: 12 additions & 2 deletions chrome/browser/extensions/api/declarative_webrequest/webrequest_rules_registry.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ class WebRequestRulesRegistry : public RulesRegistryWithCache {
HostPermissionsChecker);

typedef std::map<URLMatcherConditionSet::ID, WebRequestRule*> RuleTriggers;
typedef std::map<WebRequestRule::GlobalRuleId, linked_ptr<WebRequestRule> >
typedef std::map<WebRequestRule::RuleId, linked_ptr<WebRequestRule> >
RulesMap;
typedef std::set<URLMatcherConditionSet::ID> URLMatches;
typedef std::set<const WebRequestRule*> RuleSet;
Expand All @@ -152,6 +152,16 @@ class WebRequestRulesRegistry : public RulesRegistryWithCache {
const WebRequestActionSet* actions,
std::string* error);

// Helper for RemoveRulesImpl and RemoveAllRulesImpl. Call this before
// deleting |rule| from one of the maps in |webrequest_rules_|. It will erase
// the rule from |rule_triggers_| and |rules_with_untriggered_conditions_|,
// and add every of the rule's URLMatcherConditionSet to
// |remove_from_url_matcher|, so that the caller can remove them from the
// matcher later.
void CleanUpAfterRule(
const WebRequestRule* rule,
std::vector<URLMatcherConditionSet::ID>* remove_from_url_matcher);

// This is a helper function to GetMatches. Rules triggered by |url_matches|
// get added to |result| if one of their conditions is fulfilled.
// |request_data| gets passed to IsFulfilled of the rules' condition sets.
Expand All @@ -168,7 +178,7 @@ class WebRequestRulesRegistry : public RulesRegistryWithCache {
// separately.
std::set<const WebRequestRule*> rules_with_untriggered_conditions_;

RulesMap webrequest_rules_;
std::map<WebRequestRule::ExtensionId, RulesMap> webrequest_rules_;

URLMatcher url_matcher_;

Expand Down

0 comments on commit b467810

Please sign in to comment.