Skip to content

JS: Make the edges of API-graphs into IPA types - #7180

Merged
erik-krogh merged 8 commits into
github:mainfrom
erik-krogh:apiLabel2
Dec 1, 2021
Merged

JS: Make the edges of API-graphs into IPA types#7180
erik-krogh merged 8 commits into
github:mainfrom
erik-krogh:apiLabel2

Conversation

@erik-krogh

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

Copy link
Copy Markdown
Contributor

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

asgerf commented Nov 22, 2021

Copy link
Copy Markdown
Contributor

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

hvitved commented Nov 22, 2021

Copy link
Copy Markdown
Contributor

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

@erik-krogh

erik-krogh commented Nov 22, 2021

Copy link
Copy Markdown
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.

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
Copy Markdown
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).

@asgerf asgerf left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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