topdown/sets_bench_test: Add intersection builtin benchmarks.
#5000
Conversation
philipaconrad
force-pushed
the
set-intersection-logic-hoist
branch
2 times, most recently
from
fdfc258
to
5f7f55b
Compare
|
Local benchmarks are not showing much, if any, improvement over doing set intersection in the naive pairwise way. I suspect this is due to the intersection algorithm guaranteeing equal or smaller sets as iteration progresses, eliminating most of the win potential observed for I'll play around with one or two more ideas in this space before dropping this PR though. 😃 |
philipaconrad
force-pushed
the
set-intersection-logic-hoist
branch
from
5f7f55b
to
fb8cb7a
Compare
topdown/sets.go
Outdated
| return nil, err | ||
| } | ||
| presentInAll = make(map[*ast.Term]struct{}, first.Len()) | ||
| first.Iter(func(x *ast.Term) error { |
There was a problem hiding this comment.
[nit] Iter(func(*ast.Term) error) -> Foreach(func(*ast.Term))
topdown/sets.go
Outdated
| // Add any surviving Terms to the output set. | ||
| for k := range presentInAll { | ||
| result.Add(k) | ||
| } | ||
| return result, err |
There was a problem hiding this comment.
💭 err? Let's put an if err != nil { ... } right after the err = and return result, nil here
philipaconrad
force-pushed
the
set-intersection-logic-hoist
branch
from
fb8cb7a
to
c43119c
Compare
|
I've tinkered around with a few different approaches for trying to do set intersection more efficiently, and I can't beat the original implementation, at least with the benchmark that I have (which generates N identical sets, the worst case). The best case (no keys match across all sets), and the average case (a few keys match across all sets) are not benchmarked currently, and those might be worth exploring. A solution that dramatically improves average-case performance with only a minor worst-case penalty might be valuable. |
This commit adds tests for the `intersection` Set builtin, and cleans up the existing tests with a new data generator function. Signed-off-by: Philip Conrad <philipaconrad@gmail.com>
philipaconrad
force-pushed
the
set-intersection-logic-hoist
branch
from
c531d15
to
438e5df
Compare
philipaconrad
changed the title
intersection builtin benchmarks.
|
I couldn't get a meaningful speedup, even with relatively pathological input sets-of-sets. I'm throwing in the towel on this for now, and have changed the PR title to reflect a massive cutting-back in scope. This PR now is limited to adding 2x new benchmarks, and cleaning up how data is generated for both the |
This PR is an experiment to try to improve performance for the set
intersectionbuiltin, inspired by work done in #4980 on the setunionbuiltin.The original logic for the builtin did pairwise
Set.Intersectioncalls between the input sets, theoretically resulting in some wasted intermediate sets. Ideally, we'd like to minimize wasted allocations, and get a faster and more efficient solution.