Skip to content

JS: Small performance improvements. #3442

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 12, 2020
Merged

JS: Small performance improvements. #3442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6d05b40
eliminate recursion from GuardControlFlowNode::dominates
erik-krogh May 7, 2020
8716790
deduplicate - and slightly optimize IndirectInclusionTest
erik-krogh May 7, 2020
f8de691
inline basicFlowStep into flowStep
erik-krogh May 7, 2020
2fbdece
add getContainedNode constraint to charpred of IndirectInclusionTest,…
erik-krogh May 12, 2020
bd768cb
autoformat
erik-krogh May 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions javascript/ql/src/semmle/javascript/CFG.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,8 +379,9 @@ class GuardControlFlowNode extends SyntheticControlFlowNode, @guard_node {
* is known to hold at `bb`.
*/
predicate dominates(ReachableBasicBlock bb) {
this = bb.getANode() or
dominates(bb.getImmediateDominator())
this = bb.getANode()
or
exists(ReachableBasicBlock prev | prev.strictlyDominates(bb) | this = prev.getANode())
}
}

Expand Down
10 changes: 6 additions & 4 deletions javascript/ql/src/semmle/javascript/InclusionTests.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,22 @@ 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.getContainedNode().getALocalSource() = DataFlow::parameterNode(callee.getAParameter()) and
inner.getContainerNode().getALocalSource() = DataFlow::parameterNode(callee.getAParameter())
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand this change.
The previous version used both getContainedNode and getContainerNode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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()).

}

override DataFlow::Node getContainerNode() {
exists(int arg |
inner.getContainerNode().getALocalSource().getEnclosingExpr() = callee.getParameter(arg) and
inner.getContainerNode().getALocalSource() =
DataFlow::parameterNode(callee.getParameter(arg)) and
result = this.getArgument(arg)
)
}

override DataFlow::Node getContainedNode() {
exists(int arg |
inner.getContainedNode().getALocalSource().getEnclosingExpr() = callee.getParameter(arg) and
inner.getContainedNode().getALocalSource() =
DataFlow::parameterNode(callee.getParameter(arg)) and
result = this.getArgument(arg)
)
}
Expand Down
18 changes: 2 additions & 16 deletions javascript/ql/src/semmle/javascript/dataflow/Configuration.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,21 +726,6 @@ private predicate basicFlowStepNoBarrier(
summary = PathSummary::return()
}

/**
* Holds if there is a flow step from `pred` to `succ` described by `summary`
* under configuration `cfg`.
*
* Summary steps through function calls are not taken into account.
*/
private predicate basicFlowStep(
DataFlow::Node pred, DataFlow::Node succ, PathSummary summary, DataFlow::Configuration cfg
) {
basicFlowStepNoBarrier(pred, succ, summary, cfg) and
isRelevant(pred, cfg) and
not isLabeledBarrierEdge(cfg, pred, succ, summary.getStartLabel()) and
not isBarrierEdge(cfg, pred, succ)
}

/**
* Holds if there is a flow step from `pred` to `succ` under configuration `cfg`,
* including both basic flow steps and steps into/out of properties.
Expand Down Expand Up @@ -1339,7 +1324,8 @@ private predicate flowStep(
DataFlow::Node pred, DataFlow::Configuration cfg, DataFlow::Node succ, PathSummary summary
) {
(
basicFlowStep(pred, succ, summary, cfg)
basicFlowStepNoBarrier(pred, succ, summary, cfg) and
isRelevant(pred, cfg)
or
// Flow through a function that returns a value that depends on one of its arguments
// or a captured variable
Expand Down