Skip to content
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

Split constant and method entries #1949

Closed
wants to merge 1 commit into from

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Apr 17, 2024

Motivation

First step for #1950, which will unblock ancestor linearization

Currently, we are putting all types of entries in the same data structure, mixing constants and methods. This leads to the following issues:

  1. Naming conflicts lead to weird scenarios. For example, the Array class and Array() method. We end up having different types of entries for the same name
  2. We get worse typing on the index API. For example, in resolve_method, we need to apply a bunch of casts since we know that the returned entry will be a method
  3. For Refactor index to nest declarations inside entries #1950 to work properly, we will need to have constants split from methods

Implementation

Created separate data structures for methods. We now have a separate hash and separate prefix tree for methods.

  • I benchmarked this and noticed no difference in indexing speed or memory consumption
  • While the APIs are now split (e.g.: resolve_constant and resolve_method), I do not think this impacts the DX of using the index. We always know if something is a method call or a constant reference, there would never be a scenario where we are in doubt about which API to invoke

Automated Tests

Adapted our tests.

@vinistock vinistock added breaking-change Non-backward compatible change server This pull request should be included in the server gem's release notes labels Apr 17, 2024
@vinistock vinistock self-assigned this Apr 17, 2024
@vinistock vinistock force-pushed the vs/split_constant_and_method_entries branch from 38327f4 to d114d41 Compare April 23, 2024 18:10
@vinistock vinistock marked this pull request as ready for review April 23, 2024 18:15
@vinistock vinistock requested a review from a team as a code owner April 23, 2024 18:15
@vinistock vinistock requested review from andyw8 and st0012 April 23, 2024 18:15
@vinistock vinistock force-pushed the vs/split_constant_and_method_entries branch 2 times, most recently from e8642f5 to 5746b68 Compare April 24, 2024 16:57
@vinistock vinistock force-pushed the vs/split_constant_and_method_entries branch from 5746b68 to 9ef56fc Compare April 25, 2024 13:32
@vinistock
Copy link
Member Author

We decided to drop this approach and go with #2024.

@vinistock vinistock closed this May 29, 2024
@vinistock vinistock deleted the vs/split_constant_and_method_entries branch June 19, 2024 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Non-backward compatible change server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants