Skip to content

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

Merged
merged 8 commits into from
Dec 1, 2021

Conversation

erik-krogh
Copy link
Contributor

@erik-krogh erik-krogh commented Nov 19, 2021

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 and optimizations 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).

@erik-krogh erik-krogh added JS no-change-note-required This PR does not need a change note labels Nov 19, 2021
@erik-krogh erik-krogh force-pushed the apiLabel2 branch 2 times, most recently from 2dfa18c to 97ee8d8 Compare November 22, 2021 10:08
@erik-krogh erik-krogh marked this pull request as ready for review November 22, 2021 11:20
@erik-krogh erik-krogh requested a review from a team as a code owner November 22, 2021 11:20
@asgerf
Copy link
Contributor

asgerf commented Nov 22, 2021

It also allows to cache the entire public API of API-graphs.

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.

@hvitved
Copy link
Contributor

hvitved commented Nov 22, 2021

I had also thought about making this change for Ruby, so perhaps I should give it a go.

@erik-krogh
Copy link
Contributor Author

erik-krogh commented Nov 22, 2021

It also allows to cache the entire public API of API-graphs.

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.

The bad join orders could still happen if we just eliminate the binding sets and cache the public API.
Something like result = someAPINode.getASuccessor(API::Label::member(someName)) could still introduce an inverseAppend.

But I'll give it a go.

@erik-krogh
Copy link
Contributor Author

It also allows to cache the entire public API of API-graphs.

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 tried two experiments where I removed the IPA type.
Both experiments showed neutral performance, so I still think we should try the IPA types.

1: Just cache the public API (implementation).
Evaluations: multi-threaded, and single-threaded both show about neutral performance (0.993 and 1.001 respectively).

2: Same code as the current PR, but revert back to labels as strings (implementation).
Evaluations: multi-threaded, and single-threaded shows that there might be a tiny performance improvement (0.997 and 0.988 respectively).

Copy link
Contributor

@asgerf asgerf left a 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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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 {
Copy link
Contributor

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".

Suggested change
class LabelMod extends ApiLabel {
class LabelModule extends ApiLabel {

}

/** Gets the edge label for the module `m`. */
LabelMod mod(string m) { result.getMod() = m }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LabelMod mod(string m) { result.getMod() = m }
LabelModule moduleLabel(string m) { result.getModule() = m }

@erik-krogh erik-krogh merged commit a077345 into github:main Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants