C++: Eliminate recursion from toString(). #3387

geoffw0 · 2020-05-01T13:37:23Z

Removes recursion from toString(), resulting in moderately better performance when it's computed. The improvement comes from Element::ElementBase::toString_dispred#ff, which is still large and complex but is no longer recursive.

Some results:

Qt, SuspiciousAddWithSizeof.ql ~105s -> ~95s (90%)
wxWidgets, SuspiciousAddWithSizeof.ql 28s -> 24s (86%)
MongoDB, SuspiciousAddWithSizeof.ql ~29s -> 26s (90%)

As above but with #3352:

Qt, SuspiciousAddWithSizeof.ql ~86s -> ~74s (86%)

As a sequence (clearing cache only at the start):

mysql, NotInitialized.ql 232s -> 217s (94%)
mysql, AssignWhereCompareMeant.ql 17s -> 16s
mysql, SuspiciousAddWithSizeof.ql 3s -> 4s

And:

wireshark, select sum(Element e | | e.toString().length()) 129s -> 110s (85%)

Note that in the case of UsingDeclarationEntry I have traded detail in the result for better performance. There's a lot of scope for more changes like this if it's something we're comfortable doing.

jbj · 2020-05-01T15:44:11Z

I'll just @-mention @github/codeql-c-analysis since the CODEOWNERS file was broken when this PR was opened.

geoffw0 · 2020-05-01T16:09:46Z

It looks like the check is failing because of the identical files check:

16:31:43  ERROR: ql/config/sync-files.py:0 -     ./cpp/ql/src/semmle/code/cpp/XML.qll
16:31:43  ERROR: ql/config/sync-files.py:0 -     ./csharp/ql/src/semmle/code/csharp/XML.qll
16:31:43  ERROR: ql/config/sync-files.py:0 -     ./java/ql/src/semmle/code/xml/XML.qll
16:31:43  ERROR: ql/config/sync-files.py:0 -     ./javascript/ql/src/semmle/javascript/XML.qll
16:31:43  ERROR: ql/config/sync-files.py:0 -     ./python/ql/src/semmle/python/xml/XML.qll

I'll fix this up, but it would be good to get feedback on those changes first.

cpp/ql/src/semmle/code/cpp/Namespace.qll

jbj · 2020-05-02T08:44:22Z

Thanks for putting in the effort to eliminate this recursion. I had no idea how many changes it would require, but it looks like it's fairly minimal.

Will all the changes here preserve the exact same output we had before? It looks like UsingDeclarationEntry doesn't, but I think that's fixable.

geoffw0 · 2020-05-04T17:06:31Z

I had no idea how many changes it would require, but it looks like it's fairly minimal.

Yes, though I should mention that I've only looked at what happens when you import cpp, if you include other library files (such as PrintIR.qll I think) it's possible there may once again be recursion. I'm not sure how such imports interact with cacheing either.

Will all the changes here preserve the exact same output we had before? It looks like UsingDeclarationEntry doesn't, but I think that's fixable.

With the exception of UsingDeclarationEntry that was the intent, and following the changes you suggested, all output should be the same now (it turned out several tests were failing due to UsingDeclarationEntry as well, all of which are now good - which is reassuring - naturally we have pretty good coverage of toString()).

geoffw0 · 2020-05-05T08:30:08Z

Fixed the "sync identical files" issue. That means the part of this change in XML.qll is duplicated across all languages - though without any other changes I don't expect there to be much benefit.

tausbn

I don't see any issue with the XML changes as far as Python is concerned.

asgerf

It's causing some slightly concerning tuple count differences for JS (XSS query on juliusjs). I understand it's not exactly the fault of this PR, but rather a consequence of the unfortunate fact that XMLFile overrides File.toString.

It seems to me that XMLFile should just not be a subclass of File at all - logically it makes no sense that File.toString should have a special value for XML files, it's just an artifact of multiple inheritance. Is it feasible to remove the File base class instead?

