Bug 118603 - Temporarily huge memory usage bump when saving a copy of a file (containing HI-res images); release is delayed
Summary: Temporarily huge memory usage bump when saving a copy of a file (containing H...
Status: RESOLVED WORKSFORME
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Draw (show other bugs)
Version:
(earliest affected)
6.2.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: regression
Depends on:
Blocks: Memory
  Show dependency treegraph
 
Reported: 2018-07-07 07:49 UTC by Telesto
Modified: 2020-06-13 12:47 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Telesto 2018-07-07 07:49:31 UTC
Description:
Bit of a nitpick. The image life cycle memory management seems needs some tweaking for saving a copy of the file (in my opinion). LibO keeps the images of the copy alive for a while. 

Steps to Reproduce:
1. Open attachment 132970 [details]

Actual Results:
* Large memory bump
* Delayed release

Expected Results:
* No delayed release & preferably no bump


Reproducible: Always


User Profile Reset: No



Additional Info:
Found in
Version: 6.2.0.0.alpha0+
Build ID: bb1d5780226bb1b9156580972eea9aa849178742
CPU threads: 4; OS: Windows 6.3; UI render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2018-07-03_05:56:48
Locale: nl-NL (nl_NL); Calc: CL

but not in
Versie: 4.4.7.2 
Build ID: f3153a8b245191196a4b6b9abd1d0da16eead600
Locale: nl_NL
Comment 1 Buovjaga 2018-07-16 18:12:35 UTC
Step 2: Save as copy

Confirmed, but I think devs will shoot this down as an outrageous nitpick :)

Arch Linux 64-bit
Version: 6.2.0.0.alpha0+
Build ID: 860a9daf2b45942a4b10ff22d36aa3fe29be19f4
CPU threads: 8; OS: Linux 4.17; UI render: default; VCL: gtk3; 
Locale: fi-FI (fi_FI.UTF-8); Calc: group threaded
Built on July 14th 2018
Comment 2 Buovjaga 2018-07-16 19:00:05 UTC
With the example file, initial res mem use was about 660M. It jumped to about 1100M when saving as copy.

I created a new file with 4 new pages and 4 new unique images (using the png as base). Initial res mem was now 1850M, it jumped to 2994M after saving as copy.

Eike had some concerns regarding documents with many images and possibly no cap for the mem use, so let's set to new.
Comment 3 Telesto 2018-07-19 06:53:43 UTC
Adding bibisectRequest; seems practical..
Comment 4 Telesto 2018-07-19 08:26:48 UTC
@Buovjaga 
Slightly off topic, but I still get confused by the term 'regression'. The QA has a user perspective and DEV's have a technical approach. Which isn't to helpful, in my opinion. Not sure if there is need for some clarification (or more confusing keywords)

My own list of sub-types:
a. A regression between versions of LibreOffice (the behavior changed to the worse, sec). Uninterested in how and why
b. Regression which happen outside the LibO code, due a change in the operation system (the code has not changed, isn't working anymore as expected) 
c. Technical approach: unexpected /unintended side-effect of a code change. Which includes a, maybe b but not c
sub a. something that the DEV broke with his commit (in strict sense)
sub b. something that the DEV broke, because of brokenness down te road because of a different code path
sub c. something that the DEV already expected to happen; (for example: harmonising font rendering made latin font handling a lot slower)
Comment 5 Buovjaga 2018-07-19 08:52:26 UTC
(In reply to Telesto from comment #4)
> @Buovjaga 
> Slightly off topic, but I still get confused by the term 'regression'. The
> QA has a user perspective and DEV's have a technical approach. Which isn't
> to helpful, in my opinion. Not sure if there is need for some clarification
> (or more confusing keywords)
> 
> My own list of sub-types:
> a. A regression between versions of LibreOffice (the behavior changed to the
> worse, sec). Uninterested in how and why
> b. Regression which happen outside the LibO code, due a change in the
> operation system (the code has not changed, isn't working anymore as
> expected) 
> c. Technical approach: unexpected /unintended side-effect of a code change.
> Which includes a, maybe b but not c
> sub a. something that the DEV broke with his commit (in strict sense)
> sub b. something that the DEV broke, because of brokenness down te road
> because of a different code path
> sub c. something that the DEV already expected to happen; (for example:
> harmonising font rendering made latin font handling a lot slower)

I'm not seeing the split in perspective. The user perspective might initially be hazy because of lack of understanding, but surely we arrive to a common perspective after discussion and investigation?

If a so-called regression is a change in behaviour that was pushed by the design team, then the users just need to accept it.

If there is an argument over changing use of systems resources (such as more mem use and less CPU), I think users without insights into specifics should stay out of the discussion. There is no use arguing, if one does not understand the reasoning behind a change and is not able to propose an alternate solution. This does not mean that testers should skip altogether investigating alarming things they observe.

Your example "harmonising font rendering made latin font handling a lot slower" is still a regression, but it requires a completely new approach as a component was changed wholesale. It is thus a lot less trivial than the usual regressions caused by a mistake or some unforeseen side-effect.

I don't see how your case b. has a relation to LibreOffice devs, or maybe you meant "the DEV of operating system X broke something". We have to work around OS bugs or change features accordingly, yes, but these cases cannot be called our regressions, because there is nothing to bisect.
Comment 6 Buovjaga 2018-07-19 16:18:24 UTC
I tried bibisecting this with win 6.1 several times, last time with rm -rf instdir/cache instdir/user on every step. I ended up with bogus results every time.
Comment 7 Jean-Baptiste Faure 2018-07-22 19:52:21 UTC
Why do you think that is a bug? RAM is made to be used.

No need to do a copy to bump the RAM consumption, it is enough to select one of the pictures. The RAM consumption is bigger if PNG is selected than JPEG.
With LO 6.1.1.0.0+ under Ubuntu 16.04 x86-64: 548 MiB when the file is loaded, 787 MiB when the JPEG is selected, 987 MiB when the PNG is selected. RAM 

If you want smooth scroll and fast display of pictures you need to use the RAM. I do not see any bad behavior here.

Best regards. JBF
Comment 8 Telesto 2018-07-31 18:23:35 UTC
Bug 119023 is probably more representative (probably the same root cause)
Comment 9 BogdanB 2018-10-12 17:21:17 UTC
I tested this bug with 
Version: 6.2.0.0.alpha0+
Build ID: 144da6d5079bcd435e6637cb5cf95305f3ec1306
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk2; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2018-10-12_02:13:01
Locale: ro-RO (ro_RO.UTF-8); Calc: threaded

And it is much better than with previous versions...

Please test again...
Comment 10 Buovjaga 2020-06-13 12:16:03 UTC
Yep, I think this can be closed. Memory bump is not huge and release is not delayed.

Arch Linux 64-bit
Version: 7.1.0.0.alpha0+
Build ID: de1b634a151c198584dc152676183f519c50a2da
CPU threads: 8; OS: Linux 5.6; UI render: default; VCL: kf5
Locale: fi-FI (fi_FI.UTF-8); UI: en-US
Calc: threaded
Built on 12 June 2020
Comment 11 Telesto 2020-06-13 12:47:25 UTC
(In reply to Buovjaga from comment #10)
> Yep, I think this can be closed. Memory bump is not huge and release is not
> delayed.
> 
> Arch Linux 64-bit
> Version: 7.1.0.0.alpha0+
> Build ID: de1b634a151c198584dc152676183f519c50a2da
> CPU threads: 8; OS: Linux 5.6; UI render: default; VCL: kf5
> Locale: fi-FI (fi_FI.UTF-8); UI: en-US
> Calc: threaded
> Built on 12 June 2020

Somewhat related.. Opening the file & clicking the image = OOM Crash -> With x86
Version: 7.1.0.0.alpha0+ (x86)
Build ID: 26483950760f0aac7bc45e93db4127f66a98fdc6
CPU threads: 4; OS: Windows 6.3 Build 9600; UI render: Skia/Raster; VCL: win
Locale: nl-NL (nl_NL); UI: en-US
Calc: CL