Skip to content

Comments

Python: fix a couple of performance issues#950

Merged
taus-semmle merged 2 commits intogithub:masterfrom
markshannon:python-fix-a-couple-of-performance-issues
Feb 19, 2019
Merged

Python: fix a couple of performance issues#950
taus-semmle merged 2 commits intogithub:masterfrom
markshannon:python-fix-a-couple-of-performance-issues

Conversation

@markshannon
Copy link
Contributor

Fixes two cases of bad join order.

Copy link
Contributor

@taus-semmle taus-semmle left a comment

Choose a reason for hiding this comment

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

Generally LGTM apart from a few questions.

)
}

private EssaVariable ssa_variable_for_module_attribute(ImportMemberNode f, PointsToContext context) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be pragma[noinline] to prevent the refactoring from being undone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't undone now. If the optimiser changes and inlines it without slowing down, then it's not a problem.

}

/** Maps the caller object/context to callee parameter/context for self in calls to methods */
private predicate self_in_method_call(ControlFlowNode obj, PointsToContext caller, ParameterDefinition self, PointsToContext callee) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, pragma[noinline], perhaps?

self.isSelf() and
exists(FunctionObject meth, CallNode call |
meth.getFunction() = self.getScope() and
callee.fromCall(call, meth, caller) and
Copy link
Contributor

Choose a reason for hiding this comment

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

No method_call(meth, caller, call)? Does the addition of self.isSelf() somehow obviate the need for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but callee.fromCall(call, meth, caller) does.

@taus-semmle taus-semmle merged commit 9e1a523 into github:master Feb 19, 2019
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.

2 participants