Merge lp:~mhr3/unity-lens-music/memory-optimizations into lp:unity-lens-music

Proposed by Michal Hruby on 2012-03-28
Status: Merged
Approved by: Michal Hruby on 2012-03-30
Approved revision: 78
Merged at revision: 77
Proposed branch: lp:~mhr3/unity-lens-music/memory-optimizations
Merge into: lp:unity-lens-music
Diff against target: 425 lines (+232/-117)
4 files modified
configure.ac (+0/-1)
src/Makefile.am (+0/-1)
src/rhythmbox-collection.vala (+231/-114)
src/track.vala (+1/-1)
To merge this branch: bzr merge lp:~mhr3/unity-lens-music/memory-optimizations
Reviewer Review Type Date Requested Status
Gord Allott (community) 2012-03-28 Approve on 2012-03-30
Review via email: mp+99682@code.launchpad.net

Commit message

Optimize memory allocation

Description of the change

There were reports from multiple users with huge music collections that the new music lens is using huge amount of memory. After profiling this turned out to be mostly used by the xml parser which was keeping the entire parsed file in memory and since we constructed the song model right after the parsing, this caused fragmentation of memory, so even though the xml parser freed everything after the model was built, the mem usage of the process remained very high.

Replace the xml parser from libxml-2.0 with simple GMarkupParser that is able to process the xml by chunks and therefore doesn't have these kind of problems.

To post a comment you must log in.
Ted Gould (ted) wrote :

Using this branch lowered my memory usage from OVER ONE HUNDRED MEGABYTES to LESS THAN THIRTY. I am also now more attractive, smarter and I have more friends.

Gord Allott (gordallott) wrote :

clever ;) +1

review: Approve
Unity Merger (unity-merger) wrote :

