Skip to content

Comments

CPP: Cache Expr.getType().#1336

Merged
jbj merged 1 commit intogithub:masterfrom
geoffw0:cached2
May 27, 2019
Merged

CPP: Cache Expr.getType().#1336
jbj merged 1 commit intogithub:masterfrom
geoffw0:cached2

Conversation

@geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented May 17, 2019

@jbj as discussed in the meeting. I tested this with a series of five queries from a clean cache (largeparameter.ql, castarraypointerarithmetic.ql, badlyboundedwrite.ql, wrongtypeformatarguments.ql, slicing.ql, all of which are on LGTM), with the first run discarded.

Chakracore
base:
	largeparameter.ql 38s
	castarraypointerarithmetic.ql 233s
	badlyboundedwrite.ql 31s
	wrongtypeformatarguments.ql 29s
	slicing.ql 8s
	TOTAL: 339s
with cached `Expr.getType()`:
	largeparameter.ql 35s
	castarraypointerarithmetic.ql 206s
	badlyboundedwrite.ql 13s
	wrongtypeformatarguments.ql 23s
	slicing.ql 3s
	TOTAL: 280s (83%)

Qt
base:
	largeparameter.ql 79s
	castarraypointerarithmetic.ql 470s
	badlyboundedwrite.ql 31s
	wrongtypeformatarguments.ql 59s
	slicing.ql 9s
	TOTAL: 648s
with cached `Expr.getType()`:
	largeparameter.qls 79s
	castarraypointerarithmetic.ql 457s
	badlyboundedwrite.ql 27s
	wrongtypeformatarguments.ql 57s
	slicing.ql 6s
	TOTAL: 626s (97%)

Note that there are more queries on LGTM that use this predicate.

The worrying part is that Expr.getType() should be rather trivial:

pragma[nomagic] Type getType() { expr_types(underlyingElement(this),unresolveElement(result),_) }

so it's a bit surprising it showed up so high in our profiling, even being so heavily used.

Other predicates from the spreadsheet that might be in a similar situation to this one: Element::getLocation, Expr::toString, Call::getTarget, Element::getFile, Location::hasLocationInfo, Access::getTarget, Class::getCanonicalMember.

@geoffw0 geoffw0 added the C++ label May 17, 2019
@geoffw0 geoffw0 requested a review from a team as a code owner May 17, 2019 17:02
@jbj
Copy link
Contributor

jbj commented May 27, 2019

Expr.getType is non-trivial because it's overridden in FunctionCall. I'd still like to investigate if we can speed it up, but caching it seems like a good choice in any case.

@jbj jbj merged commit d2fa7aa into github:master May 27, 2019
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.

2 participants