Invalidate font manager when rcParam family lists change. #3077
Conversation
|
If this works, I'd advocate merging this *instead of * #3074, as this (hopefully) fixes the underlying problem. |
|
@mdboom, sorry, I was too quick to merge #3074. |
|
I actually spent some time investigating the first option (generating a global hash of the rcParams when they change) but there are an awful lot of entry points to where rcParams can change (reloading from a file, the context manager, changing a value directly), and this wasn't really baked in from the start. Plus, there seems to be no reason to invalidate the cache if an unrelated parameter has changed. |
|
@mdboom, Agreed; I was just trying to avoid remaking the rcParams key every time. In fact, I don't have a clear picture of how often the lookup occurs; the overhead of remaking the rcParams key might be utterly negligible. I suspect it is. |
| """ | ||
| # A list of rcparam names that, when changed, invalidated this | ||
| # cache. | ||
| invalidating_rcparams = [ |
There was a problem hiding this comment.
might want to make this a set for trivial performance boost (<30ns per lookup)
There was a problem hiding this comment.
@tacaswell, actually, it looks like a tuple is as good or better; but the larger gain--still trivial, about 300 ns out of 1.2 microseconds in a simplified test--is from replacing the key generation loop with a list comprehension:
key = [id(fontManager)] + [rcParams[param] for param in self.invalidating_rcparams]There was a problem hiding this comment.
Cool. I'll make these changes.
|
I'm going to restart the build, to make sure this wasn't a fluke ;) |
|
Looks good. Now, it's just that pesky CHANGELOG entry... |
According to the data model (https://docs.python.org/2/reference/datamodel.html#object.__hash__) objects that define __hash__ need to define __eq__ or __cmp__. By default, all user defined classes have a __cmp__ which evaluates to False for all other objects. Previously we do not define either __cmp__ or __eq__ on FontProperties, This results in never finding the property cached in the dict, hence the growth in the number of FontProperties (and I assume the stuff in them). By adding __eq__ and __ne__ we complete the data model and adding FontProperties to dictionaries should work as expected. This was not a problem before, but in matplotlib#3077 the caching key was changed from hash(prop) -> prop. Closes matplotlib#3264
According to the data model (https://docs.python.org/2/reference/datamodel.html#object.__hash__) objects that define __hash__ need to define __eq__ or __cmp__. By default, all user defined classes have a __cmp__ which evaluates to False for all other objects. Previously we do not define either __cmp__ or __eq__ on FontProperties, This results in never finding the property cached in the dict, hence the growth in the number of FontProperties (and I assume the stuff in them). By adding __eq__ and __ne__ we complete the data model and adding FontProperties to dictionaries should work as expected. This was not a problem before, but in matplotlib#3077 the caching key was changed from hash(prop) -> prop. Closes matplotlib#3264
According to the data model (https://docs.python.org/2/reference/datamodel.html#object.__hash__) objects that define __hash__ need to define __eq__ or __cmp__. By default, all user defined classes have a __cmp__ which evaluates to False for all other objects. Previously we do not define either __cmp__ or __eq__ on FontProperties, This results in never finding the property cached in the dict, hence the growth in the number of FontProperties (and I assume the stuff in them). By adding __eq__ and __ne__ we complete the data model and adding FontProperties to dictionaries should work as expected. This was not a problem before, but in matplotlib#3077 the caching key was changed from hash(prop) -> prop. Closes matplotlib#3264
A possible fix for #3074 and related issues.