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

events.py: enhance efficiency of clear() by eliminating full dictionary rebuild #33521

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

deanlee
Copy link
Contributor

@deanlee deanlee commented Sep 9, 2024

The current clear() method rebuilds the entire dictionary from the keys in EVENTS on each call, resulting in inefficient performance. For example, in selfdrived, clear() is among the top five most time-consuming operations, with its execution time just behind SelfdriveD.publish_selfdriveState(self, CS), which is unreasonable:


     5000    0.192    0.000    0.467    0.000 selfdrived.py:415(publish_selfdriveState)
     5000    0.042    0.000    0.324    0.000 events.py:68(clear)

After this PR, the execution time of clear() has been reduced from 0.324 to 0.0311, making it approximately 10 times faster. This improvement should enhance the runtime efficiency of both selfdrived and card.

Changes:

  1. Added self.prev_event_set to track previously active events.
  2. Refactored clear() to use set operations for efficient counter reset instead of rebuilding the entire dictionary.
  3. Updated add() method to increment event counters directly.

@sshane
Copy link
Contributor

sshane commented Sep 9, 2024

trigger-jenkins

@deanlee deanlee force-pushed the event_improve_clear_efficiency branch from 85f67ee to abffd51 Compare September 10, 2024 19:29
@deanlee
Copy link
Contributor Author

deanlee commented Sep 10, 2024

Using defaultdict in the Events class is more efficient, especially since Events() may be instantiated frequently, such as in functions like create_common_events(). For example, the Events instance is created on every loop in card:


 ncalls  tottime  percall  cumtime  percall filename:lineno(function)
 4999    0.017    0.000    0.077    0.000 events.py:51(__init__)

Initializing counters from EVENTS in the __init__ method provides no real benefit and results in unnecessary CPU usage. Switching to defaultdict streamlines the process, reducing overhead by eliminating unnecessary initialization steps.

def clear(self) -> None:
self.event_counters = {k: (v + 1 if k in self.events else 0) for k, v in self.event_counters.items()}
# Get events no longer active
Copy link
Contributor

@sshane sshane Sep 10, 2024

Choose a reason for hiding this comment

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

This looks immediately more confusing to read, anything else we can try? Why not one loop and an if?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A more common and readable approach is to loop through event_counters and check if event_name is not in the events list. The code snippet would look like this:

for event_name in self.event_counters:
    if event_name not in self.events:
        self.event_counters[event_name] = 0

However, this approach is significantly less efficient than the version in this PR and might not offer much improvement over the master version. I'll consider if there's a better solution.

Copy link
Contributor Author

@deanlee deanlee Oct 26, 2024

Choose a reason for hiding this comment

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

I can't think of a better solution for now. I've improved the variable name and comments to make the code easier to understand. The logic is straightforward: maintain a list of previously active events, compare it with the current active events, and remove those that are no longer active.

@deanlee deanlee marked this pull request as draft September 10, 2024 19:55
Copy link
Contributor

This PR has had no activity for 9 days. It will be automatically closed in 2 days if there is no activity.

@github-actions github-actions bot added the stale label Sep 20, 2024
@deanlee deanlee force-pushed the event_improve_clear_efficiency branch from abffd51 to e1bcd22 Compare September 20, 2024 05:18
@github-actions github-actions bot removed the stale label Sep 21, 2024
Copy link
Contributor

This PR has had no activity for 9 days. It will be automatically closed in 2 days if there is no activity.

@github-actions github-actions bot added the stale label Sep 30, 2024
@deanlee deanlee force-pushed the event_improve_clear_efficiency branch from e1bcd22 to 7506f17 Compare September 30, 2024 03:03
@github-actions github-actions bot removed the stale label Oct 1, 2024
Copy link
Contributor

This PR has had no activity for 9 days. It will be automatically closed in 2 days if there is no activity.

@github-actions github-actions bot added the stale label Oct 11, 2024
Copy link
Contributor

This PR has been automatically closed due to inactivity. Feel free to re-open once activity resumes.

@github-actions github-actions bot closed this Oct 13, 2024
@adeebshihadeh adeebshihadeh reopened this Oct 13, 2024
@deanlee deanlee force-pushed the event_improve_clear_efficiency branch from 7506f17 to 85a6b37 Compare October 13, 2024 04:38
@github-actions github-actions bot removed the stale label Oct 14, 2024
Copy link
Contributor

This PR has had no activity for 9 days. It will be automatically closed in 2 days if there is no activity.

@github-actions github-actions bot added the stale label Oct 24, 2024
@deanlee deanlee marked this pull request as ready for review October 26, 2024 08:59
@deanlee deanlee force-pushed the event_improve_clear_efficiency branch from 59f9e47 to 33d0e79 Compare October 26, 2024 09:00
@adeebshihadeh
Copy link
Contributor

trigger-jenkins

@github-actions github-actions bot removed the stale label Oct 27, 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.

3 participants