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

Refactored Perceptionhash for performance #54

Merged
merged 5 commits into from
May 26, 2022
Merged

Refactored Perceptionhash for performance #54

merged 5 commits into from
May 26, 2022

Conversation

evanoberholster
Copy link
Contributor

@evanoberholster evanoberholster commented May 12, 2022

Refactored PerceptionHash for performance. If you want me to change the name to replace existing function that would be acceptable.

Features:

  • Significantly reduces allocations
  • Uses static DCT1D tables
  • Uses sync pool to reduce pixel data allocations
  • Uses image.Image optimizations for obtaining pixels for improved inlining of functions
  • Backward compatible with PerceptionHash, they are interchangeable. Confirmed with passing tests.
  • Performance without image resize as below.
name                      time/op
PerceptionHash/Regular-12         358µs ±26%
PerceptionHash/Fast-12           84.8µs ± 7%
PerceptionHash/Parallel-12       82.5µs ± 3%
PerceptionHash/Fast-Parallel-12  11.5µs ± 1%

name                      alloc/op
PerceptionHash/Regular-12         193kB ± 0%
PerceptionHash/Fast-12            17.0B ± 0%
PerceptionHash/Parallel-12        193kB ± 0%
PerceptionHash/Fast-Parallel-12    198B ± 0%

name                      allocs/op
PerceptionHash/Regular-12         4.68k ± 0%
PerceptionHash/Fast-12             0.00     
PerceptionHash/Parallel-12        4.68k ± 0%
PerceptionHash/Fast-Parallel-12    0.00 

@evanoberholster evanoberholster requested a review from corona10 as a code owner May 12, 2022 04:30
@evanoberholster
Copy link
Contributor Author

Comparison between PerceptionHash and PerceptionHashFast:

name               old time/op    new time/op    delta
PerceptionHash-12     945µs ± 9%     689µs ± 2%  -27.08%  (p=0.000 n=170+177)

name               old alloc/op   new alloc/op   delta
PerceptionHash-12     471kB ± 0%     298kB ± 0%  -36.80%  (p=0.000 n=174+198)

name               old allocs/op  new allocs/op  delta
PerceptionHash-12     4.74k ± 0%     0.07k ± 0%  -98.61%  (p=0.000 n=200+200)

Copy link
Owner

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

@evanoberholster Looks nice! Thank you for your contribution!!

If you want me to change the name to replace existing function that would be acceptable.

I prefer this one(change the name ) :)

@corona10 corona10 self-assigned this May 12, 2022
@corona10
Copy link
Owner

Please add your name to the following file.
https://github.com/corona10/goimagehash/blob/master/AUTHORS.md

@corona10
Copy link
Owner

Significantly reduces allocations
Uses static DCT1D tables
Uses sync pool to reduce pixel data allocations
Uses image.Image optimizations for obtaining pixels for improved inlining of functions
Backward compatible with PerceptionHash, they are interchangeable. Confirmed with passing tests.
Performance without image resize as below.

Thanks, I love all of your changes :)

@evanoberholster evanoberholster requested a review from corona10 May 12, 2022 20:47
Copy link
Owner

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Would you like to switch the PerceptionHashFast into PerceptionHash and remove old implementation(including old dct implementation)?

hashcompute.go Outdated
// Uses static DCT tables for improved performance.
// Implementation follows
// http://www.hackerfactor.com/blog/index.php?/archives/432-Looks-Like-It.html
func PerceptionHashFast(img image.Image) (*ImageHash, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
func PerceptionHashFast(img image.Image) (*ImageHash, error) {
func PerceptionHash(img image.Image) (*ImageHash, error) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made these changes in new update.

@evanoberholster
Copy link
Contributor Author

PerceptionHashFast DCT static tables only support a size of 64x64. We would need to include further static tables to support other image sizes, (eg 256x256)

@corona10
Copy link
Owner

@evanoberholster

PerceptionHashFast DCT static tables only support a size of 64x64.

Ah okay, let's replace PerceptionHash only.

@evanoberholster
Copy link
Contributor Author

Sorry for the delay I have been busy at work. Will get it replaced and update the PR.

@corona10
Copy link
Owner

Sorry for the delay I have been busy at work. Will get it replaced and update the PR.

Yeah, Don't worry I can wait :)

@evanoberholster evanoberholster requested a review from corona10 May 25, 2022 04:20
Copy link
Owner

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Almost looks good to me, Just 1 nit comment and I will take a look at more later :)
Thanks for the change!

hashcompute_test.go Outdated Show resolved Hide resolved
Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@evanoberholster
Copy link
Contributor Author

I agree that those tests are redundant. Thanks.

hashcompute_test.go Outdated Show resolved Hide resolved
@corona10 corona10 merged commit 45da7a6 into corona10:master May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants