Merge lp://staging/~nick-dedekind/unity/multi-instance-icon-loader into lp://staging/unity

Proposed by Nick Dedekind
Status: Work in progress
Proposed branch: lp://staging/~nick-dedekind/unity/multi-instance-icon-loader
Merge into: lp://staging/unity
Diff against target: 2350 lines (+1138/-850)
10 files modified
dash/ResultRendererTile.cpp (+2/-2)
dash/StandaloneDash.cpp (+3/-4)
unity-shared/CMakeLists.txt (+1/-0)
unity-shared/CoverArt.cpp (+14/-9)
unity-shared/CoverArt.h (+4/-2)
unity-shared/IconLoader.cpp (+112/-828)
unity-shared/IconLoader.h (+16/-5)
unity-shared/IconLoaderImpl.h (+119/-0)
unity-shared/IconLoaderTask.cpp (+756/-0)
unity-shared/IconLoaderTask.h (+111/-0)
To merge this branch: bzr merge lp://staging/~nick-dedekind/unity/multi-instance-icon-loader
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Needs Fixing
Michal Hruby (community) Needs Fixing
Nick Dedekind (community) Needs Fixing
Review via email: mp+129125@code.staging.launchpad.net

Commit message

Added multi-instance Icon Loaders with management.

Description of the change

Added multi-instance Icon Loaders with management.

Icon loaders can be created as requested and deleted when the last reference is removed. This allows better control of internal caching in areas such as dash previews.

All icon loading now asynchronous.
Refactored IconLoaderTask into class.
Refactored IconLoaderTask to remove "shadow tasks" (now adds a slot to existing tasks).
Added task thread-safe ref counting for loader instance deletion during IO scheduled job processing.

To post a comment you must log in.
Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

Needs unit test.

review: Needs Fixing
Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

TODO:
o Refactoring to remove sync/async from single call.
o Refactor IconLoaderTask into class.
o Tasks need thread-safe ref counting for loader instance deletion during IO scheduled job processing.

review: Needs Fixing
Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

All icon loading now asynchronous.
Refactored IconLoaderTask into class.
Refactored IconLoaderTask to remove "shadow tasks" (now adds a slot to existing tasks).
Added task thread-safe ref counting for loader instance deletion during IO scheduled job processing.

Revision history for this message
Michal Hruby (mhr3) wrote :

1136 + timer_.reset(new glib::Timeout(0, sigc::mem_fun(this, &IconLoaderImpl::Iteration), glib::Source::Priority::LOW));

Why the change to Timeout instead of Idle?

2138 + InvokeSlot();
2139 +
2140 + if (!impl_->coalesce_timeout_)
2141 + {

We want to dispatch the slots in the coalesce callback (as the dispatch will often cause a QueueDraw which has higher prio than finishing other loader tasks - ie causes "finish, redraw, finish, redraw, finish, redraw" vs "finish, finish, finish, redraw").

1103 + task_slot_map_[handle] = iter->second;

Am I right in thinking these elements in the map will "leak"?

review: Needs Fixing
2827. By Nick Dedekind

Merge with trunk

2828. By Nick Dedekind

Invoke icon slot on coalesce.

2829. By Nick Dedekind

Fixed referencing issue.

Revision history for this message
Nick Dedekind (nick-dedekind) wrote :

> 1136 + timer_.reset(new glib::Timeout(0, sigc::mem_fun(this,
> &IconLoaderImpl::Iteration), glib::Source::Priority::LOW));
>
> Why the change to Timeout instead of Idle?
>
> 2138 + InvokeSlot();
> 2139 +
> 2140 + if (!impl_->coalesce_timeout_)
> 2141 + {
>
> We want to dispatch the slots in the coalesce callback (as the dispatch will
> often cause a QueueDraw which has higher prio than finishing other loader
> tasks - ie causes "finish, redraw, finish, redraw, finish, redraw" vs "finish,
> finish, finish, redraw").
>

Done. Reduced coalesce time to 200ms.

> 1103 + task_slot_map_[handle] = iter->second;
>
> Am I right in thinking these elements in the map will "leak"?

These should be released in IconLoaderImpl::DisconnectHandle when the slot is invoked.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)

Unmerged revisions

2829. By Nick Dedekind

Fixed referencing issue.

2828. By Nick Dedekind

Invoke icon slot on coalesce.

2827. By Nick Dedekind

Merge with trunk

2826. By Nick Dedekind

IconLoader code refactor.

2825. By Nick Dedekind

Merge with trunk

2824. By Nick Dedekind

Added multi-instance icon loaders management.

2823. By Nick Dedekind

Fixed dash standalone seg-fault on close.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.