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

Purge intermediate relations, but not input/output #2286

Merged

Conversation

adamjseitz
Copy link
Contributor

In #2066, a new option was added to the C++ interface to allow purging intermediate relations. However, the current implementation purges input and output relations, which makes it impractical to use with performIO disabled (since the output relations would be purged before there is an opportunity for user code to read the output relation). By excluding input and output relations from those that are purged, the C++ interface can be used with pruneImdtRels enabled but performIO disabled.

@aeflores
Copy link
Contributor

@adamjseitz maybe we should add a test? I see there are some C++ tests in https://github.com/souffle-lang/souffle/tree/master/tests/interface

@b-scholz
Copy link
Member

Can you run clang-format? I.e.,

diff --git a/src/synthesiser/Synthesiser.cpp b/src/synthesiser/Synthesiser.cpp
[11](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:12)
index 58b7ead..487f340 100644
[12](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:13)
--- a/src/synthesiser/Synthesiser.cpp
[13](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:14)
+++ b/src/synthesiser/Synthesiser.cpp
[14](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:15)
@@ -466,7 +466,8 @@void Synthesiser::emitCode(std::ostream& out, const Statement& stmt) {
[15](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:16)
[16](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:17)
            auto Relation = synthesiser.lookup(clear.getRelation());
[17](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:18)
            bool isIntermediate = !contains(synthesiser.loadRelations, Relation->getName()) &&
[18](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:19)
-                !contains(synthesiser.storeRelations, Relation->getName()) && !Relation->isTemp();
[19](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:20)
+                                  !contains(synthesiser.storeRelations, Relation->getName()) &&
[20](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:21)
+                                  !Relation->isTemp();
[21](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:22)
[22](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:23)
            if (isIntermediate) {
[23](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:24)
                out << "if (pruneImdtRels) ";
[24](https://github.com/souffle-lang/souffle/runs/6947156929?check_suite_focus=true#step:3:25)

@codecov
Copy link

codecov bot commented Jun 18, 2022

Codecov Report

Merging #2286 (dc76fd6) into master (8979bdf) will increase coverage by 0.24%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2286      +/-   ##
==========================================
+ Coverage   77.17%   77.41%   +0.24%     
==========================================
  Files         458      461       +3     
  Lines       29159    28998     -161     
==========================================
- Hits        22504    22450      -54     
+ Misses       6655     6548     -107     
Impacted Files Coverage Δ
src/synthesiser/Synthesiser.h 100.00% <ø> (ø)
src/synthesiser/Synthesiser.cpp 83.85% <100.00%> (+0.01%) ⬆️
src/include/souffle/profile/ProfileEvent.h 92.85% <0.00%> (-5.11%) ⬇️
src/ast2ram/seminaive/ClauseTranslator.cpp 97.84% <0.00%> (-0.12%) ⬇️
src/ast/utility/SipsMetric.cpp
src/ast/utility/SipsMetric.h
src/ast2ram/utility/SipGraph.h 100.00% <0.00%> (ø)
src/ast2ram/utility/SipsMetric.cpp 55.19% <0.00%> (ø)
src/include/souffle/utility/SubsetCache.h 88.23% <0.00%> (ø)
src/ast2ram/utility/SipsMetric.h 30.00% <0.00%> (ø)
... and 5 more

@adamjseitz adamjseitz force-pushed the aseitz-purge-intermediate-relations branch from b97c28c to dc76fd6 Compare June 27, 2022 15:44
@adamjseitz
Copy link
Contributor Author

@adamjseitz maybe we should add a test? I see there are some C++ tests in https://github.com/souffle-lang/souffle/tree/master/tests/interface

I modified the existing insert_print test to utilize pruneImdtRels to verify that it's working - it didn't seem worth adding a near-duplicate test.

Can you run clang-format? I.e.,

Sorry! I think the formatting is fixed now.

Copy link
Member

@b-scholz b-scholz left a comment

Choose a reason for hiding this comment

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

Thanks for the refactoring!

@b-scholz b-scholz merged commit 045b15a into souffle-lang:master Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants