From adf051cb65451a23141ff941d759e435bb01fec8 Mon Sep 17 00:00:00 2001 From: Gabriel Ferreira Date: Tue, 23 Mar 2021 14:03:32 -0300 Subject: [PATCH] Reduce comparison overhead of logging window --- picard/log.py | 36 +++++++++++++-- test/test_log.py | 112 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 145 insertions(+), 3 deletions(-) create mode 100644 test/test_log.py diff --git a/picard/log.py b/picard/log.py index c36af33f7..f97fb8d84 100644 --- a/picard/log.py +++ b/picard/log.py @@ -3,12 +3,13 @@ # Picard, the next-generation MusicBrainz tagger # # Copyright (C) 2007, 2011 Lukáš Lalinský -# Copyright (C) 2008-2010, 2019 Philipp Wolfer +# Copyright (C) 2008-2010, 2019, 2021 Philipp Wolfer # Copyright (C) 2012-2013 Michael Wiencek # Copyright (C) 2013, 2015, 2018-2019 Laurent Monin # Copyright (C) 2016-2018 Sambhav Kothari # Copyright (C) 2017 Sophist-UK # Copyright (C) 2018 Wieland Hoffmann +# Copyright (C) 2021 Gabriel Ferreira # # This program is free software; you can redistribute it and/or # modify it under the terms of the GNU General Public License @@ -89,6 +90,26 @@ class TailLogHandler(logging.Handler): self.tail_logger.updated.emit() +def _calculate_bounds(previous_position, first_position, last_position, queue_length): + # If first item of the queue is bigger than prev, use first item position - 1 as prev + # e.g. queue = [8, 9, 10] , prev = 6, new_prev = 8-1 = 7 + if previous_position < first_position: + previous_position = first_position-1 + + # The offset of the first item in the queue is + # equal to the length of the queue, minus the length to be printed + offset = queue_length - (last_position - previous_position) + + # If prev > last_position, offset will be bigger than queue length offset > queue_length + # This will force an empty list + if offset > queue_length: + offset = queue_length + + # If offset < 1, there is a discontinuity in the queue positions + # Setting queue_length to 0 informs the caller that something is wrong and the slow path should be taken + return offset, queue_length + + class TailLogger(QtCore.QObject): updated = QtCore.pyqtSignal() @@ -100,8 +121,17 @@ class TailLogger(QtCore.QObject): def contents(self, prev=-1): with self._queue_lock: - contents = [x for x in self._log_queue if x.pos > prev] - return contents + # If log queue is empty, return + if not self._log_queue: + return [] + offset, length = _calculate_bounds(prev, self._log_queue[0].pos, self._log_queue[-1].pos, len(self._log_queue)) + + if offset >= 0: + return (self._log_queue[i] for i in range(offset, length)) + # If offset < 0, there is a discontinuity in the queue positions + # Use a slower approach to get the new content. + else: + return (x for x in self._log_queue if x.pos > prev) def clear(self): with self._queue_lock: diff --git a/test/test_log.py b/test/test_log.py new file mode 100644 index 000000000..d84a8c6bd --- /dev/null +++ b/test/test_log.py @@ -0,0 +1,112 @@ +# -*- coding: utf-8 -*- +# +# Picard, the next-generation MusicBrainz tagger +# +# Copyright (C) 2021 Gabriel Ferreira +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License +# as published by the Free Software Foundation; either version 2 +# of the License, or (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + +from collections import deque + +from test.picardtestcase import PicardTestCase + +from picard.log import _calculate_bounds + + +class TestLogItem: + def __init__(self, pos=0): + self.pos = pos + + +class TestLogItemQueue: + def __init__(self): + self._log_queue = deque(maxlen=10) + + def contents(self, prev=-1): + if not self._log_queue: + return [] + offset, length = _calculate_bounds(prev, self._log_queue[0].pos, self._log_queue[-1].pos, len(self._log_queue)) + + if offset >= 0: + return (self._log_queue[i] for i in range(offset, length)) + # If offset < 0, there is a discontinuity in the queue positions + # Use a slower approach to get the new content. + else: + return (x for x in self._log_queue if x.pos > prev) + + def push(self, item): + self._log_queue.append(item) + + +class LogQueueCommonTest(PicardTestCase): + def setUp(self): + super().setUp() + self.item_queue = TestLogItemQueue() + + +class LogQueueBoundsTestCase(LogQueueCommonTest): + def test_1(self): + # Common case where the item positions are within the max size of the queue + # [0,1,2,3,4,5,6,7], len = 8, maxlen = 10, offset = 0 + for i in range(8): + self.item_queue.push(TestLogItem(i)) + content_list = self.item_queue.contents() + self.assertListEqual([x.pos for x in content_list], list(range(0, 8))) + + def test_2(self): + # Common case where the item positions are outside the max size of the queue + # Which means the positions do not match the index of items in the queue + # [5,6,7,8,9,10,11,12,13,14], len = 10, offset = len - (last - prev) = 10 - (14-7) = 3 + for i in range(15): + self.item_queue.push(TestLogItem(i)) + content_list = self.item_queue.contents(7) # prev value + self.assertListEqual([x.pos for x in content_list], list(range(8, 15))) + + def test_3(self): + # Previous case but the previous item (2) was already removed from the queue + # So we pick the first item in the queue in its place + # [5,6,7,8,9,10,11,12,13,14], len = 10, maxlen = 10, prev = 5-1 = 4, offset = 0 + for i in range(15): + self.item_queue.push(TestLogItem(i)) + content_list = self.item_queue.contents(2) + self.assertListEqual([x.pos for x in content_list], list(range(5, 15))) + + def test_4(self): + # In case we have only one element but use different prev values + self.item_queue.push(TestLogItem(10)) + content_list = self.item_queue.contents() # prev = -1 is smaller than 10, so we update prev from -1 to 10-1 = 9 + self.assertListEqual([x.pos for x in content_list], [10]) + + content_list = self.item_queue.contents(2) # prev = 2 is smaller than 10, so we update prev from 2 to 10-1 = 9 + self.assertListEqual([x.pos for x in content_list], [10]) + + content_list = self.item_queue.contents(9) # prev = 9 is smaller than 10, so we update prev from 9 to 10-1 = 9 + self.assertListEqual([x.pos for x in content_list], [10]) + + content_list = self.item_queue.contents(10) # prev = 10 is equal to 10, so we use it as is + self.assertListEqual([x.pos for x in content_list], []) + + content_list = self.item_queue.contents(20) # prev = 20 is bigger than 10, so we use it as is + self.assertListEqual([x.pos for x in content_list], []) + + def test_5(self): + # This shouldn't really happen, but here is a test for it + # In case of a discontinuity e.g. [4,5,11], we have len = 3, prev = 3, last_pos=11, + # which results in offset = 3 - (11-4) = -4, which is completely absurd offset, when the correct would be 0 + self.item_queue.push(TestLogItem(4)) + self.item_queue.push(TestLogItem(5)) + self.item_queue.push(TestLogItem(11)) + content_list = self.item_queue.contents(3) + self.assertListEqual([x.pos for x in content_list], [4, 5, 11])