From 168c10448b5142654b3e0be8fa2c21388d7e5b29 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 7 Apr 2015 23:59:36 +0200 Subject: [PATCH 01/10] models: Use copy.copy for creating copies --- mopidy/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index 45961f30..4f8ae6fe 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -1,5 +1,6 @@ from __future__ import absolute_import, unicode_literals +import copy import json # TODO: split into base models, serialization and fields? @@ -207,8 +208,7 @@ class ImmutableObject(object): :type values: dict :rtype: new instance of the model being copied """ - other = self.__class__() - other.__dict__.update(self.__dict__) + other = copy.copy(self) for key, value in values.items(): if key not in self._fields: raise TypeError( From 08fd99ffdbe73e9a005351e14600bc243c3e588e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 8 Apr 2015 00:11:23 +0200 Subject: [PATCH 02/10] models: Add Identifer type which interns values. This gives some moderate memory saving since the values are used in multiple places. --- mopidy/models.py | 25 +++++++++++++++---------- tests/models/test_models.py | 20 ++++++++++---------- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index 4f8ae6fe..e936e22c 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -75,6 +75,11 @@ class String(Field): super(String, self).__init__(type=basestring, default=default) +class Identifier(String): + def validate(self, value): + return intern(str(super(Identifier, self).validate(value))) + + class Integer(Field): """ @@ -290,7 +295,7 @@ class Ref(ImmutableObject): """ #: The object URI. Read-only. - uri = String() + uri = Identifier() #: The object name. Read-only. name = String() @@ -354,7 +359,7 @@ class Image(ImmutableObject): """ #: The image URI. Read-only. - uri = String() + uri = Identifier() #: Optional width of the image or :class:`None`. Read-only. width = Integer(min=0) @@ -375,13 +380,13 @@ class Artist(ImmutableObject): """ #: The artist URI. Read-only. - uri = String() + uri = Identifier() #: The artist name. Read-only. name = String() #: The MusicBrainz ID of the artist. Read-only. - musicbrainz_id = String() + musicbrainz_id = Identifier() class Album(ImmutableObject): @@ -406,7 +411,7 @@ class Album(ImmutableObject): """ #: The album URI. Read-only. - uri = String() + uri = Identifier() #: The album name. Read-only. name = String() @@ -424,7 +429,7 @@ class Album(ImmutableObject): date = String() # TODO: add date type #: The MusicBrainz ID of the album. Read-only. - musicbrainz_id = String() + musicbrainz_id = Identifier() #: The album image URIs. Read-only. images = Collection(type=basestring, container=frozenset) @@ -469,7 +474,7 @@ class Track(ImmutableObject): """ #: The track URI. Read-only. - uri = String() + uri = Identifier() #: The track name. Read-only. name = String() @@ -508,7 +513,7 @@ class Track(ImmutableObject): comment = String() #: The MusicBrainz ID of the track. Read-only. - musicbrainz_id = String() + musicbrainz_id = Identifier() #: Integer representing when the track was last modified. Exact meaning #: depends on source of track. For local files this is the modification @@ -571,7 +576,7 @@ class Playlist(ImmutableObject): """ #: The playlist URI. Read-only. - uri = String() + uri = Identifier() #: The playlist name. Read-only. name = String() @@ -607,7 +612,7 @@ class SearchResult(ImmutableObject): """ # The search result URI. Read-only. - uri = String() + uri = Identifier() # The tracks matching the search query. Read-only. tracks = Collection(type=Track, container=tuple) diff --git a/tests/models/test_models.py b/tests/models/test_models.py index 77383a6e..7e0d6bb9 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -86,7 +86,7 @@ class RefTest(unittest.TestCase): def test_repr_without_results(self): self.assertEqual( - "Ref(name=u'foo', type=u'artist', uri=u'uri')", + "Ref(name=u'foo', type=u'artist', uri='uri')", repr(Ref(uri='uri', name='foo', type='artist'))) def test_serialize_without_results(self): @@ -200,7 +200,7 @@ class ArtistTest(unittest.TestCase): def test_repr(self): self.assertEqual( - "Artist(name=u'name', uri=u'uri')", + "Artist(name=u'name', uri='uri')", repr(Artist(uri='uri', name='name'))) def test_serialize(self): @@ -365,12 +365,12 @@ class AlbumTest(unittest.TestCase): def test_repr_without_artists(self): self.assertEqual( - "Album(name=u'name', uri=u'uri')", + "Album(name=u'name', uri='uri')", repr(Album(uri='uri', name='name'))) def test_repr_with_artists(self): self.assertEqual( - "Album(artists=[Artist(name=u'foo')], name=u'name', uri=u'uri')", + "Album(artists=[Artist(name=u'foo')], name=u'name', uri='uri')", repr(Album(uri='uri', name='name', artists=[Artist(name='foo')]))) def test_serialize_without_artists(self): @@ -609,12 +609,12 @@ class TrackTest(unittest.TestCase): def test_repr_without_artists(self): self.assertEqual( - "Track(name=u'name', uri=u'uri')", + "Track(name=u'name', uri='uri')", repr(Track(uri='uri', name='name'))) def test_repr_with_artists(self): self.assertEqual( - "Track(artists=[Artist(name=u'foo')], name=u'name', uri=u'uri')", + "Track(artists=[Artist(name=u'foo')], name=u'name', uri='uri')", repr(Track(uri='uri', name='name', artists=[Artist(name='foo')]))) def test_serialize_without_artists(self): @@ -844,7 +844,7 @@ class TlTrackTest(unittest.TestCase): def test_repr(self): self.assertEqual( - "TlTrack(tlid=123, track=Track(uri=u'uri'))", + "TlTrack(tlid=123, track=Track(uri='uri'))", repr(TlTrack(tlid=123, track=Track(uri='uri')))) def test_serialize(self): @@ -977,12 +977,12 @@ class PlaylistTest(unittest.TestCase): def test_repr_without_tracks(self): self.assertEqual( - "Playlist(name=u'name', uri=u'uri')", + "Playlist(name=u'name', uri='uri')", repr(Playlist(uri='uri', name='name'))) def test_repr_with_tracks(self): self.assertEqual( - "Playlist(name=u'name', tracks=[Track(name=u'foo')], uri=u'uri')", + "Playlist(name=u'name', tracks=[Track(name=u'foo')], uri='uri')", repr(Playlist(uri='uri', name='name', tracks=[Track(name='foo')]))) def test_serialize_without_tracks(self): @@ -1114,7 +1114,7 @@ class SearchResultTest(unittest.TestCase): def test_repr_without_results(self): self.assertEqual( - "SearchResult(uri=u'uri')", + "SearchResult(uri='uri')", repr(SearchResult(uri='uri'))) def test_serialize_without_results(self): From 0fee1b4b1133eaa01d45cbce49f131d1f359adf3 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 8 Apr 2015 00:30:53 +0200 Subject: [PATCH 03/10] models: Switch to slots to reduce memory usage per instance --- mopidy/models.py | 33 ++++++++++++++++++++++----------- tests/models/test_fields.py | 13 +++++-------- tests/models/test_models.py | 2 +- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index e936e22c..35f1f9eb 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -45,7 +45,7 @@ class Field(object): def __get__(self, instance, owner): if not instance: return self - return instance.__dict__.get(self._name, self._default) + return getattr(instance, '_' + self._name, self._default) def __set__(self, instance, value): if value is not None: @@ -54,10 +54,11 @@ class Field(object): if value is None or value == self._default: self.__delete__(instance) else: - instance.__dict__[self._name] = value + setattr(instance, '_' + self._name, value) def __delete__(self, instance): - instance.__dict__.pop(self._name, None) + if hasattr(instance, '_' + self._name): + delattr(instance, '_' + self._name) class String(Field): @@ -134,12 +135,14 @@ class FieldOwner(type): """Helper to automatically assign field names to descriptors.""" def __new__(cls, name, bases, attrs): - attrs['_fields'] = [] + fields = {} for key, value in attrs.items(): if isinstance(value, Field): - attrs['_fields'].append(key) + fields[key] = '_' + key value._name = key - attrs['_fields'].sort() + + attrs['_fields'] = fields + attrs['__slots__'] = ['_' + field for field in fields] return super(FieldOwner, cls).__new__(cls, name, bases, attrs) @@ -165,14 +168,23 @@ class ImmutableObject(object): super(ImmutableObject, self).__setattr__(key, value) def __setattr__(self, name, value): + if name in self.__slots__: + return super(ImmutableObject, self).__setattr__(name, value) raise AttributeError('Object is immutable.') def __delattr__(self, name): + if name in self.__slots__: + return super(ImmutableObject, self).__delattr__(name) raise AttributeError('Object is immutable.') + def _items(self): + for field, key in self._fields.items(): + if hasattr(self, key): + yield field, getattr(self, key) + def __repr__(self): kwarg_pairs = [] - for (key, value) in sorted(self.__dict__.items()): + for key, value in sorted(self._items()): if isinstance(value, (frozenset, tuple)): if not value: continue @@ -185,15 +197,14 @@ class ImmutableObject(object): def __hash__(self): hash_sum = 0 - for key, value in self.__dict__.items(): + for key, value in self._items(): hash_sum += hash(key) + hash(value) return hash_sum def __eq__(self, other): if not isinstance(other, self.__class__): return False - - return self.__dict__ == other.__dict__ + return dict(self._items()) == dict(other._items()) def __ne__(self, other): return not self.__eq__(other) @@ -224,7 +235,7 @@ class ImmutableObject(object): def serialize(self): data = {} data['__model__'] = self.__class__.__name__ - for key, value in self.__dict__.items(): + for key, value in self._items(): if isinstance(value, (set, frozenset, list, tuple)): value = [ v.serialize() if isinstance(v, ImmutableObject) else v diff --git a/tests/models/test_fields.py b/tests/models/test_fields.py index 5347b83c..ff863487 100644 --- a/tests/models/test_fields.py +++ b/tests/models/test_fields.py @@ -29,15 +29,14 @@ class FieldDescriptorTest(unittest.TestCase): instance = create_instance(Field()) self.assertIsNone(instance.attr) - def test_field_does_not_store_default_in_dict(self): + def test_field_does_not_store_default(self): instance = create_instance(Field()) - self.assertNotIn('attr', instance.__dict__) + self.assertFalse(hasattr(instance, '_attr')) def test_field_assigment_and_retrival(self): instance = create_instance(Field()) instance.attr = 1234 self.assertEqual(1234, instance.attr) - self.assertEqual(1234, instance.__dict__['attr']) def test_field_can_be_reassigned(self): instance = create_instance(Field()) @@ -50,14 +49,14 @@ class FieldDescriptorTest(unittest.TestCase): instance.attr = 1234 del instance.attr self.assertEqual(None, instance.attr) - self.assertNotIn('attr', instance.__dict__) + self.assertFalse(hasattr(instance, '_attr')) def test_field_can_be_set_to_none(self): instance = create_instance(Field()) instance.attr = 1234 instance.attr = None self.assertEqual(None, instance.attr) - self.assertNotIn('attr', instance.__dict__) + self.assertFalse(hasattr(instance, '_attr')) def test_field_can_be_set_default(self): default = object() @@ -65,7 +64,7 @@ class FieldDescriptorTest(unittest.TestCase): instance.attr = 1234 instance.attr = default self.assertEqual(default, instance.attr) - self.assertNotIn('attr', instance.__dict__) + self.assertFalse(hasattr(instance, '_attr')) class FieldTest(unittest.TestCase): @@ -183,13 +182,11 @@ class CollectionTest(unittest.TestCase): instance = create_instance(Collection(type=int, container=frozenset)) instance.attr = [] self.assertEqual(frozenset(), instance.attr) - self.assertNotIn('attr', instance.__dict__) def test_collection_gets_stored_in_container(self): instance = create_instance(Collection(type=int, container=frozenset)) instance.attr = [1, 2, 3] self.assertEqual(frozenset([1, 2, 3]), instance.attr) - self.assertEqual(frozenset([1, 2, 3]), instance.__dict__['attr']) def test_collection_with_wrong_type(self): instance = create_instance(Collection(type=int, container=frozenset)) diff --git a/tests/models/test_models.py b/tests/models/test_models.py index 7e0d6bb9..8f72dd34 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -55,7 +55,7 @@ class GenericCopyTest(unittest.TestCase): def test_copying_track_to_remove(self): track = Track(name='foo').copy(name=None) - self.assertEqual(track.__dict__, Track().__dict__) + self.assertFalse(hasattr(track, '_name')) class RefTest(unittest.TestCase): From 86481b1d504ee1f25ac1a8a5dadbe58ee8e12161 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 8 Apr 2015 00:34:02 +0200 Subject: [PATCH 04/10] models: Shortcut case where copy didn't change anything We no longer copy in this case and will just give you the same instance back. --- mopidy/models.py | 2 ++ tests/models/test_models.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/mopidy/models.py b/mopidy/models.py index 35f1f9eb..af409570 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -224,6 +224,8 @@ class ImmutableObject(object): :type values: dict :rtype: new instance of the model being copied """ + if not values: + return self other = copy.copy(self) for key, value in values.items(): if key not in self._fields: diff --git a/tests/models/test_models.py b/tests/models/test_models.py index 8f72dd34..ef484fa9 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -12,7 +12,7 @@ class GenericCopyTest(unittest.TestCase): def compare(self, orig, other): self.assertEqual(orig, other) - self.assertNotEqual(id(orig), id(other)) + self.assertEqual(id(orig), id(other)) def test_copying_track(self): track = Track() From dd270ab87b87239fe4d42c0f851a5d9472ce75b9 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 8 Apr 2015 00:36:03 +0200 Subject: [PATCH 05/10] models: Stop using globals to get model names in JSON decoding. --- mopidy/models.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index af409570..c12da719 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -282,13 +282,13 @@ def model_json_decoder(dct): """ if '__model__' in dct: + models = {c.__name__: c for c in ImmutableObject.__subclasses__()} model_name = dct.pop('__model__') - cls = globals().get(model_name, None) - if issubclass(cls, ImmutableObject): + if model_name in models: kwargs = {} for key, value in dct.items(): kwargs[key] = value - return cls(**kwargs) + return models[model_name](**kwargs) return dct From b7375323e902d0dbbb0ff1aea75753a616a037cc Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 8 Apr 2015 01:14:56 +0200 Subject: [PATCH 06/10] models: Memoize identical instances automatically This combined with the previous changes has brought the memory use for a 14k track test-set down from about 75MB to 17MB or so. Note that this does however, mean that copy is now lying to us as it does not such thing whenever it can avoid it. --- mopidy/models.py | 23 ++++++++++++++++++----- tests/models/test_fields.py | 2 +- tests/models/test_models.py | 16 ++++++++++++++++ 3 files changed, 35 insertions(+), 6 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index c12da719..97b524ea 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -2,6 +2,7 @@ from __future__ import absolute_import, unicode_literals import copy import json +import weakref # TODO: split into base models, serialization and fields? @@ -24,7 +25,7 @@ class Field(object): """ def __init__(self, default=None, type=None, choices=None): - self._name = None # Set by FieldOwner + self._name = None # Set by ImmutableObjectMeta self._choices = choices self._default = default self._type = type @@ -130,7 +131,7 @@ class Collection(Field): return self._default.__class__(value) or None -class FieldOwner(type): +class ImmutableObjectMeta(type): """Helper to automatically assign field names to descriptors.""" @@ -142,8 +143,20 @@ class FieldOwner(type): value._name = key attrs['_fields'] = fields + attrs['_instances'] = weakref.WeakValueDictionary() attrs['__slots__'] = ['_' + field for field in fields] - return super(FieldOwner, cls).__new__(cls, name, bases, attrs) + + for base in bases: + if '__weakref__' in getattr(base, '__slots__', []): + break + else: + attrs['__slots__'].append('__weakref__') + + return super(ImmutableObjectMeta, cls).__new__(cls, name, bases, attrs) + + def __call__(cls, *args, **kwargs): # noqa: N805 + instance = super(ImmutableObjectMeta, cls).__call__(*args, **kwargs) + return cls._instances.setdefault(weakref.ref(instance), instance) class ImmutableObject(object): @@ -157,7 +170,7 @@ class ImmutableObject(object): :type kwargs: any """ - __metaclass__ = FieldOwner + __metaclass__ = ImmutableObjectMeta def __init__(self, *args, **kwargs): for key, value in kwargs.items(): @@ -232,7 +245,7 @@ class ImmutableObject(object): raise TypeError( 'copy() got an unexpected keyword argument "%s"' % key) super(ImmutableObject, other).__setattr__(key, value) - return other + return self._instances.setdefault(weakref.ref(other), other) def serialize(self): data = {} diff --git a/tests/models/test_fields.py b/tests/models/test_fields.py index ff863487..1bf46b7f 100644 --- a/tests/models/test_fields.py +++ b/tests/models/test_fields.py @@ -9,7 +9,7 @@ def create_instance(field): """Create an instance of a dummy class for testing fields.""" class Dummy(object): - __metaclass__ = FieldOwner + __metaclass__ = ImmutableObjectMeta attr = field return Dummy() diff --git a/tests/models/test_models.py b/tests/models/test_models.py index ef484fa9..90701b14 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -8,6 +8,22 @@ from mopidy.models import ( TlTrack, Track, model_json_decoder) +class CachingTest(unittest.TestCase): + + def test_same_instance(self): + self.assertIs(Track(), Track()) + + def test_same_instance_with_values(self): + self.assertIs(Track(uri='test'), Track(uri='test')) + + def test_different_instance_with_different_values(self): + self.assertIsNot(Track(uri='test1'), Track(uri='test2')) + + def test_different_instance_with_copy(self): + t = Track(uri='test1') + self.assertIsNot(t, t.copy(uri='test2')) + + class GenericCopyTest(unittest.TestCase): def compare(self, orig, other): From 05244f7e60e95676898209a6c96899d74b6c982a Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 8 Apr 2015 01:41:50 +0200 Subject: [PATCH 07/10] models: Deprecate copy and add replace method Changed as with the memoization copy was lying, so replace is a better name. --- docs/api/models.rst | 5 +-- mopidy/models.py | 34 ++++++++++++++----- mopidy/utils/deprecation.py | 3 ++ tests/models/test_models.py | 66 ++++++++++++++++++------------------- 4 files changed, 65 insertions(+), 43 deletions(-) diff --git a/docs/api/models.rst b/docs/api/models.rst index 7192c284..d2d8ec0a 100644 --- a/docs/api/models.rst +++ b/docs/api/models.rst @@ -8,7 +8,7 @@ and immutable. In other words, they can only be set through the class constructor during instance creation. If you want to modify a model, use the -:meth:`~mopidy.models.ImmutableObject.copy` method. It accepts keyword +:meth:`~mopidy.models.ImmutableObject.replace` method. It accepts keyword arguments for the parts of the model you want to change, and copies the rest of the data from the model you call it on. Example:: @@ -16,7 +16,7 @@ the data from the model you call it on. Example:: >>> track1 = Track(name='Christmas Carol', length=171) >>> track1 Track(artists=[], length=171, name='Christmas Carol') - >>> track2 = track1.copy(length=37) + >>> track2 = track1.replace(length=37) >>> track2 Track(artists=[], length=37, name='Christmas Carol') >>> track1 @@ -75,6 +75,7 @@ Data model helpers ================== .. autoclass:: mopidy.models.ImmutableObject + :members: .. autoclass:: mopidy.models.Field diff --git a/mopidy/models.py b/mopidy/models.py index 97b524ea..bd449a89 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -4,6 +4,8 @@ import copy import json import weakref +from mopidy.utils import deprecation + # TODO: split into base models, serialization and fields? @@ -166,6 +168,10 @@ class ImmutableObject(object): constructor. Fields should be :class:`Field` instances to ensure type safety in our models. + Note that since these models can not be changed, we heavily memoize them + to save memory. So constructing a class with the same arguments twice will + give you the same instance twice. + :param kwargs: kwargs to set as fields on the object :type kwargs: any """ @@ -224,23 +230,35 @@ class ImmutableObject(object): def copy(self, **values): """ - Copy the model with ``field`` updated to new value. + .. deprecated:: 1.1 + Use :meth:`replace` instead. Note that we no longer return copies. + """ + deprecation.warn('model.immutable.copy') + return self.replace(**values) + + def replace(self, **kwargs): + """ + Replace the fields in the model and return a new instance Examples:: # Returns a track with a new name - Track(name='foo').copy(name='bar') + Track(name='foo').replace(name='bar') # Return an album with a new number of tracks - Album(num_tracks=2).copy(num_tracks=5) + Album(num_tracks=2).replace(num_tracks=5) - :param values: the model fields to modify - :type values: dict - :rtype: new instance of the model being copied + Note that internally we memoize heavily to keep memory usage down given + our overly repetitive data structures. So you might get an existing + instance if it contains the same values. + + :param kwargs: kwargs to set as fields on the object + :type kwargs: any + :rtype: instance of the model with replaced fields """ - if not values: + if not kwargs: return self other = copy.copy(self) - for key, value in values.items(): + for key, value in kwargs.items(): if key not in self._fields: raise TypeError( 'copy() got an unexpected keyword argument "%s"' % key) diff --git a/mopidy/utils/deprecation.py b/mopidy/utils/deprecation.py index 57042347..4f26b41b 100644 --- a/mopidy/utils/deprecation.py +++ b/mopidy/utils/deprecation.py @@ -40,6 +40,9 @@ _MESSAGES = { 'tracklist.add() "tracks" argument is deprecated', 'core.tracklist.add:uri_arg': 'tracklist.add() "uri" argument is deprecated', + + 'models.immutable.copy': + 'ImmutableObject.copy() is deprecated, use ImmutableObject.replace()', } diff --git a/tests/models/test_models.py b/tests/models/test_models.py index 90701b14..0407056c 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -19,58 +19,58 @@ class CachingTest(unittest.TestCase): def test_different_instance_with_different_values(self): self.assertIsNot(Track(uri='test1'), Track(uri='test2')) - def test_different_instance_with_copy(self): + def test_different_instance_with_replace(self): t = Track(uri='test1') - self.assertIsNot(t, t.copy(uri='test2')) + self.assertIsNot(t, t.replace(uri='test2')) -class GenericCopyTest(unittest.TestCase): +class GenericReplaceTest(unittest.TestCase): def compare(self, orig, other): self.assertEqual(orig, other) self.assertEqual(id(orig), id(other)) - def test_copying_track(self): + def test_replace_track(self): track = Track() - self.compare(track, track.copy()) + self.compare(track, track.replace()) - def test_copying_artist(self): + def test_replace_artist(self): artist = Artist() - self.compare(artist, artist.copy()) + self.compare(artist, artist.replace()) - def test_copying_album(self): + def test_replace_album(self): album = Album() - self.compare(album, album.copy()) + self.compare(album, album.replace()) - def test_copying_playlist(self): + def test_replace_playlist(self): playlist = Playlist() - self.compare(playlist, playlist.copy()) + self.compare(playlist, playlist.replace()) - def test_copying_track_with_basic_values(self): + def test_replace_track_with_basic_values(self): track = Track(name='foo', uri='bar') - copy = track.copy(name='baz') - self.assertEqual('baz', copy.name) - self.assertEqual('bar', copy.uri) + other = track.replace(name='baz') + self.assertEqual('baz', other.name) + self.assertEqual('bar', other.uri) - def test_copying_track_with_missing_values(self): + def test_replace_track_with_missing_values(self): track = Track(uri='bar') - copy = track.copy(name='baz') - self.assertEqual('baz', copy.name) - self.assertEqual('bar', copy.uri) + other = track.replace(name='baz') + self.assertEqual('baz', other.name) + self.assertEqual('bar', other.uri) - def test_copying_track_with_private_internal_value(self): + def test_replace_track_with_private_internal_value(self): artist1 = Artist(name='foo') artist2 = Artist(name='bar') track = Track(artists=[artist1]) - copy = track.copy(artists=[artist2]) - self.assertIn(artist2, copy.artists) + other = track.replace(artists=[artist2]) + self.assertIn(artist2, other.artists) - def test_copying_track_with_invalid_key(self): + def test_replace_track_with_invalid_key(self): with self.assertRaises(TypeError): - Track().copy(invalid_key=True) + Track().replace(invalid_key=True) - def test_copying_track_to_remove(self): - track = Track(name='foo').copy(name=None) + def test_replace_track_to_remove(self): + track = Track(name='foo').replace(name=None) self.assertFalse(hasattr(track, '_name')) @@ -209,7 +209,7 @@ class ArtistTest(unittest.TestCase): def test_invalid_kwarg_with_name_matching_method(self): with self.assertRaises(TypeError): - Artist(copy='baz') + Artist(replace='baz') with self.assertRaises(TypeError): Artist(serialize='baz') @@ -816,9 +816,9 @@ class TrackTest(unittest.TestCase): self.assertEqual(track1, track2) self.assertEqual(hash(track1), hash(track2)) - def test_copy_can_reset_to_default_value(self): + def test_replace_can_reset_to_default_value(self): track1 = Track(name='name1') - track2 = Track(name='name1', album=Album()).copy(album=None) + track2 = Track(name='name1', album=Album()).replace(album=None) self.assertEqual(track1, track2) self.assertEqual(hash(track1), hash(track2)) @@ -943,7 +943,7 @@ class PlaylistTest(unittest.TestCase): playlist = Playlist( uri='an uri', name='a name', tracks=tracks, last_modified=last_modified) - new_playlist = playlist.copy(uri='another uri') + new_playlist = playlist.replace(uri='another uri') self.assertEqual(new_playlist.uri, 'another uri') self.assertEqual(new_playlist.name, 'a name') self.assertEqual(list(new_playlist.tracks), tracks) @@ -955,7 +955,7 @@ class PlaylistTest(unittest.TestCase): playlist = Playlist( uri='an uri', name='a name', tracks=tracks, last_modified=last_modified) - new_playlist = playlist.copy(name='another name') + new_playlist = playlist.replace(name='another name') self.assertEqual(new_playlist.uri, 'an uri') self.assertEqual(new_playlist.name, 'another name') self.assertEqual(list(new_playlist.tracks), tracks) @@ -968,7 +968,7 @@ class PlaylistTest(unittest.TestCase): uri='an uri', name='a name', tracks=tracks, last_modified=last_modified) new_tracks = [Track(), Track()] - new_playlist = playlist.copy(tracks=new_tracks) + new_playlist = playlist.replace(tracks=new_tracks) self.assertEqual(new_playlist.uri, 'an uri') self.assertEqual(new_playlist.name, 'a name') self.assertEqual(list(new_playlist.tracks), new_tracks) @@ -981,7 +981,7 @@ class PlaylistTest(unittest.TestCase): playlist = Playlist( uri='an uri', name='a name', tracks=tracks, last_modified=last_modified) - new_playlist = playlist.copy(last_modified=new_last_modified) + new_playlist = playlist.replace(last_modified=new_last_modified) self.assertEqual(new_playlist.uri, 'an uri') self.assertEqual(new_playlist.name, 'a name') self.assertEqual(list(new_playlist.tracks), tracks) From c85323bfa0f9029f03a219ccad3d59044a7d022c Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 8 Apr 2015 01:47:57 +0200 Subject: [PATCH 08/10] docs: Add memory improvements --- docs/changelog.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index b8f4f530..3f195ff1 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -19,6 +19,11 @@ Models - Added type checks and other sanity checks to model construction and serialization. (Fixes: :issue:`865`) +- Memory usage for models has been greatly improved. We now have a lower + overhead per instance by using slots, intern identifiers and automatically + reuse instances. For the test data set this was developed against, a library + of ~14000 tracks, went from needing ~75MB to ~17MB. (Fixes: :issue:`348`) + Internal changes ---------------- From fb0e4dc7a193e4ffefd8ff4ff69e72aa0aa33323 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 8 Apr 2015 23:20:59 +0200 Subject: [PATCH 09/10] models: Assign slots from fields --- mopidy/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/models.py b/mopidy/models.py index bd449a89..3dd2c67c 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -145,8 +145,8 @@ class ImmutableObjectMeta(type): value._name = key attrs['_fields'] = fields + attrs['__slots__'] = fields.values() attrs['_instances'] = weakref.WeakValueDictionary() - attrs['__slots__'] = ['_' + field for field in fields] for base in bases: if '__weakref__' in getattr(base, '__slots__', []): From 2cb2750b39a87aa0c9300bf1f6044bc7b6ec2749 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 8 Apr 2015 23:23:55 +0200 Subject: [PATCH 10/10] models: Simplify JSON decoder code --- mopidy/models.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/mopidy/models.py b/mopidy/models.py index 3dd2c67c..4b268474 100644 --- a/mopidy/models.py +++ b/mopidy/models.py @@ -313,13 +313,11 @@ def model_json_decoder(dct): """ if '__model__' in dct: + # TODO: move models to a global constant once we split this module models = {c.__name__: c for c in ImmutableObject.__subclasses__()} model_name = dct.pop('__model__') if model_name in models: - kwargs = {} - for key, value in dct.items(): - kwargs[key] = value - return models[model_name](**kwargs) + return models[model_name](**dct) return dct