-
Notifications
You must be signed in to change notification settings - Fork 1.7k
JS: Make the edges of API-graphs into IPA types #7180
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
2dfa18c
to
97ee8d8
Compare
97ee8d8
to
e9df860
Compare
Have you tried this without an IPA type? We could have a predicate containing all labels occurring the graph, and use that to eliminate the binding sets and cache everything. |
I had also thought about making this change for Ruby, so perhaps I should give it a go. |
The bad join orders could still happen if we just eliminate the binding sets and cache the public API. But I'll give it a go. |
I tried two experiments where I removed the IPA type. 1: Just 2: Same code as the current PR, but revert back to labels as strings (implementation). |
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.
Ok, just a few comments otherwise LGTM
bindingset[result] | ||
string mod(string m) { result = "module " + m } | ||
/** A label in the API-graph */ | ||
abstract class ApiLabel extends Label::TLabel { |
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.
Is there a reason for this to be abstract?
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.
No, there is not. Good point.
I'll make the class non-abstract.
override string toString() { result = "return" } | ||
} | ||
|
||
class LabelMod extends ApiLabel { |
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.
I'd like to avoid using the abbreviation "mod = module".
class LabelMod extends ApiLabel { | |
class LabelModule extends ApiLabel { |
} | ||
|
||
/** Gets the edge label for the module `m`. */ | ||
LabelMod mod(string m) { result.getMod() = m } |
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.
LabelMod mod(string m) { result.getMod() = m } | |
LabelModule moduleLabel(string m) { result.getModule() = m } |
While looking at #6571 I kept running into performance issues related to API-graphs.
The RA would often contain an
inverseappend[..]
, where it would do string-comparisons on every edge in order to extract the member-edges.It sometimes took a lot of effort to avoid that bad join.
The obvious solution is to convert the edges into an IPA-type.
This gives a more complicated implementation, but it's an implementation where future use of API-graphs should perform better.
It also allows to
cache
the entire public API of API-graphs.I encountered a bunch of bad join order from this refactoring, so those were fixed along the way.
A multi-threaded evaluation shows a limited performance improvement.
Running single-threaded shows a more noticeable performance improvement.
The
cache more predicates
andoptimizations in global data flow
commits have no impact on performance on their own.(Evaluation of just those two commits).
And the
make ApiLabel into a IPA type
commit also has no impact on performance on its own (evaluation).