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.

Sign up for GitHub

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

Jump to bottom

serialize/deserialize colors to/from bytes instead of strings #1049

Merged
merged 3 commits into from
Jan 27, 2023
Merged

serialize/deserialize colors to/from bytes instead of strings #1049

merged 3 commits into from
Jan 27, 2023

Conversation

Dobatymo
Copy link
Contributor

It's a tiny bit faster, saves a bit of memory and simplifies the code a bit. Also if in the future 3rd party perceptual hashes will be used, it makes saving/restoring to/from numpy arrays even easier (and even faster), since it's only a .tobytes() or .frombuffer() then.

For benchmarking run

import secrets
from timeit import repeat
from core.pe.cache import string_to_colors
#from core.pe.cache import bytes_to_colors

colorsbytes = secrets.token_bytes(32 * 3)

# assert string_to_colors(colors.hex()) == bytes_to_colors(colors)

print(
    "string_to_colors",
    min(repeat("func(data)", repeat=3, globals={"data": colorsbytes.hex(), "func": string_to_colors})),
)
#print("bytes_to_colors", min(repeat("func(data)", repeat=3, globals={"data": colorsbytes, "func": bytes_to_colors})))

@Dobatymo
serialize/deserialize colors to/from bytes instead of strings
f1153c8 
it's a tiny bit faster and saves a bit of memory
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.2% 0.2% Duplication

glubsy
glubsy approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@glubsy glubsy left a comment

Choose a reason for hiding this comment

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

Have not tested but looks good.

@Dobatymo
Copy link
Contributor Author

Sorry I forgot to mention that this requires users to delete their picture cache. This PR so far doesn't handle old format caches gracefully. Any suggestions?

@arsenetar
Copy link
Owner

@Dobatymo This looks pretty good, I do want to spend some more time actually running this. With regards to the picture cache, for the sqlite implementation this could do something similar to what has been done with the file cache db to automatically migrate and clear out the db when changes are made see the class at

dupeguru/core/fs.py

Line 96 in 1f1dfa8

class FilesDB:
. For the shelve implementation, it would need a bit different approach, I have been considering removing that back-end anyhow and just doing sqlite only to simplify things but wanted to verify the performance difference between the two before I finalized that decision.

@arsenetar
Copy link
Owner

Merged in changes to resolve a couple minor conflicts from changes I made recently (removing shelve cache, formatting). Going to be looking at adding a migration to the picture db similar to the other so users don't have to worry about manually clearing cache.

@Dobatymo
Copy link
Contributor Author

Thank you for keep working on my PR :)
I am sorry I don't have any time recently to implement a migration path.

@arsenetar
feat: Add migration for picture cache db
46521c8 
- Add migration (just delete db and change to new schema) for picture
  cache following the same sort of strategy as the file digest cache
- Rename mtime column to mtime_ns to match file cache for consistency
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.1% 0.1% Duplication

@arsenetar
Copy link
Owner

@Dobatymo No problem, got the changes pulled in here to "migrate" the cache now, will do a bit more testing on my end then merge this as I think it all looks good to go.

@arsenetar arsenetar merged commit e41c916 into arsenetar:master Jan 27, 2023
@Dobatymo Dobatymo deleted the colors-bytes branch January 27, 2023 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glubsy glubsy glubsy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Dobatymo @arsenetar @glubsy