Apache OpenOffice (AOO) Bugzilla – Issue 75399
let store save some megabytes of memory
Last modified: 2013-03-11 15:04:11 UTC
I am attaching patches which are updates of ooo-build/patches/test/store-*diff. Michael let me update them so that the store changes are backward compatible with original format. store-core.diff - changes in the store module, introduce format version 2, be backward compatible with original format version 1 store-registry.diff - changes in registry to use updated store. regmerge gets new option -i/-internal which enables the use of new format store-install.diff - let the build create types.rdb and services.rdb with new format. I have done measurements on suse linux 32bit and it saves around 5.6M of memory, about 5.4M of shared memory space and about 200k of private memory space - measured with pmap on empty started writer.
Created attachment 43704 [details] proposed patch
Created attachment 43705 [details] proposed patch
Created attachment 43706 [details] proposed patch
TM->MBA: One for you ? Please have a look, thanks !
No, it's code from Matthias Hüttsch
Created attachment 43891 [details] updated patch
I have attached an updated version of the core patch, which contains big endian fixes and few cleanings.
Seems like this patch breaks build on win32 platform with .NET 2003: -------------------------------------------------- guw.exe /cygdrive/c/PROGRA~1/MICROS~2.NET/Vc7/bin/cl.exe @/tmp/mkUv6Skj storbase.cxx e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(675) : warning C424 4: 'argument' : conversion from 'sal_uInt32' to 'sal_uInt16', possible loss of d ata e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(1051) : warning C42 44: 'argument' : conversion from 'sal_uInt32' to 'sal_uInt16', possible loss of data e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(1544) : error C2057 : expected constant expression e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(1544) : error C2466 : cannot allocate an array of constant size 0 e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(1544) : error C2133 : 'aPageHeadData' : unknown size e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(1978) : error C2057 : expected constant expression e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(1978) : error C2466 : cannot allocate an array of constant size 0 e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(1978) : error C2133 $ e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(2215) : error C2057 : expected constant expression e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(2215) : error C2466 : cannot allocate an array of constant size 0 $ : 'data' : unknown size e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(2268) : error C2057 : expected constant expression e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(2268) : error C2466 : cannot allocate an array of constant size 0 e:\build\2.2.0\src\OOo_2.2_src\store\source\storbase.cxx(2268) : error C2133 : 'data' : unknown size dmake: Error code 2, while making '../wntmsci10.pro/slo/storbase.obj' ---* tg_merge.mk *--- ERROR: Error 65280 occurred while making /cygdrive/e/build/2.2.0/src/OOo_2.2 _src/store/source --------------------------------------------------
I have patch for this build problem. Let me clean it and I will attach it soon.
updated store-core.diff (not yet attached here) passed store module.
Hi Radek, Thanks for providing this patch. I'm sorry it took more time than expected to write down my comments. First of all, I think the patch indeed does what it claims to do (perhaps modulo some minor platform / compiler issues), and this is good. There are a couple of points that I think could be solved different, though. (*) the proposed change is not local to module 'store', but touches a couple more modules. This is due to an (syntactically incompatible) change to store's api. (*) From what I can see, this api change in turn is due (and necessary) to an (semantically incompatible) change to (a) the maximum name length (only a single 'OStorePageLongNameData' page) so that names must in fact be shorter than STORE_MAXIMUM_NAMESIZE=256 characters, and (b) the reduction in STORE_DEFAULT_PAGESIZE reduces the maximum stream length (determined by the number of single / double / triple indirect pages) from the GByte range into the MByte range. I'm happily taking your proposed change as good input to the very similar change that I'm currently working on (which does not introduce the above mentioned semantic changes, and thus does not need an incompatible api change so that it can stay local to module 'store'). As this patch will not be applied verbatim (as is), I'm changing the issue type from 'PATCH' to 'ENHANCEMENT', and add the issue to my upcoming cws where our combined changes will then be integrated. Thanks, Matthias
Correcting project to 'UCB', changing issue type to 'ENHANCEMENT', and setting target milestone to '2.x'...
Hi Matthias, thanks for your comments. Let me comment on them too. * The changes outside store module are only to use the advantage of the new format. Usage of the new format is meant for OOo package internally - thus new regmerge -i switch. I don't see any problem with it, until some 3rd party code tries to put gigabytes of data into internal registries, which is unlikely. If the code needs to be ABI compatible I can modify my changes to add new methods instead of adding new parameters (with default values) * about semantically incompatible changes, the maximum name length can be fixed easily to 256+ characters again (by linking more blocks) * I think it might be possible to enlarge the stream length limit as well, but I don't see any point in doing so. Given that store module is only used for registry files. The new default block size was optimized for current .rdb files usage. See doc/store.txt analysis in ooo-build from Michael. Excerpt from it: Analysis: types.rdb 98% of files < 2^12 bytes < = 4096 bytes types.rdb 100% of names < 64 bytes services.rdb 99% of files < 2^8 bytes < = 256 bytes services.rdb 99% of names < 64 bytes Largest file == 2^15 bytes == 32k. I would be quite disappointed if you throw my patch away as I spent some time with it as you might see ;-) Please let me know if you are willing accept my patch if I modify it to be ABI compatible and remove name max length. I might look into maximal stream length as well if you insist :) Radek
mhu: Ping? ;-) Could you please comment on Radek's comments? And - changing back to PATCH, because it indeed is a patch ;-) It saves a large amount of memory, so it would be great for everyone to have this issue sorted out faster. [Not to mention that we use it in ooo-build for quite some time, so it affects just upstream and StarOffice.]
Hi Radek, Jan, Well, yes, I should be more responsive. But I'm neither ignoring your work, nor your comments; don't worry. Probably my initial comments weren't as clear as I thought, so please let me try again. As I'm working on the very same problem (plus an additional somewhat invasive change to the store module to save even more memory) as Radeks patch, I am accepting to merge Radeks proposed changes with / incorporate into my own changes (and appropriately recognize Radeks contribution). [btw, Michael should have known that I'm working on the same problem; please ask him]. As both patches (Radeks and my own) are quite large, and heavily interfere on a line by line level (not semantically), the changes can only be merged manually. That's what I meant with "not applying the patch verbatim (as is)", and changed the issue type to "enhancement". Please understand that I'm not going to rework my changes more than necessary to incorporate Radeks changes. But, as I said, your changes will be incorporated, just not "as is". Hope this resolves any misunderstandings, Matthias
thanks for clarifying Matthias - looking forward to seeing the results.
Hi Matthias, thanks for making it more clear. Let me know if I can help you in any way with it. Cheers Radek
FYI, on FreeBSD you don't need to include alloca.h: --- store/source/storbase.cxx +++ store/source/storbase.cxx @@ -87,8 +87,10 @@ #endif #ifdef WNT #include <malloc.h> #else +#ifndef __FreeBSD__ #include <alloca.h> #endif +#endif using namespace store;
I hope this patch can be included soon. as we know that still many people complain about OOo performance, or OOo eat too much memory. can we also update this ? http://tools.openoffice.org/performance/windows/index.html which we can see the memory usage which is growing from older version, instead of decreased. from 625 to 638 by this patch I believe we will make many people happy. Thanks, and good luck
I'm adding this comment to all open issues with Issue Type == PATCH. We have 220 such issues, many of them quite old. I apologize for that. We need your help in prioritizing which patches should be integrated into our next release, Apache OpenOffice 4.0. If you have submitted a patch and think it is applicable for AOO 4.0, please respond with a comment to let us know. On the other hand, if the patch is no longer relevant, please let us know that as well. If you have any general questions or want to discuss this further, please send a note to our dev mailing list: dev@openoffice.apache.org Thanks! -Rob