With that said if it's important for C++ then LGTM. I'll deal with the fallout if it turns out to be an issue for JS.

jbj · 2020-05-12T11:52:28Z

I take back what I said in the meeting yesterday about this PR not having much benefit unless every last bit of recursion is eliminated.

I just looked at the evaluation logs for a large customer snapshot and found this in iteration 2:

[2020-05-12 12:38:57] (715s) Starting to evaluate predicate Element::ElementBase::toString_dispred#ff#cur_delta/2[2]@6df78e (iteration 2)
[2020-05-12 12:40:51] (829s) Tuple counts for Element::ElementBase::toString_dispred#ff#cur_delta:
                      0         ~0%      {2} r1 = JOIN Element::ElementBase::toString_dispred#ff#join_rhs AS L WITH Element::ElementBase::toString_dispred#ff#prev_delta AS R ON FIRST 1 OUTPUT L.<1>, R.<1>
                      0         ~0%      {2} r2 = JOIN namespace_decls_10#join_rhs AS L WITH Element::ElementBase::toString_dispred#ff#prev_delta AS R ON FIRST 1 OUTPUT L.<1>, R.<1>
                      0         ~0%      {2} r3 = r1 \/ r2
                      205059986 ~0%      {2} r4 = SCAN Element::ElementBase::toString_dispred#ff#prev_delta AS I OUTPUT I.<0>, ("using " ++ I.<1>)
                      0         ~0%      {2} r5 = JOIN r4 WITH Namespace::UsingDeclarationEntry::getDeclaration_dispred#ff_10#join_rhs AS R ON FIRST 1 OUTPUT R.<1>, r4.<1>
                      0         ~0%      {2} r6 = r3 \/ r5
                      0         ~0%      {2} r7 = JOIN Namespace::UsingDirectiveEntry::getNamespace_dispred#ff_10#join_rhs AS L WITH Element::ElementBase::toString_dispred#ff#prev_delta AS R ON FIRST 1 OUTPUT L.<1>, ("using namespace " ++ R.<1>)
                      0         ~0%      {2} r8 = r6 \/ r7
                      0         ~0%      {2} r9 = r8 AND NOT Element::ElementBase::toString_dispred#ff#prev AS R(r8.<0>, r8.<1>)
                                         return r9

Notice how it concatenates "using " with all results from the previous iteration before filtering those results to contain only UsingDeclarationEntry (0 rows). That means it constructs 205M strings that are immediately discarded.

The RA for iteration 1 seem fine in contrast, with the concatenation always happening before the filtering.

geoffw0 · 2020-05-12T15:31:51Z

It's causing some slightly concerning tuple count differences for JS (XSS query on juliusjs).

Thanks for spotting this. As far as I can tell from your report this isn't affecting results, just the number of rows in some intermediate computations - with potential performance impact. Do we know whether performance changes on this snapshot? Is the concern the extra DataFlow::DataFlow::Node::getAstNode_dispred#ff rows or does something else look fishy?

Unfortunately I'm not very well set up to test/debug this - I'm not set up for JS dev and haven't been able to find a single CPP snapshot that actually has XML elements in it.

jbj · 2020-05-20T10:38:29Z

~~The XML.qll problem may go away when #3520 (comment) happens.~~

jbj · 2020-05-25T15:10:35Z

@asgerf

It's causing some slightly concerning tuple count differences for JS (XSS query on juliusjs). I understand it's not exactly the fault of this PR, but rather a consequence of the unfortunate fact that XMLFile overrides File.toString.

There's clearly something I don't understand here. AFAICT the toString predicates on File and XMLParent both return the absolute path, so why does anything change with this PR? Would it work to implement XMLFile.toString as result = this.getAbsolutePath() instead? That would avoid going through the potentially ambiguous getName member.

It seems to me that XMLFile should just not be a subclass of File at all - logically it makes no sense that File.toString should have a special value for XML files, it's just an artifact of multiple inheritance. Is it feasible to remove the File base class instead?

There's no telling whether external users depend on this inheritance relationship, but I can easily imagine that they do. If we ensure that all ambiguous overrides on XMLFile are resolved in favour of File instead of XMLParent, does that solve the problem?

asgerf · 2020-05-26T07:34:29Z

(@geoffw0 sorry I missed your comment from 14d ago.)

There's clearly something I don't understand here. AFAICT the toString predicates on File and XMLParent both return the absolute path, so why does anything change with this PR? [...]

I agree the results should be the same, it's just a question of how well the optimizer can handle the code.

In any case, I'm not able to reproduce the results anymore; I must have done something wrong in my initial test. Instead I get a large reduction in tuple counts now. Sorry about that. Please just move ahead with the PR.

jbj

I've re-triggered the tests and will merge if they pass.

C++: Eliminate recursion from toString().

46332d4

geoffw0 added the C++ label May 1, 2020

rdmarsh2 reviewed May 1, 2020

View reviewed changes

cpp/ql/src/semmle/code/cpp/Namespace.qll Outdated Show resolved Hide resolved

geoffw0 added 3 commits May 4, 2020 17:46

C++: Update the 'usings' tests.

9fc37d1

C++: Combine the usings tests and add detail about classes.

3d43160

C++: Improve solution for UsingDeclarationEntry.

511d7c9

geoffw0 requested a review from a team as a code owner May 4, 2020 17:02

Sync identical files.

a70f534

geoffw0 requested review from a team as code owners May 5, 2020 08:28

geoffw0 added 4 commits May 5, 2020 10:05

C++: Make getAnonymousParameterDescription private.

31a7e2c

C++: Autoformat.

0b381b9

C++: Fix isfromtemplateinstantiation test.

2940f47

C++: Autoformat.

3e2e69c

tausbn approved these changes May 6, 2020

View reviewed changes

asgerf reviewed May 6, 2020

View reviewed changes

jbj approved these changes May 26, 2020

View reviewed changes

jbj merged commit 5deeda0 into github:master May 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

C++: Eliminate recursion from toString(). #3387

C++: Eliminate recursion from toString(). #3387

Uh oh!

geoffw0 commented May 1, 2020

Uh oh!

jbj commented May 1, 2020

Uh oh!

geoffw0 commented May 1, 2020

Uh oh!

Uh oh!

jbj commented May 2, 2020

Uh oh!

geoffw0 commented May 4, 2020

Uh oh!

geoffw0 commented May 5, 2020

Uh oh!

tausbn left a comment

Uh oh!

asgerf left a comment

Uh oh!

jbj commented May 12, 2020

Uh oh!

geoffw0 commented May 12, 2020

Uh oh!

jbj commented May 20, 2020 •

edited

Loading

Uh oh!

jbj commented May 25, 2020

Uh oh!

asgerf commented May 26, 2020

Uh oh!

jbj left a comment

Uh oh!

Uh oh!

C++: Eliminate recursion from toString(). #3387

C++: Eliminate recursion from toString(). #3387

Uh oh!

Conversation

geoffw0 commented May 1, 2020

Uh oh!

jbj commented May 1, 2020

Uh oh!

geoffw0 commented May 1, 2020

Uh oh!

Uh oh!

jbj commented May 2, 2020

Uh oh!

geoffw0 commented May 4, 2020

Uh oh!

geoffw0 commented May 5, 2020

Uh oh!

tausbn left a comment

Choose a reason for hiding this comment

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

jbj commented May 12, 2020

Uh oh!

geoffw0 commented May 12, 2020

Uh oh!

jbj commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbj commented May 25, 2020

Uh oh!

asgerf commented May 26, 2020

Uh oh!

jbj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jbj commented May 20, 2020 •

edited

Loading