-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Refactor heap and cache outers #12608
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
Conversation
This is safe because heap is monotonistic and fields abstractions are immutable
In extending the abstract heap to reason about multiple objects, a key concern is the design of addresses. Since in this case, we want must information (sets of fields that must have been assigned), we need each abstract address to represent (at most) one single concrete object. Since we want the heap to reason about objects whose constructors we are analyzing, perhaps the design of addresses could be based on the stack of constructors that we analyze. For example, for the program
the call stack in our analysis might look like this:
Then our addresses could be something like |
@olhotak Thanks for the feedback. This PR is essentially the same as the master --- the heap enables more caching. It would be good to motivate the proposal above with concrete examples which are related to concerns about expressiveness, soundness or termination. Let's discuss more tomorrow. |
I thought some more about how to design the We have two analyses, This suggests keeping a single But then we want to leak non-hot values to methods (method parameters), constructors, and to local variables of methods. For this, we need to keep track of which method parameters, constructor parameters (fields), and local variables have non-hot values. So we need an Environment. An Environment should be a map from parameters/variables to Unlike Then perhaps such an Environment can replace I find After this, one possible further precision improvement could be to extend the |
Thank you @olhotak for the many good points, I agree with many of them. At the high-level, many styles of analysis are equivalent in essence,
In addition to local reasoning, another aspect that is heavily Note that it's not enough that I conjecture many analysis lie in the category of heap-monotonistic
Yes, that's correct. Introduction of environment is a natural step. We As a first step, to support non-hot values to primary constructors, To support non-hot values for secondary constructors, we may still
Conceptually, it makes sense. However, it ignores an important
I agree, conceptually it's possible to treat
This does not sound like a good idea from the engineering point of The fact that we associate outers with object in concrete semantics On the other hand, a design consideration of ADI is to be as close as
This is because currently we only support warm objects of inner
It's tempting to allow such use cases. However, we need to be cautious
Coming back to the PR itself, this PR is intended to achieve the following
I believe the current PR is the simplest from the perspective of |
case class Objekt(klass: ClassSymbol, val fields: mutable.Map[Symbol, Value]) { | ||
val promotedValues = mutable.Set.empty[Value] | ||
} | ||
case class Objekt(klass: ClassSymbol, fields: mutable.Map[Symbol, Value], outers: mutable.Map[ClassSymbol, Value]) | ||
|
||
/** Abstract heap stores abstract objects | ||
* | ||
* As in the OOPSLA paper, the abstract heap is monotonistic. | ||
* | ||
* This is only one object we need to care about, hence it's just `Objekt`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line of the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, now it's removed
if target.isOneOf(Flags.Method | Flags.Lazy) then | ||
if target.hasSource then | ||
val cls = target.owner.enclosingClass.asClass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changed from target.owner.asClass
to target.owner.enclosingClass.asClass
. Is that intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's intentional -- here we reuse the code for calling local methods.
The related code change can be found below for case id: Ident =>
.
val value = Warm(klass, outer) | ||
if !heap.contains(value) then | ||
val obj = Objekt(klass, fields = mutable.Map.empty, outers = mutable.Map(klass -> outer)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this Objekt
continue to have empty fields
and outers
forever since we never call init
on the new value
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call happens at line 367 -- it's a constructor call on the object.
Some refactoring: