Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PICARD-1204: Read and dump values to QSettings only when necessary #854

Merged
merged 4 commits into from Mar 1, 2018

Conversation

samj1912
Copy link
Collaborator

@samj1912 samj1912 commented Mar 1, 2018

Reading and dumping too often from QSettings causes picard to lock up.
This makes sure we only read on start and dump on change.

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences:

Problem

Solution

Action

@samj1912 samj1912 changed the title PICARD-1204: Read and dump values to QtConfig only when necessary PICARD-1204: Read and dump values to QSettings only when necessary Mar 1, 2018
@samj1912 samj1912 force-pushed the picard_1204 branch 3 times, most recently from 7da4fdc to 20fdc07 Compare March 1, 2018 14:18
Reading and dumping too often from QSettings causes picard to lock up.
This makes sure we only read on start and dump on change.
picard/config.py Outdated
self.load_keys()

def __qt_keys(self):
return filter(lambda key: key.startswith('%s/' % self.__name),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to cache '%s/' % self.__name:

def __qt_keys(self):
   prefix = '%s/' % self.__name
   def prefixed(key):
       return key.startswith(prefix)
    return filter(prefixed, self.__qt_config.allKeys())

picard/tagger.py Outdated
@@ -230,7 +231,6 @@ def __init__(self, picard_args, unparsed_args, localedir, autoupdate):
self.unclustered_files = UnclusteredFiles()
self.nats = None
self.window = MainWindow()
self.exit_cleanup = []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't needed anymore.

@samj1912 samj1912 force-pushed the picard_1204 branch 2 times, most recently from bdd62e8 to ca9eaad Compare March 1, 2018 14:29
picard/config.py Outdated
self.load_keys()

def __qt_keys(self):
prefix = '%s/' % self.__name
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using % format, since key building here are very simple, using concatenation will waste less cpu cycles.
Since config options can be read in loops, optimization makes sense, and doesn't cost much in readibility.

prefix = '%s/' % self.__name
key = "%s/%s" % (self.__name, name)

vs

prefix = self.__name + '/'
key = self.__name + '/' + name
from timeit import timeit

runs = 10000000


def print_results(time, start_string):
    print('%s\nTotal: %.4f\nAvg: %.4fns\n' % (start_string, time,
                                              (time/runs)*1000000000))

t1 = timeit('"%s/%s" % (a, b)',
            setup='a="xxx"; b="yyy"',
            number=runs)
t2 = timeit("a + '/' + b",
            setup='a="xxx"; b="yyy"',
            number=runs)
t3 = timeit('a + "/" + b',
            setup='a="xxx"; b="yyy"',
            number=runs)
t4 = timeit('"{}/{}".format(a, b)',
            setup='a="xxx"; b="yyy"',
            number=runs)

print_results(t1, '% replacement')
print_results(t2, 'concatenation single quotes')
print_results(t3, 'concatenation double quotes')
print_results(t4, '.format method')
% replacement
Total: 3.1121
Avg: 311.2064ns

concatenation single quotes
Total: 1.6318
Avg: 163.1769ns

concatenation double quotes
Total: 1.7592
Avg: 175.9204ns

.format method
Total: 4.3462
Avg: 434.6209ns


def raw_value(self, key):
def raw_value(self, name):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did this so that there is a consistency between name and key

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok ;)


def raw_value(self, key):
def raw_value(self, name):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok ;)

@samj1912 samj1912 merged commit 61d5550 into metabrainz:master Mar 1, 2018
@samj1912 samj1912 deleted the picard_1204 branch March 1, 2018 18:34
zas added a commit to zas/picard that referenced this pull request Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants