Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Aug 9, 2021

I noticed this bad join order when running UnreachableCode.ql:

[2021-08-04 11:04:54] (3288s) Tuple counts for AstExtended::AstNode::containsInScope_dispred#bf/2@06c1ea:
                      485725      ~0%      {1} r1 = SCAN py_stmts OUTPUT In.0 'this'
                      485725      ~0%      {2} r2 = JOIN r1 WITH AstExtended::AstNode::getScope_dispred#ff ON FIRST 1 OUTPUT Rhs.1, Lhs.0 'this'
                      10971648538 ~0%      {2} r3 = JOIN r2 WITH AstExtended::AstNode::getScope_dispred#ff_10#join_rhs ON FIRST 1 OUTPUT Lhs.1 'this', Rhs.1 'inner'
                      5089967     ~0%      {2} r4 = JOIN r3 WITH AstExtended::AstNode::contains_dispred#bf ON FIRST 2 OUTPUT Lhs.0 'this', Lhs.1 'inner'
                      5046621     ~1%      {2} r5 = r4 AND NOT @py_scope#f(Lhs.1 'inner')
                                           return r5

https://jenkins.internal.semmle.com/job/Changes/job/Python-Differences/654/:

  D0(s) D1(s) D1/D0
g/openstack/nova 2054 2085 1.01509
g/django/django 1026 1033 1.00682
g/python/cpython 1777 1785 1.00450
g/ytdl-org/youtube-dl 437 438 1.00229
g/pallets/flask 607 603 0.99341
g/saltstack/salt 2008 1986 0.98904
g/FreeCAD/FreeCAD 5770 4405 0.76343

@github-actions github-actions bot added the Python label Aug 9, 2021
@hvitved hvitved marked this pull request as ready for review August 9, 2021 16:14
@hvitved hvitved requested a review from a team as a code owner August 9, 2021 16:14
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Aug 9, 2021
Copy link
Contributor

@yoff yoff 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 find and the solution! 💪

Out of curiosity: Is it correct that it might still choose to evaluate not inner instanceof Scope before this.contains(inner) and could we avoid that with binding directives?

@hvitved
Copy link
Contributor Author

hvitved commented Aug 10, 2021

Out of curiosity: Is it correct that it might still choose to evaluate not inner instanceof Scope before this.contains(inner) and could we avoid that with binding directives?

That shouldn't matter much, what matters is that we get rid of the self-join of AstExtended::AstNode::getScope.

@hvitved hvitved merged commit d658ef1 into github:main Aug 10, 2021
@hvitved hvitved deleted the python/contains-in-scope-perf branch August 10, 2021 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-required This PR does not need a change note Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants