Java: Cache interpretElement. #15570
Conversation
|
Testing locally, this change does fix the duplication of work I noticed. 👍 |
aschackmull
added
the
no-change-note-required
|
DCA looks mostly uneventful, with a possible speedup on one project. |
| @@ -430,6 +430,7 @@ private Element interpretElement0( | |||
| } | |||
|
|
|||
| /** Gets the source/sink/summary/neutral element corresponding to the supplied parameters. */ | |||
| cached | |||
| Element interpretElement( | |||
There was a problem hiding this comment.
I suspect that this is actually not the right thing to cache. I checked where it is used, and I think it ultimately boils down to the TDataFlowCallable newtype not being cached.
There was a problem hiding this comment.
We probably also need to cache viableCallableExt in DataFlowImplCommon.qll.
There was a problem hiding this comment.
The latter is done here: #15582
There was a problem hiding this comment.
I'm not so sure. There's a rather large dependency fan-out from interpretElement. And by not caching it, I can see that it would end up in at least two different DIL stages. It's used in at least ExternalFlow::sourceNode and ExternalFlow::sinkNode, which form their own stage, and in FlowSummaryImpl::summaryElement, which feeds into SummarizedCallableAdapter and its member predicates, which in turn feeds into the entire shared FlowSummaryImpl node synthesis. And the output of that is being used in at least two different cached stages as well (local steps are currently cached separately from the DataFlowImplCommon bulk caching).
Henning noticed a lot of fastTC duplication (29x at the DIL level across a query suite). Cachaca has been saving us somewhat, but not completely, so this PR should eliminate the duplication.