No commit message specified.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'configure.ac'
2--- configure.ac 2012-03-23 12:09:52 +0000
3+++ configure.ac 2012-03-28 07:32:18 +0000
4@@ -63,7 +63,6 @@
5 dee-1.0 >= 1.0.7
6 sqlite3 >= 3.7.7
7 gee-1.0
8- libxml-2.0
9 json-glib-1.0
10 unity >= 4.99.0)
11
12
13=== modified file 'src/Makefile.am'
14--- src/Makefile.am 2012-02-01 10:07:59 +0000
15+++ src/Makefile.am 2012-03-28 07:32:18 +0000
16@@ -28,7 +28,6 @@
17 --pkg gio-2.0 \
18 --pkg gio-unix-2.0 \
19 --pkg glib-2.0 \
20- --pkg libxml-2.0 \
21 $(MAINTAINER_VALAFLAGS)
22
23 unity_music_daemon_LDADD = \
24
25=== modified file 'src/rhythmbox-collection.vala'
26--- src/rhythmbox-collection.vala 2012-03-22 09:13:52 +0000
27+++ src/rhythmbox-collection.vala 2012-03-28 07:32:18 +0000
28@@ -33,36 +33,179 @@
29 ARTWORK,
30 MIMETYPE,
31 GENRE,
32+ ALBUM_ARTIST,
33 TRACK_NUMBER,
34- ALBUM_ARTIST,
35 YEAR,
36- PLAY_COUNT
37+ PLAY_COUNT,
38+
39+ N_COLUMNS
40 }
41
42 class RhythmboxCollection : Object
43 {
44
45- GenericArray<Track> tracks = new GenericArray<Track> ();
46 SequenceModel all_tracks;
47 FilterModel tracks_by_play_count;
48
49+ HashTable<unowned string, Variant> variant_store;
50+ HashTable<int, Variant> int_variant_store;
51+ Variant row_buffer[11];
52+
53 Analyzer analyzer;
54 Index index;
55 ICUTermFilter ascii_filter;
56
57 string media_art_dir;
58
59- // contains genre maps
60- Genre genre = new Genre ();
61+ class XmlParser: Object
62+ {
63+ const MarkupParser parser =
64+ {
65+ start_tag,
66+ end_tag,
67+ process_text,
68+ null,
69+ null
70+ };
71+
72+ // contains genre maps
73+ Genre genre = new Genre ();
74+
75+ MarkupParseContext context;
76+ bool is_rhythmdb_xml = false;
77+
78+ construct
79+ {
80+ context = new MarkupParseContext (parser, 0, this, null);
81+ }
82+
83+ public bool parse (string content, size_t len) throws MarkupError
84+ {
85+ return context.parse (content, (ssize_t) len);
86+ }
87+
88+ bool processing_song;
89+ Track current_track;
90+ int current_data = -1;
91+
92+ private void start_tag (MarkupParseContext context, string name,
93+ [CCode (array_length = false, array_null_terminated = true)] string[] attr_names, [CCode (array_length = false, array_null_terminated = true)] string[] attr_values)
94+ throws MarkupError
95+ {
96+ if (!processing_song)
97+ {
98+ switch (name)
99+ {
100+ case "rhythmdb": is_rhythmdb_xml = true; break;
101+ case "entry":
102+ bool is_song = false;
103+ for (int i = 0; attr_names[i] != null; i++)
104+ {
105+ if (attr_names[i] == "type" && attr_values[i] == "song")
106+ is_song = true;
107+ }
108+ if (!is_song) return;
109+ processing_song = true;
110+ current_track = new Track ();
111+ break;
112+ }
113+ }
114+ else
115+ {
116+ switch (name)
117+ {
118+ case "location": current_data = Columns.URI; break;
119+ case "title": current_data = Columns.TITLE; break;
120+ case "artist": current_data = Columns.ARTIST; break;
121+ case "album": current_data = Columns.ALBUM; break;
122+ case "genre": current_data = Columns.GENRE; break;
123+ case "track-number": current_data = Columns.TRACK_NUMBER; break;
124+ case "play-count": current_data = Columns.PLAY_COUNT; break;
125+ case "date": current_data = Columns.YEAR; break;
126+ case "media-type": current_data = Columns.MIMETYPE; break;
127+ case "album-artist": current_data = Columns.ALBUM_ARTIST; break;
128+ default: current_data = -1; break;
129+ }
130+ }
131+ }
132+
133+ public signal void track_info_ready (Track track);
134+
135+ private void end_tag (MarkupParseContext content, string name)
136+ throws MarkupError
137+ {
138+ switch (name)
139+ {
140+ case "location":
141+ case "title":
142+ case "artist":
143+ case "album":
144+ case "genre":
145+ case "track-number":
146+ case "play-count":
147+ case "date":
148+ case "media-type":
149+ case "album-artist":
150+ if (current_data >= 0) current_data = -1;
151+ break;
152+ case "entry":
153+ if (processing_song && current_track != null)
154+ {
155+ track_info_ready (current_track);
156+ }
157+ processing_song = false;
158+ break;
159+ }
160+ }
161+
162+ private void process_text (MarkupParseContext context,
163+ string text, size_t text_len)
164+ throws MarkupError
165+ {
166+ if (!processing_song || current_data < 0) return;
167+ switch (current_data)
168+ {
169+ case Columns.URI: current_track.uri = text; break;
170+ case Columns.TITLE: current_track.title = text; break;
171+ case Columns.ARTIST: current_track.artist = text; break;
172+ case Columns.ALBUM: current_track.album = text; break;
173+ case Columns.ALBUM_ARTIST:
174+ current_track.album_artist = text;
175+ break;
176+ case Columns.GENRE:
177+ current_track.genre = genre.get_id_for_genre (text.down ());
178+ break;
179+ case Columns.MIMETYPE:
180+ current_track.mime_type = text;
181+ break;
182+ case Columns.YEAR:
183+ current_track.year = int.parse (text) / 365;
184+ break;
185+ case Columns.PLAY_COUNT:
186+ current_track.play_count = int.parse (text);
187+ break;
188+ case Columns.TRACK_NUMBER:
189+ current_track.track_number = int.parse (text);
190+ break;
191+ }
192+ }
193+ }
194
195 construct
196 {
197+ static_assert (11 == Columns.N_COLUMNS); // sync with row_buffer size
198 media_art_dir = Path.build_filename (
199 Environment.get_user_cache_dir (), "media-art");
200
201+ variant_store = new HashTable<unowned string, Variant> (str_hash,
202+ str_equal);
203+ int_variant_store = new HashTable<int, Variant> (direct_hash,
204+ direct_equal);
205 all_tracks = new SequenceModel ();
206 // the columns correspond to the Columns enum
207- all_tracks.set_schema ("s", "s", "s", "s", "s", "s", "s", "s", "s", "i", "i");
208+ all_tracks.set_schema ("s", "s", "s", "s", "s", "s", "s", "s", "i", "i", "i");
209+ assert (all_tracks.get_schema ().length == Columns.N_COLUMNS);
210+
211
212 var filter = Dee.Filter.new_sort ((row1, row2) =>
213 {
214@@ -137,116 +280,90 @@
215 return null;
216 }
217
218+ private Variant cached_variant_for_string (string? input)
219+ {
220+ unowned string text = input != null ? input : "";
221+ Variant? v = variant_store[text];
222+ if (v != null) return v;
223+
224+ v = new Variant.string (text);
225+ // key is owned by value... awesome right?
226+ variant_store[v.get_string ()] = v;
227+ return v;
228+ }
229+
230+ private Variant cached_variant_for_int (int input)
231+ {
232+ Variant? v = int_variant_store[input];
233+ if (v != null) return v;
234+
235+ v = new Variant.int32 (input);
236+ // let's not cache every random integer
237+ if (input < 128)
238+ int_variant_store[input] = v;
239+ return v;
240+ }
241+
242+ private void prepare_row_buffer (Track track)
243+ {
244+ Variant uri = new Variant.string (track.uri);
245+ Variant title = new Variant.string (track.title);
246+ Variant artist = cached_variant_for_string (track.artist);
247+ Variant album_artist = cached_variant_for_string (track.album_artist);
248+ Variant album = cached_variant_for_string (track.album);
249+ Variant mime_type = cached_variant_for_string (track.mime_type);
250+ Variant artwork = cached_variant_for_string (track.artwork_path);
251+ Variant genre = cached_variant_for_string (track.genre);
252+ Variant track_number = cached_variant_for_int (track.track_number);
253+ Variant year = cached_variant_for_int (track.year);
254+ Variant play_count = cached_variant_for_int (track.play_count);
255+
256+ row_buffer[0] = uri;
257+ row_buffer[1] = title;
258+ row_buffer[2] = artist;
259+ row_buffer[3] = album;
260+ row_buffer[4] = artwork;
261+ row_buffer[5] = mime_type;
262+ row_buffer[6] = genre;
263+ row_buffer[7] = album_artist;
264+ row_buffer[8] = track_number;
265+ row_buffer[9] = year;
266+ row_buffer[10] = play_count;
267+ }
268+
269 public void parse_file (string path)
270 {
271- Xml.Parser.init ();
272-
273- Xml.Doc* doc = Xml.Parser.parse_file (path);
274- if (doc == null) {
275- return;
276- }
277-
278- Xml.Node* root = doc->get_root_element ();
279- if (root == null) {
280- delete doc;
281- return;
282- }
283- parse_node (root);
284- delete doc;
285-
286- Xml.Parser.cleanup ();
287- }
288-
289- private void parse_node (Xml.Node* node)
290- {
291- for (Xml.Node* iter = node->children; iter != null; iter = iter->next) {
292- if (iter->type != Xml.ElementType.ELEMENT_NODE) {
293- continue;
294- }
295- for (Xml.Attr* prop = iter->properties; prop != null; prop = prop->next) {
296- string attr_content = prop->children->content;
297- if (attr_content == "song") {
298- Track track = new Track ();
299-
300- for (Xml.Node* iter_track = iter->children; iter_track != null;
301- iter_track = iter_track->next) {
302- if (iter_track->type != Xml.ElementType.ELEMENT_NODE) {
303- continue;
304- }
305- switch (iter_track->name) {
306- case "title":
307- string node_content = iter_track->get_content ();
308- track.title = node_content;
309- break;
310- case "location":
311- string node_content = iter_track->get_content ();
312- track.uri = node_content;
313- break;
314- case "artist":
315- string node_content = iter_track->get_content ();
316- track.artist = node_content;
317- break;
318- case "media-type":
319- string node_content = iter_track->get_content ();
320- track.mime_type = node_content;
321- break;
322- case "album":
323- string node_content = iter_track->get_content ();
324- track.album = node_content;
325- break;
326- case "genre":
327- string node_content = iter_track->get_content ();
328- track.genre = node_content;
329- break;
330- case "track-number":
331- string node_content = iter_track->get_content ();
332- track.track_number = node_content;
333- break;
334- case "album-artist":
335- string node_content = iter_track->get_content ();
336- track.album_artist = node_content;
337- break;
338- case "date":
339- string node_content = iter_track->get_content ();
340- track.year = int.parse (node_content);
341- break;
342- case "play-count":
343- string node_content = iter_track->get_content ();
344- track.play_count = int.parse (node_content);
345- break;
346- }
347- }
348- // append to tracks array
349- tracks.add (track);
350-
351- // Get cover art
352- string albumart = get_albumart (track);
353- if (albumart != null)
354- track.artwork_path = albumart;
355- else
356- track.artwork_path = "audio-x-generic";
357-
358- // Get genre filter id
359- track.genre = genre.get_id_for_genre(track.genre.down ());
360-
361- if (track.year > 0) {
362- track.year /= 365;
363- }
364-
365- all_tracks.append (track.uri, track.title,
366- track.artist, track.album,
367- track.artwork_path,
368- track.mime_type,
369- track.genre,
370- track.track_number,
371- track.album_artist,
372- track.year,
373- track.play_count);
374- }
375- // Next track
376- parse_node (iter);
377- }
378- }
379+ var parser = new XmlParser ();
380+ parser.track_info_ready.connect ((track) =>
381+ {
382+ // Get cover art
383+ string albumart = get_albumart (track);
384+ if (albumart != null)
385+ track.artwork_path = albumart;
386+ else
387+ track.artwork_path = "audio-x-generic";
388+
389+ prepare_row_buffer (track);
390+ all_tracks.append_row (row_buffer);
391+ });
392+
393+ var file = File.new_for_path (path);
394+
395+ try
396+ {
397+ var stream = file.read (null);
398+ uint8 buffer[65536];
399+
400+ size_t bytes_read;
401+ while ((bytes_read = stream.read (buffer, null)) > 0)
402+ {
403+ parser.parse ((string) buffer, bytes_read);
404+ }
405+ }
406+ catch (Error err)
407+ {
408+ warning ("Error while parsing rhythmbox DB: %s", err.message);
409+ }
410 }
411
412 public void search (LensSearch search,
413
414=== modified file 'src/track.vala'
415--- src/track.vala 2012-03-19 18:52:51 +0000
416+++ src/track.vala 2012-03-28 07:32:18 +0000
417@@ -29,7 +29,7 @@
418 public string album { get; set; }
419 public string album_artist { get; set; }
420 public string genre { get; set; }
421- public string track_number { get; set; }
422+ public int track_number { get; set; }
423 public int year { get; set; }
424 public int play_count { get; set; }
425 }

Subscribers

People subscribed via source and target branches

to all changes: