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

chore: Improve setup performance #401

Merged
merged 14 commits into from Oct 6, 2023
Merged

Conversation

fgmacedo
Copy link
Owner

@fgmacedo fgmacedo commented Aug 19, 2023

Related:

This is the initial profiling for reference:
combined_initial

Benchmark with develop

Benchmark the current iteration: Setup 20% faster than develop.

To reproduce, run:

pytest --profile-svg --benchmark-compare=0001
---------------------------------------------------------------------------------------------------- benchmark: 2 tests ----------------------------------------------------------------------------------------------------
Name (time in us)                                Min                   Max                  Mean              StdDev                Median                 IQR            Outliers         OPS            Rounds  Iterations
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
test_setup_performance (NOW)                560.9685 (1.0)        623.3466 (1.0)        581.4283 (1.0)       20.5187 (1.0)        575.3756 (1.0)       17.5415 (1.0)           2;1  1,719.9025 (1.0)          10        1000
test_setup_performance (0001_be10b53)     6,016.6518 (10.73)    6,557.5370 (10.52)    6,255.2341 (10.76)    213.6017 (10.41)    6,149.6845 (10.69)    366.7481 (20.91)         3;0    159.8661 (0.09)         10         700
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

combined

@ghost
Copy link

ghost commented Aug 19, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@fgmacedo fgmacedo changed the title chore: Setup of pytest-profiling and pytest-benchmark chore: Improve setup performance Aug 19, 2023
@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (be10b53) 100.00% compared to head (3c0f804) 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           develop      #401    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           20        20            
  Lines          995      1101   +106     
  Branches       165       175    +10     
==========================================
+ Hits           995      1101   +106     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
statemachine/callbacks.py 100.00% <100.00%> (ø)
statemachine/dispatcher.py 100.00% <100.00%> (ø)
statemachine/signature.py 100.00% <100.00%> (ø)
statemachine/state.py 100.00% <100.00%> (ø)
statemachine/statemachine.py 100.00% <100.00%> (ø)
statemachine/transition.py 100.00% <100.00%> (ø)
statemachine/utils.py 100.00% <100.00%> (ø)

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

@sonarcloud
Copy link

sonarcloud bot commented Aug 29, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Oct 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@fgmacedo fgmacedo marked this pull request as ready for review October 3, 2023 20:06
default_resolver = resolver_factory(machine, model)

# clone states and transitions to avoid sharing callbacks references between instances
self.states = States(
{s.id: s.clone()._setup(self, default_resolver) for s in class_states}
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is the main change on this PR: Instead of cloning states to get isolated SM instances, we now register the callbacks references on the SM instance. So States and Transitions are kind of immutable at SM runtime.

@@ -70,6 +71,8 @@ def __init__(
self.__rtc = rtc
self.__processing: bool = False
self._external_queue: deque = deque()
self._callbacks_registry = CallbacksRegistry()
Copy link
Owner Author

@fgmacedo fgmacedo Oct 4, 2023

Choose a reason for hiding this comment

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

This new _callbacks_registry keeps the reference for all callback methods used in actions, conditions, and guards.

from .exceptions import AttrNotFound
from .exceptions import InvalidDefinition
from .i18n import _
from .utils import ensure_iterable


class CallbackWrapper:
Copy link
Owner Author

Choose a reason for hiding this comment

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

The CallbackWrapper is now only about concrete callback references, and all the metadata attributes are decomposed into a new CallbackMeta.

machine.__dict__[self._storage] = self
def _setup(self, register):
register(self.enter)
register(self.exit)
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is now like a "visitor" pattern.

@@ -208,6 +213,59 @@ def initial(self):
def final(self):
return self._final


class InstanceState(State):
Copy link
Owner Author

Choose a reason for hiding this comment

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

A thin "proxy" for State that has is_active with a reference to a concrete instance of the SM.


after("after_transition")
def _setup(self, register):
register(self.validators)
Copy link
Owner Author

Choose a reason for hiding this comment

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

This is now like a visitor pattern.

@fgmacedo fgmacedo merged commit 63d2048 into develop Oct 6, 2023
14 checks passed
@fgmacedo fgmacedo deleted the macedo/improve-setup-speed branch October 6, 2023 13:21
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.

Action callback gets called twice when mixing definition methods Expensive instantiation of StateMachine
1 participant