Skip to content

Ruby: exclude Object class from API graph #13683

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 2 commits into from
Jul 10, 2023

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Jul 7, 2023

The first commit unreverts #13496, which was previously reverted in #13620 due to performance issues.

The second commit fixes the performance issue by excluding Object from the API graph's model of the class hierarchy. API graphs aren't generally suitable for resolving methods on Object so there isn't a need for it. It caused the graph shape to become expensive for fastTC and also avoids problems with top-level include statements adding ancestors to Object.

  • Evaluation with RAM usage limited to 8 GB (down from the default of 15GB) shows there are no failing projects anymore.
  • Another evaluation with the default settings shows some failures, because the worker doesn't actually have enough memory to run with the default setting of 15 GB RAM. I've opened a PR internally to resolve the issue in DCA.

@asgerf asgerf marked this pull request as ready for review July 7, 2023 10:10
@asgerf asgerf requested review from a team as code owners July 7, 2023 10:10
/** Instances of `mod` with epsilon edges to its descendents, and to its upward node. */
MkModuleInstanceDown(DataFlow::ModuleNode mod) or
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to create an abstraction/class/predicate to capture this repeated logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would conflict with some of the follow-up changes I mentioned in my update so I decided to just keep it simple.

@calumgrant calumgrant requested a review from alexrford July 10, 2023 08:22
Copy link
Contributor

@alexrford alexrford left a comment

Choose a reason for hiding this comment

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

LGTM

@asgerf asgerf merged commit d88f557 into github:main Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants