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

Keep isSoftRemoved calls in-iteration, prevent List updates. #8370

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

isoos
Copy link
Collaborator

@isoos isoos commented Dec 6, 2024

This is about 1-1.5% of the time when loading the search snapshot.

@isoos isoos requested review from jonasfj and sigurdm December 6, 2024 13:04
Copy link
Contributor

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

Not sure I like this change - it seems to distribute complexity

@isoos
Copy link
Collaborator Author

isoos commented Dec 6, 2024

it seems to distribute complexity

Not sure: why?

In two cases the change is local: it is replacing a List.removeWhere() into a List.where iteration.
In the snapshot building case we don't add the soft-removed package to the snapshot in the first place, so we don't need to remove them later.

@sigurdm
Copy link
Contributor

sigurdm commented Jan 7, 2025

Not sure: why?

It seems to me that it distributes the burden of attention from the creation of the snapshot to the usage of it. That feels backwards, and not worth it for a 1% improvement.

Is there a way to construct the snapshot so it never contains the soft-removed packages in the first place?

@isoos
Copy link
Collaborator Author

isoos commented Jan 7, 2025

Is there a way to construct the snapshot so it never contains the soft-removed packages in the first place?

The changes on line 135 in app/lib/search/backend.dart do exactly that.

@sigurdm
Copy link
Contributor

sigurdm commented Jan 7, 2025

Ah! - but why do you then need to account for them in app/lib/frontend/handlers/custom_api.dart line 82?

@isoos
Copy link
Collaborator Author

isoos commented Jan 7, 2025

Ah! - but why do you then need to account for them in app/lib/frontend/handlers/custom_api.dart line 82?

That's a different use case with a different data source. We get the names from the name tracker, which has all the names from all the Package entities. Having a second look on it, I will move that logic inside the name-tracker, as it should be part of the getVisiblePackageNames method.

@isoos isoos merged commit 3f98201 into dart-lang:master Jan 7, 2025
32 checks passed
@isoos isoos deleted the soft-removed branch January 7, 2025 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants