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

Optimize indexing comment collection #2547

Merged
merged 1 commit into from
Sep 16, 2024
Merged

Conversation

vinistock
Copy link
Member

Motivation

Closes #2340

This PR makes the index fetch entry documentation only when requested, rather than eagerly indexing all comments. The idea is that not storing all documentation for all entries eagerly will reduce memory usage and speed up initial indexing.

In benchmarks, the impact was not as high as I had hoped. About 7.5% memory reduction and 5% indexing time reduction.

Implementation

The idea is that we add a flag to ignore comments on the initial indexing. When a file is modified, we turn on the flag so that we can capture the comments on a file currently opened in the UI.

When anything tries to read comments, the index fetches them lazily using Prism.parse_file_comments.

Automated Tests

Existing tests cover it.

@vinistock vinistock added server This pull request should be included in the server gem's release notes other Changes that aren't bugfixes, enhancements or breaking changes labels Sep 12, 2024
@vinistock vinistock self-assigned this Sep 12, 2024
Copy link
Contributor

@andyw8 andyw8 left a comment

Choose a reason for hiding this comment

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

Overall this seems failry unobtrusive for a small but decent reduction in memory use.

@vinistock vinistock force-pushed the vs-lazily-fetch-comments branch from 273719f to a0b74ed Compare September 13, 2024 18:46
@vinistock vinistock marked this pull request as ready for review September 13, 2024 18:47
@vinistock vinistock requested a review from a team as a code owner September 13, 2024 18:47
@vinistock vinistock requested a review from st0012 September 13, 2024 18:47
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

I think it should be considered a breaking change when Entry#comments is changed to return a string instead of an array of strings. Is this change necessary for the performance improvement?

@vinistock
Copy link
Member Author

I think it should be considered a breaking change when Entry#comments is changed to return a string instead of an array of strings. Is this change necessary for the performance improvement?

Yes, it reduces the number of objects allocated since we're not longer maintaining and array of multiple strings, but instead a single string.

Conceptually, I agree, it's a breaking change. But do we know if any addon is actually invoking comments directly on entries? The Rails addon doesn't do that.

@st0012
Copy link
Member

st0012 commented Sep 13, 2024

Conceptually, I agree, it's a breaking change. But do we know if any addon is actually invoking comments directly on entries? The Rails addon doesn't do that.

I don't know any, and I don't plan to block the PR for this. But IMO it's worth adding the breaking change label here just in case.

Do we know how much of the memory reduction is contributed by lazy indexing, and how much is contributed by the type change?
If, for example, the lazy comment indexing just contributed to 1~2% of memory reduction by itself, then I'd prefer not maintaining 2 different comment collection logic.

@vinistock
Copy link
Member Author

Do we know how much of the memory reduction is contributed by lazy indexing, and how much is contributed by the type change?
If, for example, the lazy comment indexing just contributed to 1~2% of memory reduction by itself, then I'd prefer not maintaining 2 different comment collection logic.

I just benchmarked this to compare. The lazy logic is responsible for 6.8% out of the 7.5% reduction. The reduction related to turning the comments from an array into strings is the smaller part.

@st0012
Copy link
Member

st0012 commented Sep 16, 2024

@vinistock Thanks for benchmarking the difference. IMO we can update this PR's title to be more generic, like "Optimize comment indexing", as the comment type change is also not trivial.

@vinistock vinistock changed the title Lazily fetch entry comments Optimize indexing comment collection Sep 16, 2024
@vinistock vinistock merged commit 9ad1d5d into main Sep 16, 2024
38 checks passed
@vinistock vinistock deleted the vs-lazily-fetch-comments branch September 16, 2024 15:31
@vinistock vinistock added breaking-change Non-backward compatible change and removed other Changes that aren't bugfixes, enhancements or breaking changes labels Sep 16, 2024
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.

Fetch index comments lazily
3 participants