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

Use std::map for faster lookup #2299

Merged
merged 2 commits into from
Aug 3, 2022
Merged

Conversation

kevinbackhouse
Copy link
Collaborator

OSS-Fuzz reported a timeout. It's not serious problem - just a moderately large file that's a bit slow to process. But I noticed that it's spending a lot of time in this line of code:

const TiffGroupStruct* ts = find(tiffGroupStruct_, TiffGroupStruct::Key(extendedTag, group));

It's doing a linear search of a 358-element array, which is a bit slow. This PR replaces the array with a std::map, which improves the running time on this particular file by approximately 10%.

poc: poc

time exiv2 poc.jpg

@codecov
Copy link

codecov bot commented Aug 2, 2022

Codecov Report

Merging #2299 (7498fa6) into main (1d0530f) will increase coverage by 0.00%.
The diff coverage is 84.44%.

@@           Coverage Diff           @@
##             main    #2299   +/-   ##
=======================================
  Coverage   63.53%   63.54%           
=======================================
  Files         118      118           
  Lines       19595    19597    +2     
  Branches     9576     9579    +3     
=======================================
+ Hits        12450    12452    +2     
  Misses       5079     5079           
  Partials     2066     2066           
Impacted Files Coverage Δ
src/tiffimage_int.cpp 78.63% <83.72%> (+0.29%) ⬆️
src/tiffimage_int.hpp 86.66% <100.00%> (-0.84%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us.

src/tiffimage_int.hpp Outdated Show resolved Hide resolved
src/tiffimage_int.hpp Outdated Show resolved Hide resolved
@neheb
Copy link
Collaborator

neheb commented Aug 2, 2022

On a somewhat related note, I wonder if it was a mistake to replace function pointers with std::function...

@@ -146,29 +146,13 @@ struct TiffImgTagStruct {
IfdId group_; //!< Group that contains the image tag
Copy link
Collaborator

@neheb neheb Aug 2, 2022

Choose a reason for hiding this comment

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

Can this whole struct be replaced with

using TiffImgTagStruct = std::pair<uint16_t, IfdId>;

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I agree. And this looks inefficient too:

return find(tiffImageTags, TiffImgTagStruct::Key(tag, group));

I think those changes should go in a separate PR though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

From what I can tell, this find function is a wrapper over std::find.

@neheb
Copy link
Collaborator

neheb commented Aug 2, 2022

 Error:  Failed to execute: D:\a\exiv2\exiv2\build\bin\iotest s0 s1 s2 http://127.0.0.1:12760/table.jpg
Error:  The asserted return code is [0], but got 1

@neheb neheb merged commit 09f61bc into Exiv2:main Aug 3, 2022
@kevinbackhouse
Copy link
Collaborator Author

I think I need to investigate why one of the tests failed.

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.

3 participants