PERF: Event emissions and perf regression.

[Carreau]

Working on #5263, I came across this weird things where a lot of time is spent checking the non-equality of two empty dict, that actually are the same instance.

It does create a 20% improvement in TextManager.__init__, which apply directly trigger the event mechanism:

def apply(self, features: Any):
    self.string._apply(features)
    # this line is a call to __call__ and represent ~99% of time
    # spent in this method regardless of before/after patch.
    self.events.values()
    self.color._apply(features)

I'm not sure why the interpreter does not check for identity first. I'm adding the

After patch:

(HEAD) $ time python -m kernprof -l -v bench.py
Wrote profile results to bench.py.lprof
Timer unit: 1e-06 s

Total time: 0.001366 s
File: /Users/bussonniermatthias/dev/napari/napari/layers/utils/text_manager.py
Function: __init__ at line 89

Line #      Hits         Time  Per Hit   % Time  Line Contents
==============================================================
class TextManager(EventedModel)
    89                                               @profile
    90                                               def __init__(
    91                                                   self, text=None, properties=None, n_text=None, features=None, **kwargs
    92                                               ):
    93         1          1.0      1.0      0.1          if ...:
    94                                                       ...
    95         1          0.0      0.0      0.0          if ...:
    96                                                       ...
    97                                                       ...
    98                                                   else:
    99         1        114.0    114.0      8.3              features = _validate_features(features)
   100         1          1.0      1.0      0.1          if ...:
   101                                                       ...
   102                                                       ...
   103                                                       ...
   104                                                           ...
   105         1          0.0      0.0      0.0          if ...:
   106                                                       _warn_about_deprecated_text_parameter()
   107                                                       kwargs['string'] = text
   108         1       1224.0   1224.0     89.6          super().__init__(**kwargs)
   109         1         11.0     11.0      0.8          self.events.add(values=Event)
   110         1         15.0     15.0      1.1          self.apply(features)

==============================================================
and  in __call__:

   766         5          2.0      0.4      2.9              self._emitting = False
   767         5          6.0      1.2      8.6              ps = event._pop_source()
   768         5          5.0      1.0      7.1              if ps is not self.source :
   769                                                           if ps != self.source:
   770                                                               raise RuntimeError(
   771                                                                   trans._(
   772                                                                       "Event source-stack mismatch.",
   773                                                                       deferred=True,
   774                                                                   )
   775                                                               )

And master baseline for info:

(|nap6) napari[main ✗]  $ time python -m kernprof -l -v bench.py
Wrote profile results to bench.py.lprof
Timer unit: 1e-06 s

Total time: 0.001803 s
File: /Users/bussonniermatthias/dev/napari/napari/layers/utils/text_manager.py
Function: __init__ at line 89

Line #      Hits         Time  Per Hit   % Time  Line Contents
==============================================================
    89                                               @profile
    90                                               def __init__(
    91                                                   self, text=None, properties=None, n_text=None, features=None, **kwargs
    92                                               ):
    93         1          1.0      1.0      0.1          if ...:
    94                                                       ...
    95         1          0.0      0.0      0.0          if ...:
    96                                                       ...
    97                                                       ...
    98                                                   else:
    99         1        112.0    112.0      6.2              features = _validate_features(features)
   100         1          0.0      0.0      0.0          if ...:
   101                                                       ...
   102                                                       ...
   103                                                       if 'string' not in kwargs:
   104                                                           ...
   105         1          1.0      1.0      0.1          if ...:
   106                                                       ...
   107                                                       ...
   108         1       1268.0   1268.0     70.3          super().__init__(**kwargs)
   109         1         11.0     11.0      0.6          self.events.add(values=Event)
   110         1        410.0    410.0     22.7          self.apply(features)

==============================================================
and in __call__:

   767         5        441.0     88.2     87.3              if event._pop_source() != self.source:
   768                                                           raise RuntimeError(
   769                                                               trans._(
   770                                                                   "Event source-stack mismatch.",
   771                                                                   deferred=True,
   772                                                               )
   773                                                           )

python -m kernprof -l -v bench.py  1.35s user 0.61s system 113% cpu 1.721 total

I'd love for someone to confirm I"m not crazy and Python does not text for identity.

And here is my bench script, from the slow benchmarks:

from napari.layers.utils.text_manager import TextManager
import numpy as np

import pandas as pd

categories = ('cat', 'car')
n= 64

features = pd.DataFrame(
            {
                'string_property': pd.Series(
                    np.random.choice(categories, n),
                    dtype=pd.CategoricalDtype(categories),
                ),
                'float_property': np.random.rand(n),
            }
        )

def f():
    string = {'constant': 'test'}
    TextManager(string=string, features=features)
f()

[codecov]

Codecov Report

Merging #5307 (72d0cd9) into main (1cc90a7) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5307      +/-   ##
==========================================
+ Coverage   89.08%   89.09%   +0.01%     
==========================================
  Files         581      581              
  Lines       49233    49236       +3     
