Bug 105500 - Small but noticeable lag when selecting shapes with sidebar enabled
Summary: Small but noticeable lag when selecting shapes with sidebar enabled
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Draw (show other bugs)
Version:
(earliest affected)
5.1.0.3 release
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:7.0.0
Keywords: bibisected, bisected, perf, regression
: 104326 109257 (view as bug list)
Depends on:
Blocks: Too-Much-File-Access Shapes
  Show dependency treegraph
 
Reported: 2017-01-24 08:25 UTC by Telesto
Modified: 2020-10-15 05:07 UTC (History)
7 users (show)

See Also:
Crash report or crash signature:


Attachments
Memory leak profile (27.89 KB, text/plain)
2017-08-17 07:46 UTC, Alex Thurgood
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Telesto 2017-01-24 08:25:34 UTC
Description:
The selection of a drawing item isn't instantaneous. There is a small delay when selecting an drawing object. 


Steps to Reproduce:
1. Open attachment 129188 [details]
2. Select a the green square
3. Now select & immediately drag blue square


Actual Results:  
There is a small delay before the dragging has some effect

Expected Results:
The dragging should occur without a lag as before in LibO5.0.0.5


Reproducible: Always

User Profile Reset: No

Additional Info:
Found in
Version: 5.4.0.0.alpha0+
Build ID: 79497f458727a0dea983847fe9d3873bf9c2e972
CPU Threads: 4; OS Version: Windows 6.19; UI Render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2017-01-24_03:10:09
Locale: nl-NL (nl_NL); Calc: CL

and in
Version: 5.1.0.3
Build ID: 5e3e00a007d9b3b6efb6797a8b8e57b51ab1f737
CPU Threads: 4; OS Version: Windows 6.2; UI Render: default; 
Locale: nl-NL (nl_NL)

but not in
Version: 5.0.6.3
Build ID: 490fc03b25318460cfc54456516ea2519c11d1aa
Locale: nl-NL (nl_NL)



User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Comment 1 Telesto 2017-01-24 08:27:22 UTC
Setting to NEW: Conformation found here: https://bugs.documentfoundation.org/show_bug.cgi?id=104312#c19
Comment 2 Aron Budea 2017-01-24 19:36:44 UTC Comment hidden (bibisection)
Comment 3 Aron Budea 2017-01-24 19:39:35 UTC
Bibisect points to the commit referenced below. It seems to be not only a unit test, but also include some sidebar code that must be causing the performance regression. Adding Cc: to Laurent Godard, please take a look.

https://cgit.freedesktop.org/libreoffice/core/commit/?id=187445b2d2885ced92be37ffb11cd2a9bb11f8d6
author	Laurent Godard <lgodard.libre@laposte.net>	2015-06-08 08:24:42 (GMT)
committer	Tomaž Vajngerl <quikee@gmail.com>	2015-06-23 06:38:56 (GMT)

"Uno api sidebar unit test tdf#91806"

Note that currently bug 104312 is causing a more severe performance issue, which likely has to be resolved before this bug can be investigated.
Comment 4 Telesto 2017-05-09 17:12:34 UTC
Bug 104312 is fixed thanks to Aron, but I'm still noticing this one on MacOS and Windows:
Version: 5.4.0.0.alpha1+
Build ID: 9d320ec4d818f86e58a15fd46248026502b1cc94
CPU threads: 4; OS: Windows 6.2; UI render: default; 
TinderBox: Win-x86@62-TDF, Branch:MASTER, Time: 2017-05-09_01:27:12
Locale: en-US (nl_NL); Calc: single
Comment 5 Telesto 2017-05-10 10:01:55 UTC
*** Bug 104326 has been marked as a duplicate of this bug. ***
Comment 6 Telesto 2017-06-28 21:00:04 UTC
The lag is most likely caused by the fact that the sidebar is refreshing when a new shape is selected, which causes the reload of all palette related files (for example standard.soc) 

It's working fine without sidebar.

