zaks.anna added a comment.
Thanks for working on this!
The main unfinished task here is to investigate ways of reducing the
performance hit. See more info below.
> The patch was tested on Android open-source platform source code.
Just to double check, have you compare the pre and after results and all of
them but this one issue are the same?
> Performance has slightly degraded (~5%) - hmm
5% is a considerable regression :(
================
Comment at: lib/StaticAnalyzer/Core/Environment.cpp:180
@@ -179,3 +177,1 @@
- for (; SI != SE; ++SI)
- SymReaper.maybeDead(*SI);
}
----------------
We are removing this because the maybeDead is no longer used, correct?
================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:390
@@ -389,3 @@
-
- } else {
- // Call checkers with the non-cleaned state so that they could query the
----------------
This removes an optimization to address the following issue:
"removeDeadBindings is not run right after the last reference to the object is
lost, which leads to imprecise error reports and failure to report the leak in
some cases. It's because of how hasDeadSymbols() is implemented. That problem
is solved if we disable the optimization, but I do not know how that effects
performance. Maybe we can come up with something more clever.
"
I suspect the removal of this optimization causes the performance regression.
In the patch I sent to the list, this was just a hack to demonstrate what
causes the issue. I am not sure we should not just remove the optimization...
The best proposal I have is to trigger remove dead bindings at the end of every
basic block. This would degrade diagnostics (you will see leaks only at the end
of the basic block), but should give us performance back or even improve
performance.
Artem and Devin, WDYT?
Artem, can you experiment with this and investigate if the diagnostics become
much worse? Maybe send a couple of examples? I suggest we implement this mode
behind a flag as a separate patch.
================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:2452
@@ -2451,3 +2440,1 @@
- SymReaper.maybeDead(*SI);
- }
}
----------------
Looks like we are saying that we should no longer add to maybeDead because it's
not used.
http://reviews.llvm.org/D18860
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
