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

fix: Fixes issue #2579 #2580

Closed
wants to merge 7 commits into from
Closed

Conversation

jleinenbach
Copy link
Contributor

Summary of the CPU Issue and Solution

Problem:
The implementation with parallel loading of Alexa components caused high CPU usage. Multiple components were loaded simultaneously, which overwhelmed the system due to the concurrent resource-heavy operations.

Solution:

  1. Serial Loading: I addressed the CPU problem by serializing the loading process. This means components are now loaded one after the other instead of all at once, reducing the CPU load.

  2. Condition with entry_setup: I reintroduced the check for if not entry_setup. This ensures that we only initialize components if they are not already set up, preventing redundant operations. For components already initialized, the same async_forward_entry_setup method is called to reload them.

Outcome:
By loading components sequentially and ensuring that unnecessary setups are skipped, we significantly reduce the CPU load while maintaining efficient processing of new Alexa clients.

jleinenbach and others added 3 commits October 3, 2024 11:41
### Summary of the CPU Issue and Solution

**Problem:**  
The implementation with **parallel loading** of Alexa components caused high CPU usage. Multiple components were loaded simultaneously, which overwhelmed the system due to the concurrent resource-heavy operations.

**Solution:**
1. **Serial Loading:**  
   I addressed the CPU problem by **serializing** the loading process. This means components are now loaded **one after the other** instead of all at once, reducing the CPU load.
   
2. **Condition with `entry_setup`:**  
   I reintroduced the check for `if not entry_setup`. This ensures that we only initialize components if they are not already set up, preventing redundant operations. For components already initialized, the same `async_forward_entry_setup` method is called to reload them.

**Outcome:**  
By loading components sequentially and ensuring that unnecessary setups are skipped, we significantly reduce the CPU load while maintaining efficient processing of new Alexa clients.
@jleinenbach jleinenbach marked this pull request as draft October 3, 2024 18:53
Copy link

@MichaelMKKelly MichaelMKKelly left a comment

Choose a reason for hiding this comment

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

line 1541 needs a ) removed from the end to fix a syntax error

danielbrunt57 and others added 4 commits October 3, 2024 16:32
Fix syntax error line 1541: unmatched ')'

Co-authored-by: Andy Barratt <[email protected]>
Line 949 s/b @_catch_login_errors
Line 1452: revert async_unload_entry parameters
@alandtse
Copy link
Owner

alandtse commented Oct 4, 2024

Please note, while I'm not opposed to what looks like ChatGPT changes, given the recent issue with #2579, I'm going to require someone actually test it before merging.

That said, I want to thank you for being willing to do PRs.

@danielbrunt57
Copy link
Collaborator

@jleinenbach Are you aware of PR #2589 to address memory exhaustion/infinite loop?

@danielbrunt57
Copy link
Collaborator

@alandtse As you're no doubt aware, I implemented @jleinenbach's original code and then amended the PR with 3 edits to address issues I encountered with it. That said, I'm running the resulting __init__.py in my system and everything here is working fine.
Also, I just now implemented the helpers.py code change in PR #2589 and everything is still working.

@dpwood
Copy link
Contributor

dpwood commented Oct 4, 2024

The problem as described by this PR doesn't fit reports of #2579.

It's not a problem of intense parallel activity. The symptom I've witnessed is sustained high CPU and increasing memory consumption until it runs out of memory and crashes.

@danielbrunt57
Copy link
Collaborator

But the helper function has never been altered so I'm trying to understand what external change has caused the helper to now be in an infinite loop. If you don't know that's fine as processing a copy of the list when the original list is going to be changed by the process is the correct way otherwise you usually end up shooting yourself in the foot!

@dpwood
Copy link
Contributor

dpwood commented Oct 4, 2024

But the helper function has never been altered so I'm trying to understand what external change has caused the helper to now be in an infinite loop. If you don't know that's fine as processing a copy of the list when the original list is going to be changed by the process is the correct way otherwise you usually end up shooting yourself in the foot!

It was altered. Foot shooting code was added in this release:

6f9a92d#diff-4e8bc1554181c711fd75da33fbb2b0e7a8a1abd5096c723a1d3baa9f3c10543c

@alandtse
Copy link
Owner

alandtse commented Oct 5, 2024

Went with #2589

@alandtse alandtse closed this Oct 5, 2024
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.

6 participants