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

Avoid set several times HACachedStates #56

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

bgoncal
Copy link
Member

@bgoncal bgoncal commented Mar 18, 2024

No description provided.

@bgoncal bgoncal self-assigned this Mar 18, 2024
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.87%. Comparing base (48e36a0) to head (950b1e9).

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #56   +/-   ##
=======================================
  Coverage   99.87%   99.87%           
=======================================
  Files          50       50           
  Lines        1593     1595    +2     
=======================================
+ Hits         1591     1593    +2     
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

if let additions = info.incoming.add {
for (entityId, updates) in additions {
if var currentState = states[entityId] {
if var currentState = updatedEntities.first(where: { $0.entityId == entityId }),
let index = updatedEntities.firstIndex(of: currentState) {
Copy link
Member

Choose a reason for hiding this comment

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

This is now pretty bad at performance - at least two O(N) added on top of another O(N) enumeration. It's also going to require another step to segment by dictionary again at the end, which is a lot of overhead for all of these changes.

I think it may be better to make the 'all' access lazy, or give HACachedStates an initializer that takes a dictionary, or something along those lines.

Copy link
Member Author

@bgoncal bgoncal Mar 18, 2024

Choose a reason for hiding this comment

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

Shame on me, I made it worse than before :P

I just updated it, I went for dealing with a dictionary for the whole process to avoid wasting by look inside an array and endup creating the cache in the end with an initializer that receives a dictionary as you said

@bgoncal bgoncal force-pushed the improve-process-update-performance branch 2 times, most recently from 931f518 to 91a2fcc Compare March 18, 2024 19:22
@bgoncal bgoncal force-pushed the improve-process-update-performance branch from 91a2fcc to 57d4c61 Compare March 18, 2024 19:27
Source/Caches/HACachedStates.swift Outdated Show resolved Hide resolved
Source/Caches/HACachedStates.swift Outdated Show resolved Hide resolved
@bgoncal bgoncal force-pushed the improve-process-update-performance branch from c4f6d6f to 950b1e9 Compare March 19, 2024 09:30
@bgoncal bgoncal merged commit 8d1361d into main Mar 19, 2024
5 checks passed
@bgoncal bgoncal deleted the improve-process-update-performance branch March 19, 2024 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants