Skip to content

Commit

Permalink
Remove unnecessary write lock when rewriting a query term. (#1592)
Browse files Browse the repository at this point in the history 
Remove the chance of a deadlock when spawning a new query.
  • Loading branch information
@samscott89
samscott89 authored Jun 15, 2022
1 parent dd2482b commit b88bef8
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 35 deletions.
4 changes: 2 additions & 2 deletions polar-core/src/polar.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,8 +209,8 @@ impl Polar {
pub fn new_query_from_term(&self, mut term: Term, trace: bool) -> Query {
use crate::vm::{Goal, PolarVirtualMachine};
{
let mut kb = self.kb.write().unwrap();
term = rewrite_term(term, &mut kb);
let kb = self.kb.read().unwrap();
term = rewrite_term(term, &kb);
}
let query = Goal::Query { term: term.clone() };
let vm =
Expand Down
66 changes: 33 additions & 33 deletions polar-core/src/rewrites.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,13 +263,13 @@ pub fn unwrap_and(term: &Term) -> TermList {
}

/// Rewrite a term.
pub fn rewrite_term(term: Term, kb: &mut KnowledgeBase) -> Term {
pub fn rewrite_term(term: Term, kb: &KnowledgeBase) -> Term {
let mut fld = Rewriter::new(kb);
fld.fold_term(term)
}

/// Rewrite a rule.
pub fn rewrite_rule(rule: Rule, kb: &mut KnowledgeBase) -> Rule {
pub fn rewrite_rule(rule: Rule, kb: &KnowledgeBase) -> Rule {
let mut fld = Rewriter::new(kb);
fld.fold_rule(rule)
}
Expand All @@ -289,23 +289,23 @@ mod tests {

#[test]
fn rewrite_anonymous_vars() {
let mut kb = KnowledgeBase::new();
let kb = KnowledgeBase::new();
let query = parse_query("[1, 2, 3] = [_, _, _]");
assert_eq!(
rewrite_term(query, &mut kb).to_string(),
rewrite_term(query, &kb).to_string(),
"[1, 2, 3] = [_1, _2, _3]"
);
}

#[test]
fn rewrite_rules() {
let mut kb = KnowledgeBase::new();
let kb = KnowledgeBase::new();
let rules = parse_rules("f(a.b);");
let rule = rules[0].clone();
assert_eq!(rule.to_string(), "f(a.b);");

// First rewrite
let rule = rewrite_rule(rule, &mut kb);
let rule = rewrite_rule(rule, &kb);
assert_eq!(rule.to_string(), "f(_value_1) if a.b = _value_1;");

// Check we can parse the rules back again
Expand All @@ -317,7 +317,7 @@ mod tests {
let rules = parse_rules("f(a.b.c);");
let rule = rules[0].clone();
assert_eq!(rule.to_string(), "f(a.b.c);");
let rule = rewrite_rule(rule, &mut kb);
let rule = rewrite_rule(rule, &kb);
assert_eq!(
rule.to_string(),
"f(_value_3) if a.b = _value_2 and _value_2.c = _value_3;"
Expand All @@ -326,15 +326,15 @@ mod tests {

#[test]
fn rewrite_forall_rhs_dots() {
let mut kb = KnowledgeBase::new();
let kb = KnowledgeBase::new();
let rules = parse_rules("foo(z, y) if forall(x in y, x.n < z);");
let rule = rewrite_rule(rules[0].clone(), &mut kb);
let rule = rewrite_rule(rules[0].clone(), &kb);
assert_eq!(
rule.to_string(),
"foo(z, y) if forall(x in y, _value_1 < z and x.n = _value_1);"
);

let query = rewrite_term(parse_query("forall(x in y, x.n < z)"), &mut kb);
let query = rewrite_term(parse_query("forall(x in y, x.n < z)"), &kb);
assert_eq!(
query.to_string(),
"forall(x in y, _value_2 < z and x.n = _value_2)"
Expand All @@ -343,13 +343,13 @@ mod tests {

#[test]
fn rewrite_nested_lookups() {
let mut kb = KnowledgeBase::new();
let kb = KnowledgeBase::new();

// Lookups with args
let rules = parse_rules("f(a, c) if a.b(c);");
let rule = rules[0].clone();
assert_eq!(rule.to_string(), "f(a, c) if a.b(c);");
let rule = rewrite_rule(rule, &mut kb);
let rule = rewrite_rule(rule, &kb);
assert_eq!(
rule.to_string(),
"f(a, c) if a.b(c) = _value_1 and _value_1;"
Expand All @@ -359,7 +359,7 @@ mod tests {
let rules = parse_rules("f(a, c, e) if a.b(c.d(e.f()));");
let rule = rules[0].clone();
assert_eq!(rule.to_string(), "f(a, c, e) if a.b(c.d(e.f()));");
let rule = rewrite_rule(rule, &mut kb);
let rule = rewrite_rule(rule, &kb);
assert_eq!(
rule.to_string(),
"f(a, c, e) if e.f() = _value_2 and c.d(_value_2) = _value_3 and a.b(_value_3) = _value_4 and _value_4;"
Expand All @@ -368,110 +368,110 @@ mod tests {

#[test]
fn rewrite_terms() {
let mut kb = KnowledgeBase::new();
let kb = KnowledgeBase::new();
let term = parse_query("x and a.b");
assert_eq!(term.to_string(), "x and a.b");
assert_eq!(
rewrite_term(term, &mut kb).to_string(),
rewrite_term(term, &kb).to_string(),
"x and a.b = _value_1 and _value_1"
);

let query = parse_query("f(a.b().c)");
assert_eq!(query.to_string(), "f(a.b().c)");
assert_eq!(
rewrite_term(query, &mut kb).to_string(),
rewrite_term(query, &kb).to_string(),
"a.b() = _value_2 and _value_2.c = _value_3 and f(_value_3)"
);

let term = parse_query("a.b = 1");
assert_eq!(
rewrite_term(term, &mut kb).to_string(),
rewrite_term(term, &kb).to_string(),
"a.b = _value_4 and _value_4 = 1"
);
let term = parse_query("{x: 1}.x = 1");
assert_eq!(term.to_string(), "{x: 1}.x = 1");
assert_eq!(
rewrite_term(term, &mut kb).to_string(),
rewrite_term(term, &kb).to_string(),
"{x: 1}.x = _value_5 and _value_5 = 1"
);
}

#[test]
fn rewrite_expressions() {
let mut kb = KnowledgeBase::new();
let kb = KnowledgeBase::new();

let term = parse_query("0 - 0 = 0");
assert_eq!(term.to_string(), "0 - 0 = 0");
assert_eq!(
rewrite_term(term, &mut kb).to_string(),
rewrite_term(term, &kb).to_string(),
"0 - 0 = _op_1 and _op_1 = 0"
);

let rules = parse_rules("sum(a, b, a + b);");
let rule = rules[0].clone();
assert_eq!(rule.to_string(), "sum(a, b, a + b);");
let rule = rewrite_rule(rule, &mut kb);
let rule = rewrite_rule(rule, &kb);
assert_eq!(rule.to_string(), "sum(a, b, _op_2) if a + b = _op_2;");

let rules = parse_rules("fib(n, a+b) if fib(n-1, a) and fib(n-2, b);");
let rule = rules[0].clone();
let rule = rewrite_rule(rule, &mut kb);
let rule = rewrite_rule(rule, &kb);
assert_eq!(rule.to_string(), "fib(n, _op_5) if n - 1 = _op_3 and fib(_op_3, a) and n - 2 = _op_4 and fib(_op_4, b) and a + b = _op_5;");
}

#[test]
fn rewrite_nested_literal() {
let mut kb = KnowledgeBase::new();
let kb = KnowledgeBase::new();
let term = parse_query("new Foo(x: bar.y)");
assert_eq!(term.to_string(), "new Foo(x: bar.y)");
assert_eq!(
rewrite_term(term, &mut kb).to_string(),
rewrite_term(term, &kb).to_string(),
"bar.y = _value_1 and new (Foo(x: _value_1), _instance_2) and _instance_2"
);

let term = parse_query("f(new Foo(x: bar.y))");
assert_eq!(term.to_string(), "f(new Foo(x: bar.y))");
assert_eq!(
rewrite_term(term, &mut kb).to_string(),
rewrite_term(term, &kb).to_string(),
"bar.y = _value_3 and new (Foo(x: _value_3), _instance_4) and f(_instance_4)"
);
}

#[test]
fn rewrite_class_constructor() {
let mut kb = KnowledgeBase::new();
let kb = KnowledgeBase::new();
let term = parse_query("new Foo(a: 1, b: 2)");
assert_eq!(term.to_string(), "new Foo(a: 1, b: 2)");

// @ means external constructor
assert_eq!(
rewrite_term(term, &mut kb).to_string(),
rewrite_term(term, &kb).to_string(),
"new (Foo(a: 1, b: 2), _instance_1) and _instance_1"
);
}

#[test]
fn rewrite_nested_class_constructor() {
let mut kb = KnowledgeBase::new();
let kb = KnowledgeBase::new();
let term = parse_query("new Foo(a: 1, b: new Foo(a: 2, b: 3))");
assert_eq!(term.to_string(), "new Foo(a: 1, b: new Foo(a: 2, b: 3))");

assert_eq!(
rewrite_term(term, &mut kb).to_string(),
rewrite_term(term, &kb).to_string(),
"new (Foo(a: 2, b: 3), _instance_1) and \
new (Foo(a: 1, b: _instance_1), _instance_2) and _instance_2"
);
}

#[test]
fn rewrite_rules_constructor() {
let mut kb = KnowledgeBase::new();
let kb = KnowledgeBase::new();
let mut rules = parse_rules("rule_test(new Foo(a: 1, b: 2));");
let rule = rules.pop().unwrap();
assert_eq!(rule.to_string(), "rule_test(new Foo(a: 1, b: 2));");
assert!(rules.is_empty());

let rule = rewrite_rule(rule, &mut kb);
let rule = rewrite_rule(rule, &kb);
assert_eq!(
rule.to_string(),
"rule_test(_instance_1) if new (Foo(a: 1, b: 2), _instance_1);"
Expand All @@ -480,12 +480,12 @@ mod tests {

#[test]
fn rewrite_not_with_lookup() {
let mut kb = KnowledgeBase::new();
let kb = KnowledgeBase::new();
let term = parse_query("not foo.x = 1");
assert_eq!(term.to_string(), "not foo.x = 1");

pretty_assertions::assert_eq!(
rewrite_term(term, &mut kb).to_string(),
rewrite_term(term, &kb).to_string(),
"not (_value_1 = 1 and foo.x = _value_1)"
)
}
Expand Down

1 comment on commit b88bef8

@github-actions
Copy link

Choose a reason for hiding this comment

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

Rust Benchmark

Benchmark suite Current: b88bef8 Previous: dd2482b Ratio
rust_get_attribute 45181 ns/iter (± 1977) 57447 ns/iter (± 5931) 0.79
n_plus_one/100 2244005 ns/iter (± 40624) 2687505 ns/iter (± 149185) 0.83
n_plus_one/500 10801228 ns/iter (± 186140) 12865943 ns/iter (± 545244) 0.84
n_plus_one/1000 18958887 ns/iter (± 83322) 25725356 ns/iter (± 1024120) 0.74
unify_once 884 ns/iter (± 734) 1163 ns/iter (± 271) 0.76
unify_twice 2593 ns/iter (± 71) 2913 ns/iter (± 204) 0.89
many_rules 54584 ns/iter (± 1062) 75457 ns/iter (± 8110) 0.72
fib/5 462277 ns/iter (± 8439) 632422 ns/iter (± 47390) 0.73
prime/3 15619 ns/iter (± 567) 21655 ns/iter (± 1858) 0.72
prime/23 15640 ns/iter (± 612) 21985 ns/iter (± 2143) 0.71
prime/43 15599 ns/iter (± 588) 22189 ns/iter (± 2297) 0.70
prime/83 17673 ns/iter (± 636) 22316 ns/iter (± 1868) 0.79
prime/255 16244 ns/iter (± 568) 20032 ns/iter (± 1562) 0.81
indexed/100 5901 ns/iter (± 595) 7519 ns/iter (± 1630) 0.78
indexed/500 7921 ns/iter (± 1883) 9641 ns/iter (± 3523) 0.82
indexed/1000 9233 ns/iter (± 552) 12403 ns/iter (± 2832) 0.74
indexed/10000 25375 ns/iter (± 2109) 25200 ns/iter (± 6152) 1.01
not 5826 ns/iter (± 150) 7757 ns/iter (± 1615) 0.75
double_not 12202 ns/iter (± 236) 15698 ns/iter (± 1229) 0.78
De_Morgan_not 7779 ns/iter (± 193) 9585 ns/iter (± 665) 0.81
load_policy 897830 ns/iter (± 5469) 1142125 ns/iter (± 91626) 0.79
partial_and/1 31129 ns/iter (± 1362) 37408 ns/iter (± 3289) 0.83
partial_and/5 110535 ns/iter (± 2927) 129038 ns/iter (± 9716) 0.86
partial_and/10 210835 ns/iter (± 4819) 250993 ns/iter (± 12386) 0.84
partial_and/20 427580 ns/iter (± 4995) 522288 ns/iter (± 31618) 0.82
partial_and/40 910931 ns/iter (± 10192) 1076474 ns/iter (± 66304) 0.85
partial_and/80 1818056 ns/iter (± 8107) 2425668 ns/iter (± 133156) 0.75
partial_and/100 2405625 ns/iter (± 12297) 3077186 ns/iter (± 196769) 0.78
partial_rule_depth/1 101322 ns/iter (± 4105) 110211 ns/iter (± 12499) 0.92
partial_rule_depth/5 333195 ns/iter (± 5739) 391814 ns/iter (± 276869) 0.85
partial_rule_depth/10 728620 ns/iter (± 11367) 856387 ns/iter (± 48770) 0.85
partial_rule_depth/20 2043945 ns/iter (± 13964) 2506029 ns/iter (± 214064) 0.82
partial_rule_depth/40 7516360 ns/iter (± 91544) 9154942 ns/iter (± 934640) 0.82
partial_rule_depth/80 43957871 ns/iter (± 453474) 64287195 ns/iter (± 7174375) 0.68
partial_rule_depth/100 80516263 ns/iter (± 546614) 101889330 ns/iter (± 5935513) 0.79

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.