==========================================
+ Hits        43857    43869      +12     
+ Misses       5376     5367       -9     

Impacted Files	Coverage Δ
napari/utils/events/event.py	86.74% <100.00%> (+0.02%)	⬆️
napari/utils/events/evented_model.py	94.15% <100.00%> (+0.07%)	⬆️
napari/utils/theme.py	89.79% <0.00%> (+0.68%)	⬆️
napari/utils/info.py	83.33% <0.00%> (+1.11%)	⬆️
napari/_qt/__init__.py	55.17% <0.00%> (+6.89%)	⬆️
napari/components/experimental/chunk/_pool.py	93.65% <0.00%> (+7.93%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Czaki]

but the comparison is of two instances of TextManager, not empty dicts.

[Carreau]

• but the comparison is of two instances of TextManager, not empty dicts.

Sure, but I still came across this while it was comparing empty dicts due to register_theme.
And optimising that had a non negligible impact to compare TextManager instances. And even with TextManager instances, I'm not sure why it does skip identity check...

This is also why I added the profile of TextManager.__init__, to show that it improves that.

[Czaki]

Sure, but I still came across this while it was comparing empty dicts due to register_theme.

I do not understand. Where is a problem with comparing empty dicts? I do not see it in the profiling output.

And optimising that had a non negligible impact to compare TextManager instances. And even with TextManager instances, I'm not sure why it does skip identity check...

Because it is how __eq__ operator is implemented:

napari/napari/utils/events/evented_model.py

Lines 325 to 345 in 1cc90a7

	def __eq__(self, other) -> bool:
	"""Check equality with another object.
	We override the pydantic approach (which just checks
	``self.dict() == other.dict()``) to accommodate more complicated types
	like arrays, whose truth value is often ambiguous. ``__eq_operators__``
	is constructed in ``EqualityMetaclass.__new__``
	"""
	if not isinstance(other, EventedModel):
	return self.dict() == other
	for f_name, eq in self.__eq_operators__.items():
	if f_name not in other.__eq_operators__:
	return False
	if (
	hasattr(self, f_name)
	and hasattr(other, f_name)
	and not eq(getattr(self, f_name), getattr(other, f_name))
	):
	return False
	return True

I never expected that python would use is operator before calling __eq__ operator. But I may be wrong.

[Carreau]

Because it is how __eq__ operator is implemented:

I'm (was) pretty sure that is an optimisation at CPython level for ==, before even calling __eq__, or maybe I'm confusing with PyPy, or another thing like set belonging.

[Carreau]

Ah, Yes, there is a shortcut for x in [a list with x], according to https://stackoverflow.com/questions/49429308/should-i-test-for-object-identity-in-eq-function

[Czaki]

Suggested change

	if ps is not self.source:
	if ps != self.source:
	if ps is not self.source and ps != self.source:

maybe single if?

[brisvag]

Should we then simply add the identity check at the top of our __eq__ implementation, to avoid this issue in similar cases?

Yeah it seems like a useful optimization to have... On the other hand, this would break stuff like:

np.nan != np.nan

[Carreau]

Should we then simply add the identity check at the top of our __eq__ implementation

Problem is that there are many sources, and you would need to add the identity check everywhere. In the case of nan, it's a problem, but I'm unsure nan can emit events. The other questions is do we actually want to check for equality here, or are we actually looking for identity ? It looks to me that really we are checking that the last remaining source is the object emitting the event, so it might be that is is actually correct while == is not...

[brisvag]

Problem is that there are many sources, and you would need to add the identity check everywhere.

Not if the problem is the slowness of EventedModel's eq. You can just add it at the top of that eq, and at least you cut down that cost, and you get the benefit in every other place where we might do model == model.

In the case of nan, it's a problem, but I'm unsure nan can emit events.

Sorry, I wasn't clear. I meant that if the interpreter used this shortcut by default, it would not allow things like nan where is returns True, but == returns False.

The other questions is do we actually want to check for equality here, or are we actually looking for identity ? It looks to me that really we are checking that the last remaining source is the object emitting the event, so it might be that is is actually correct while == is not...

Actually, I think you're right. Either way, I think the addition to __eq__ I suggested above would still be useful for other cases.

[Carreau]

Ok, I've move the instance test to the EventedModel __eq__, I didn't touch non-evented model classes, maybe some of them could use the same instance testing but should be reviewed on a case by case basis.

I've also asserted that sources that don't test equal still are instances of EventedModel

[kcpevey]

@Carreau you have an approval on this, just bringing it to attention in case you forgot about it :)

[Carreau]

you have an approval on this, just bringing it to attention in case you forgot about it :)

I try to always avoid self-merge, and try to follow the policy that only @napari/core-devs should merged – at least I was reminded some time ago I generally should not be the one merging PRs, so I'm not merging.