Skip to content

Java: Fix more performance issues with future versions of codeql.#6755

Merged
aschackmull merged 3 commits intogithub:mainfrom
alexet:alexet/cache-params-string
Oct 1, 2021
Merged

Java: Fix more performance issues with future versions of codeql.#6755
aschackmull merged 3 commits intogithub:mainfrom
alexet:alexet/cache-params-string

Conversation

@alexet
Copy link
Copy Markdown
Contributor

@alexet alexet commented Sep 24, 2021

It turns out that between writing the previous fixes and retesting, even more problems arose for java. I think java is more affected because Reftype is bigger than Class, but most uses only need Class so trivial type tests affect java more than other langauges.

List of changes:

  • The params string change was more important than I thought. Although the fix only removed 10s, that predicate was evaluated multiple times so the saving multiplied. This caches it instead so we only evalaute it in full once.
  • The changes to AndroidComponent were no longer good enough. Instead this forces the TC to be materialised rather than use the bad magic version (which was done repeatedly). I decided to do this via getAnAncestor so I didn't have to define new predicates and as we evaluate the full TC we always want to use the non-magicked version after that. Note that only one use in each stage needs to be converted but as AndroidComponent was top in the magic as it has the first name alphabetically. I have opened an internal issue about making doing this change easier.
  • There was fairly bad magic for ConfusingOverloading.ql, the existing magic didn't help much so removing it seems better.

@alexet alexet requested a review from aschackmull September 24, 2021 16:35
@alexet alexet requested a review from a team as a code owner September 24, 2021 16:35
@github-actions github-actions bot added the Java label Sep 24, 2021
@aschackmull
Copy link
Copy Markdown
Contributor

The caching of params string, and the nomagic in ConfusingOverloading.ql LGTM. But I'm not sure I fully understand what goes on with AndroidComponent. This statement in particular is confusing to me:

Note that only one use in each stage needs to be converted but as AndroidComponent was top in the magic as it has the first name alphabetically. I have opened an internal issue about making doing this change easier.

Note also, that the pragma[inline] on getAnAncestor highlights a number of cases where TC is needlessly applied twice resulting in compilation errors - those should be easy fixes.

@alexet alexet force-pushed the alexet/cache-params-string branch from 0fa9ecc to 447eb23 Compare September 29, 2021 15:01
@alexet
Copy link
Copy Markdown
Contributor Author

alexet commented Sep 29, 2021

So with the magic, if within a stage for a call we pick no magic there is no magic ever. This means we simply need a single nomagic call to prevent magic. There were a few other places with problematic magic.

I ended not actually changing getAnAncestor as putting inline on it was problematic due to uses with noopt so I made a more local change.

I have opened an issue about making it easier to prevent magic for tcs.

@alexet alexet added the no-change-note-required This PR does not need a change note label Sep 30, 2021
@aschackmull aschackmull merged commit eb26b4a into github:main Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants