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

Optimization of the first call of zim::Archive::iterEfficient() #724

Merged
merged 3 commits into from
Aug 22, 2022

Conversation

veloman-yunkan
Copy link
Collaborator

  1. memory usage of FileImpl::m_articleListByCluster has been halved
  2. general purpose O(NlogN) sorting algorithm was replaced with a counting sort approach (with O(N) time complexity).

@codecov
Copy link

codecov bot commented Aug 14, 2022

Codecov Report

Merging #724 (bb7c998) into master (920b97d) will increase coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head bb7c998 differs from pull request most recent head 5e2d46b. Consider uploading reports for the commit 5e2d46b to get more accurate results

@@            Coverage Diff             @@
##           master     #724      +/-   ##
==========================================
+ Coverage   84.57%   84.67%   +0.09%     
==========================================
  Files          98       98              
  Lines        4297     4325      +28     
  Branches     1864     1869       +5     
==========================================
+ Hits         3634     3662      +28     
  Misses        662      662              
  Partials        1        1              
Impacted Files Coverage Δ
src/fileimpl.h 94.11% <ø> (ø)
src/fileimpl.cpp 86.56% <100.00%> (+1.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kelson42 kelson42 added this to the 8.1.0 milestone Aug 14, 2022
Copy link
Collaborator

@mgautierfr mgautierfr left a comment

Choose a reason for hiding this comment

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

A small change needed but it seems good to me.
Nice improvement !

src/fileimpl.cpp Outdated Show resolved Hide resolved
@mgautierfr
Copy link
Collaborator

LGTM. Please rebase-fixup on master

The new class Grouping<ObjectId, GroupId> implements the current
straightforward approach for sorting the dirents in cluster order, but
opens a way for switching to a better solution.

A user-observable effect of this change is the lower memory usage after
`FileImpl::prepareArticleListByCluster()` has completed (but an increased
high-watermark memory usage of that function).
Got rid of O(NlogN) sorting in FileImpl::prepareArticleListByCluster().
Now its time-complexitiy is O(N). Also, its high-watermark memory usage was
lowered roughly to the level before Grouping was introduced.
@veloman-yunkan
Copy link
Collaborator Author

LGTM. Please rebase-fixup on master

Done

@mgautierfr mgautierfr merged commit dc917b0 into master Aug 22, 2022
@mgautierfr mgautierfr deleted the optimization_of_iterEfficient branch August 22, 2022 18:23
@kelson42 kelson42 modified the milestones: 8.2.0, 8.1.0 Sep 5, 2022
mgautierfr added a commit that referenced this pull request Nov 30, 2022
* Optimization ofthe first call to `zim::Archive::iterEfficient` (@veloman-yunkan #724)
* Add some documentation to `zim::writer::IndexData` (@mgautierfr #727)
* Correctly catch and rethrow exception thrown in worker threads at creation (@mgautierfr #496 #748)
* Optimization of `Entry::getItem()` (@veloman-yunkan #732)
* Fix declaration of `zim::setICUDataDirectory()` (@MohitMaliFtechiz #733)
* Add `zim::Archive::getMediatCount()` (@mgautierfr #730)
* Make compilaton of examples optional (@mgautierfr #738)
* Add a CI for wasm (@mgautierfr #746)
* Make constructor of SuggestionItem public (@veloman-yunkan #740)
@mgautierfr mgautierfr mentioned this pull request Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants