Skip to content

Conversation

aschackmull
Copy link
Contributor

This fixes the performance of UselessParameter.ql, BoxedVariable.ql and FLinesOfCommentedCode.ql on jdk11 when toString is being cached as reported on #2204. There were two cases of bad magic being introduced. The predicate jsniComment was a bit different in that it was already badly join-ordered, but wasn't being evaluated due to a sentinel check - caching toString somehow changed this, so it was being evaluated (to 0 tuples).

@yh-semmle
Copy link
Contributor

Having rerun the test, there is now a non-negligible increase in runtime for ExecTainted.ql on JDK 11 that didn't occur previously. Could that be an indication that the concern about potential cache-invalidation applies to the Java libraries?

@aschackmull
Copy link
Contributor Author

I'm having some trouble reproducing this. How big was the time increase?

@yh-semmle
Copy link
Contributor

The results of the test run are no longer available. I'll trigger another one.

@aschackmull
Copy link
Contributor Author

I'm btw. currently investigating a separate cache-invalidation problem that's triggered by exactly ExecTainted.ql - unrelated to this PR it's causing a recalculation of virtual dispatch. So if the time increase is consistent with the time it takes to recalculate toString then it could be the same issue.

@aschackmull
Copy link
Contributor Author

See #2447

@yo-h
Copy link
Contributor

yo-h commented Nov 26, 2019

Thanks, the issue addressed by #2447 seems like a highly plausible cause. Could you rebase this PR onto #2447, and I'll trigger another test.

@aschackmull aschackmull force-pushed the java/cached-tostring-perf-fixes branch from d22e3ed to 3d0e3aa Compare November 27, 2019 08:06
@aschackmull
Copy link
Contributor Author

Rebased

@yo-h
Copy link
Contributor

yo-h commented Dec 2, 2019

Performance looks good now overall. (A few queries, in particular AlertSuppression.ql, UrlRedirect.ql and ImpossibleJavadocThrows.ql, took up to 2x as long to run in this test on JDK 11, but they all take < 30s, so it doesn't seem blocking.)

Did you investigate the extent to which the cache size increases? (Unfortunately, the test job cleans up that data when it finishes.)

@aschackmull
Copy link
Contributor Author

Did you investigate the extent to which the cache size increases? (Unfortunately, the test job cleans up that data when it finishes.)

As best as I have been able to measure (comparing runs of ImpossibleJavadocThrows.ql from a cold cache along with post-run codeql database clean) the caching of toString on jdk11 appears to take approx. 164 MB. Given the size of the snapshot that doesn't seem too bad.

@yo-h yo-h merged commit 69a2632 into github:master Dec 17, 2019
@aschackmull aschackmull deleted the java/cached-tostring-perf-fixes branch December 17, 2019 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants