Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SmartLocalDefs is often recomputed #138

Open
ericbodden opened this issue Oct 22, 2013 · 10 comments
Open

SmartLocalDefs is often recomputed #138

ericbodden opened this issue Oct 22, 2013 · 10 comments

Comments

@ericbodden
Copy link
Member

A simple investigation shows that Soot often recomputes SmartLocalDefs multiple times for the same method (often five times). The analysis is rather costly and should probably be cached instead.

@ericbodden
Copy link
Member Author

Note: This might not be as easily fixable as I thought. The problem is that the Jimple code is often transformed between subsequent invocations. I guess SSA/Shimple might help here but that would be a quite incompatible change.

@patricklam
Copy link
Contributor

Could we checksum the Jimple Body or otherwise ensure that it's not changed?

SSA form also imposes a lot of constraints in keeping things up to date; it
may not actually have a performance win and certainly makes code more
complicated.

On Tue, Oct 22, 2013 at 10:24 AM, Eric Bodden notifications@github.comwrote:

Note: This might not be as easily fixable as I thought. The problem is
that the Jimple code is often transformed between subsequent invocations. I
guess SSA/Shimple might help here but that would be a quite incompatible
change.


Reply to this email directly or view it on GitHubhttps://github.com//issues/138#issuecomment-26807215
.

@StevenArzt
Copy link
Contributor

I would vote against SSA since there are many transformers which would have to be rewritten to maintan SSA (which would complicate the code quite a bit) or would have to be followed by a step that re-establishes SSA. In sum, I'm not sure that this would by us anything in terms of performance.

@badlogic
Copy link

We are hitting this issue as well, and it imposes quite some runtime overhead in our application. Most of the time seems to be spend in the flow analysis according to JProfiler. I saw some recent changes in ForwardFlowAnalysis, did this improve performance?

@ericbodden
Copy link
Member Author

Yes, we had some performance improvements in the past year, however the main problem still remains: The conversion to Jimple happens in several passes, each pass of which might change (and often does change) the respective JimpleBody. After each of these passes, the SimpleLocalDefs must be recomputed, unless we find a way to incrementally update it, or to maybe perform some checksums as @patricklam suggested. If I find some time I will look into this a bit.

@badlogic
Copy link

Great. I ported the latest flow analysis code which had a tiny impact on
the runtime.

On Sat, Jan 31, 2015 at 2:38 PM, Eric Bodden notifications@github.com
wrote:

Yes, we had some performance improvements in the past year, however the
main problem still remains: The conversion to Jimple happens in several
passes, each pass of which might change (and often does change) the
respective JimpleBody. After each of these passes, the SimpleLocalDefs must
be recomputed, unless we find a way to incrementally update it, or to maybe
perform some checksums as @patricklam https://github.com/patricklam
suggested. If I find some time I will look into this a bit.


Reply to this email directly or view it on GitHub
#138 (comment).

ericbodden pushed a commit that referenced this issue Feb 1, 2015
…#138

The mechanism works as follows:
Clients that previously used to create a new SmartLocalDefs based on a SimpleLiveLocals based on an ExceptionalUnitGraph now instead call a factory method on the new SmartLocalDefsPool. This method either creates a new analysis (along with a new default unit graph), or it returns the one that has previously been created for the same (!) body. To know when body was changed, the pool keeps track of modification counts for bodies. A body was modified when either its locals, units or traps were modified. Luckily, those three chains already keep track of a modification count, which we reuse here.

Clients that use SmartLocalDefs over regular ExceptionalUnitGraphs have been updated to use the pool. Some use other UnitGraphs, however, for instance some passes for Android. Those have not been updated.
@ericbodden
Copy link
Member Author

I implemented some pooling in commit 454f7c4
Let's see what our nightly builds say about this. Hope it works. I ran some statistics. If it works then we should safe the creation of about every second SmartLocalDefs instance (and the connected SimpleLiveLocals and ExceptionalUnitGraph instances).

I was unable to update some clients that use DalvikThrowAnalysis. To pool for those, too, we would need to extend the mechanism a bit.

@badlogic
Copy link

badlogic commented Feb 2, 2015

This is awesome! I'll try to backport it to our Soot fork and will report
results. Looks like a simple fix :)
On Feb 2, 2015 1:01 AM, "Eric Bodden" notifications@github.com wrote:

I implemented some pooling in commit 454f7c4
454f7c4
Let's see what our nightly builds say about this. Hope it works. I ran
some statistics. If it works then we should safe the creation of about
every second SmartLocalDefs instance (and the connected SimpleLiveLocals
and ExceptionalUnitGraph instances).

I was unable to update some clients that use DalvikThrowAnalysis. To pool
for those, too, we would need to extend the mechanism a bit.


Reply to this email directly or view it on GitHub
#138 (comment).

@ericbodden
Copy link
Member Author

Note to self: we should find a way to clear the pool at a sensible time, when Jimplification has completed.

@ericbodden
Copy link
Member Author

@badlogic You can probably simply cherry-pick the commit via git.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants