JS: Small performance improvements. #3442
Conversation
erik-krogh
added
JS
Awaiting evaluation
|
Neat! I'd say these changes looked sensible even without the performance boost 👍 |
| @@ -71,8 +71,7 @@ module InclusionTest { | |||
| count(this.getACallee()) = 1 and | |||
| count(callee.getAReturnedExpr()) = 1 and | |||
| not this.isImprecise() and | |||
| inner.getContainerNode().getALocalSource().getEnclosingExpr() = callee.getAParameter() and | |||
| inner.getContainedNode().getALocalSource().getEnclosingExpr() = callee.getAParameter() | |||
| inner.getContainerNode().getALocalSource() = DataFlow::parameterNode(callee.getAParameter()) | |||
There was a problem hiding this comment.
I do not understand this change.
The previous version used both getContainedNode and getContainerNode.
There was a problem hiding this comment.
You are right, that was a mistake from my end, I didn't notice the Contained/Container.
Everything ends up working anyway, as getContainedNode is called all the places the InclusionTest is used. And thus the getContainedNode constraint still enforced, just not in the CharPred.
I fixed it anyway (and refactored two other getEnclosingExpr()).
… and refactor two getEnclosingExpr()
|
Great. We are just waiting for the evaluation then. |
|
A big-apps evaluation looks very good. All evaluations were without the latest change to I think this is ready to land. |
erik-krogh
removed
the
Awaiting evaluation
I tried to figure out the performance regression in #3391, and that lead me to find some completely unrelated performance improvements. Which is why I'm opening a separate PR for these performance improvements.
8716790 and f8de691 helps reduce the impact of #3391, and 6d05b40 just improves performance in general.
An evaluation on nightly shows about 5% performance improvement.
(A redo of the all the regressions, and yet another redo of just the worst offender).
I have confirmed that each commit on its own improve performance, although the main impact seems to come from 6d05b40.
Here is an evaluation where master is compared first with just 6d05b40 and with all 3 commits together: link (in a sheet because formatting got weird in the report).
6d05b40 can cause some single queries to be slower, as it forces the
cached ReachableBasicBlock::strictlyDominatespredicate to be evaluated, and the query might not have needed that predicate without my change.But as
strictlyDominatesis a cached predicate, that performance penalty completely disappears when running many queries.TODO: