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

CPU Spike - Ingestion suggestions #20289

Open
data-sync-user opened this issue May 16, 2024 · 6 comments
Open

CPU Spike - Ingestion suggestions #20289

data-sync-user opened this issue May 16, 2024 · 6 comments
Assignees

Comments

@data-sync-user
Copy link
Collaborator

data-sync-user commented May 16, 2024

We have a large spike in CPU exceptions that correlates with this experiment. It started around May 1st.

!image-20240516-165948.png|width=1007,height=378,alt="image-20240516-165948.png"!

┆Issue is synchronized with this Jira Task

@data-sync-user data-sync-user changed the title CPU Spike - Sync problem with suggestions CPU Spike - Ingestion suggestions May 16, 2024
@linabutler linabutler self-assigned this May 16, 2024
@linabutler
Copy link
Member

I think the "CPU exception" is a diagnostic that's emitted whenever the app is using too much CPU time in the background, and the OS terminates it—it's not a typical user-visible crash.

I think the high CPU usage might be caused by SuggestStore.ingest() blocking the main thread until it's done. ingest() is called from the RustFirefoxSuggest actor, but, per @OrlaM's comment here, actors aren't really appropriate for this—we should be using a dispatch queue instead, and awaiting its result via withCheckedContinuation to make it play well with async/await.

Once we have a SuggestStore.interruptEverything() API (https://bugzilla.mozilla.org/show_bug.cgi?id=1897264) in the Rust component, we can change iOS to:

@linabutler
Copy link
Member

Call SuggestStore.ingest() and SuggestStore.query() on a (concurrent) dispatch queue.

Nitpicking my own analysis (🧐): it's almost definitely more efficient to use a pair of serial queues for this (with appropriate QoS), than a concurrent queue. There's a bit more about that captured here.

@data-sync-user
Copy link
Collaborator Author

➤ Norberto Andres Furlan commented:

Lina Butler to confirm if we can have this for FX 127.

CC: Nive Suresh Orla Mitchell

@data-sync-user
Copy link
Collaborator Author

➤ Diana Andreea Barladeanu commented:

Hi, Lina Butler ! What should QA test in this ticket? Thanks!

@data-sync-user
Copy link
Collaborator Author

➤ Lina Butler commented:

Hi Diana Andreea Barladeanu! 👋 I think just a smoke test that enables the Cross-Platform Suggest experiment, and makes sure that the suggestions are eventually ingested and show up in the address bar, would be sufficient for testing. This ticket reworked the implementation, but the user-facing experience should be the same. Thanks!

@data-sync-user
Copy link
Collaborator Author

➤ Diana Andreea Barladeanu commented:

Validated on v127 (42448), with iPhone 15 (17.4.1).

!RPReplay_Final1717484132.mp4|width=590,height=1280,alt="RPReplay_Final1717484132.mp4"!

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

No branches or pull requests

2 participants