Version: 6.0.0.0.alpha0+
Build ID: 9f3814af7264ce90685a82cbf4eb015a38f22bf7
CPU threads: 4; OS: Windows 6.19; UI render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2017-06-28_00:47:42
Locale: nl-NL (nl_NL); Calc: CL
Comment 7 Alex Thurgood 2017-08-17 07:43:43 UTC
Am enclosing a memory profile trace produced on macOS with Apple's Instruments memory leak checker/profiler application.

1) Load test file
2) Select first square with mouse.
3) Select second square with mouse.
4) Repeat steps 2 and 3 for a total of 3 times (3 cycles)
5) Close the document.
6) Shut down the office process.
Comment 8 Alex Thurgood 2017-08-17 07:46:15 UTC
Created attachment 135607 [details]
Memory leak profile
Comment 9 Alex Thurgood 2017-08-17 07:52:01 UTC
In PaletteManager.cxx :

line 109:

 pPalette.reset(new PaletteSOC(aFileStat.getFileURL(), aFNameWithoutExt));

in the following block:

{
                        std::unique_ptr<Palette> pPalette;
                        if( aFName.endsWithIgnoreAsciiCase(".gpl") )
                            pPalette.reset(new PaletteGPL(aFileStat.getFileURL(), aFNameWithoutExt));
                        else if( aFName.endsWithIgnoreAsciiCase(".soc") )
                            pPalette.reset(new PaletteSOC(aFileStat.getFileURL(), aFNameWithoutExt));
                        else if ( aFName.endsWithIgnoreAsciiCase(".ase") )
                            pPalette.reset(new PaletteASE(aFileStat.getFileURL(), aFNameWithoutExt));

                        if( pPalette && pPalette->IsValid() )
                            m_Palettes.push_back( std::move(pPalette) );
                        aNames.insert(aFNameWithoutExt);
                    }


seems to be invoked numerous times and not released.
Comment 10 Alex Thurgood 2017-08-17 07:53:00 UTC
In addition, there is also:

tbcontrl.cxx, lines 3235-3236:

void SvxColorListBox::EnsurePaletteManager()
{
    if (!m_xPaletteManager)
    {
        m_xPaletteManager.reset(new PaletteManager);
        m_xPaletteManager->SetColorSelectFunction(m_aColorWrapper);
        m_xPaletteManager->SetLastColor(m_aSelectedColor.first);
    }
}
Comment 11 Julien Nabet 2017-08-17 09:50:35 UTC
(In reply to Alex Thurgood from comment #9)
> In PaletteManager.cxx :
> 
> line 109:
> 
>  pPalette.reset(new PaletteSOC(aFileStat.getFileURL(), aFNameWithoutExt));
>...
I think this one needs

"m_Palettes.clear();" in PaletteManager::~PaletteManager()

(see https://opengrok.libreoffice.org/xref/core/svx/source/tbxctrls/PaletteManager.cxx#63)
Comment 12 Julien Nabet 2017-08-17 09:53:27 UTC
(In reply to Alex Thurgood from comment #10)
> In addition, there is also:
> 
> tbcontrl.cxx, lines 3235-3236:
> 
> void SvxColorListBox::EnsurePaletteManager()
> {
>     if (!m_xPaletteManager)
>     {
>         m_xPaletteManager.reset(new PaletteManager);
>         m_xPaletteManager->SetColorSelectFunction(m_aColorWrapper);
>         m_xPaletteManager->SetLastColor(m_aSelectedColor.first);
>     }
> }

I think we should add m_xPaletteManager.reset(nullptr);
in SvxColorToolBoxControl::~SvxColorToolBoxControl (see https://opengrok.libreoffice.org/xref/core/svx/source/tbxctrls/tbcontrl.cxx#2800)

BTW, perhaps m_xPaletteManager from colorbox.hxx could be deleted since it seems unused.

Here too, I'll be able to submit a patch about it tonight if nobody does it.
Comment 13 Commit Notification 2017-08-17 20:32:38 UTC
Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=8c0cc5cd7befffc6e8e6361ba67807a799cc997f

Related tdf#105500: leaks in PaletteManager and SvxColorToolBoxControl

It will be available in 6.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 14 JoNi 2017-08-17 23:12:38 UTC
(In reply to Commit Notification from comment #13)
> Julien Nabet committed a patch related to this issue.
> It has been pushed to "master":
> 
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=8c0cc5cd7befffc6e8e6361ba67807a799cc997f
> 
> Related tdf#105500: leaks in PaletteManager and SvxColorToolBoxControl

I doubt this patch does anything.
~SvxColorToolBoxControl destructs all it's members including m_xPaletteManager.
The unique_ptr<PaletteManager> destructor calls destructor of the PaletteManager object.
~PaletteManager destructs all it's members too. This includes the vector m_Palettes. A vector destructor calls the destructor for all objects it holds (clear() does the same). So finally the unique_ptr<Pallette> destructors release all the Palette objects.
We don't have to worry about reset, clear, delete or anything.

That said, I don't see any obvious leaks. But I got no idea how to interpret  attachment 135607 [details], looks like it's only listing allocations.
Comment 15 Commit Notification 2017-08-18 07:05:56 UTC
Noel Grandin committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=bc8a46534e03d9dc81c92756957cbae84d72b953

Revert "Related tdf#105500: leaks in PaletteManager and SvxColorToolBoxControl"

It will be available in 6.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 16 Telesto 2017-10-21 15:16:02 UTC
It seems to work without any issue - on MacOS - when I remove the following: 

// TODO: this call is redundant but mandatory for unit test to update context on document loading
UpdateConfigurations(); 

https://opengrok.libreoffice.org/xref/core/sfx2/source/sidebar/SidebarController.cxx#281
Comment 17 Telesto 2017-11-18 16:02:36 UTC
*** Bug 109257 has been marked as a duplicate of this bug. ***
Comment 18 Telesto 2017-12-22 21:51:59 UTC
Removing "target:6.0.0". The commit in question has been reverted.

The issue seems to be the most problematic on MacOS. The reports are still coming in, but end up in general MacOS performance bug reports:
https://bugs.documentfoundation.org/show_bug.cgi?id=106154#c28
https://bugs.documentfoundation.org/show_bug.cgi?id=106703#c2

I priority bump would be nice (in my opinion). The source of the problem is known (comment 16).
Comment 19 Pierre-Alain Dorange 2018-03-03 07:28:02 UTC
With last 5.x revision it was even worst.
Draw module was not usable, all the interface was so slow and so unresponsiness...

Even with the sidebar hidden rgere was noticable lags.
Comment 20 Telesto 2019-03-28 18:46:40 UTC
Repro with
Version: 6.3.0.0.alpha0+
Build ID: 20ea90a557b5bc744fd234e3a20ab1db484cf88b
CPU threads: 4; OS: Windows 6.3; UI render: default; VCL: win; 
TinderBox: Win-x86@42, Branch:master, Time: 2019-03-22_03:21:58
Locale: nl-NL (nl_NL); UI-Language: en-US
Calc: threaded
Comment 21 sdc.blanco 2020-10-05 21:31:25 UTC
WFM, how about you?

Version: 7.1.0.0.alpha0+ (x64)
Build ID: 3c6177be2705303044e3de262689d593f3d0f282
CPU threads: 8; OS: Windows 10.0 Build 19041; UI render: Skia/Raster; VCL: win
Locale: da-DK (en_DK); UI: en-US
Calc: threaded
Comment 22 Aron Budea 2020-10-15 05:07:03 UTC
I agree. Reverse bibisected the fix with repo bibisect-linux-64-7.0 to the following commit. Let's close as FIXED.

https://cgit.freedesktop.org/libreoffice/core/commit/?id=5b77d17c4f1ca734babf962b45c1aa07bdca14e9
author		Michael Meeks <michael.meeks@collabora.com>	2020-01-04 18:09:20 +0000
committer	Michael Meeks <michael.meeks@collabora.com>	2020-01-06 19:16:28 +0100

sidebar: allow panels to lurk around instead of being disposed.