XLSX document created by FPSpreadsheet not readable by LibreOffice
lazarus / 49-FPSpreadsheet

----------------------------------------------------------------------
rvk · 2014-07-07 17:28

When i create a .xlsx document with FPSpreadsheet i can open it in
Excel but not with LibreOffice (latest version).

I can extract all files (from the .xlsx) en re-zip with 7-zip to a
.xlsx file. After that i can read it with LibreOffice.
(so the internal files are ok)

Normal .ods created with FPSpreadsheet can also be read by
LibreOffice.
(so i figured LibreOffice should be able to read the zipped .xlsx too)

Why would LibreOffice not be able to read a .xlsx created with the
same zip-functions as an .ods (which it can read)?
(I get the standard Text Import in LibreOffice when trying to read a
.xlsx)

(I have tried both the release version and the latest svn version)

----------------------------------------------------------------------
BigChimp · 2014-07-07 18:18

Please always post your Lazarus and FPC version. Those matter. (i.e.
different zip routines are used depending on FPC versions).
In this case, please also specify LibreOffice version...

Strange; obviously LibreOffice expects something that fpspreadsheet
does not provide at least with xlsx. It would also be helpful to
upload an example program and even nicer if you could upload the
faulty xlsx and the corrected xlsx...

----------------------------------------------------------------------
rvk · 2014-07-07 18:47

This is with
Windows 7 Professional
Lazarus 1.2.4 r45510 FPC 2.6.4 i386-win32-win32/win64
   with fpspreadsheet revision 3294
LibreOffice Version: 4.2.5.2

Code to reproduce test.xlsx and test.ods in C:\TEMP (xlsx and ods
files are attached)
(Clean project with one button)
Code: [Select]uses
  fpspreadsheet;

procedure TForm1.Button1Click(Sender: TObject);
var
  MyWorkbook: TsWorkbook;
  MyWorksheet: TsWorksheet;
  i, j: integer;
begin
  MyWorkbook := TsWorkbook.Create;
  MyWorksheet := MyWorkbook.AddWorksheet('My Worksheet');
  for i := 0 to 40 do
    for j := 0 to 20 do
      MyWorksheet.WriteUTF8Text(i, j, 'aj' + IntToStr(i) + ' ' +
IntToStr(j));
  MyWorkbook.WriteToFile('c:\temp\test' + STR_OOXML_EXCEL_EXTENSION,
sfOOXML);
  MyWorkbook.WriteToFile('c:\temp\test' +
STR_OPENDOCUMENT_CALC_EXTENSION, sfOpenDocument);
  MyWorkbook.Free;
end;

When i try to open test.xlsx in LibreOffice i get the Text Import
dialog.
When i take test.xlsx and extract it to a directory i can rezip it
with 7-zip.
(Resulting in attached test_rezipped.xlsx)

The test_rezipped.xlsx is readable by LibreOffice without problems.

So the structure of the files (in the xlsx) seem to be correct but
there is something wrong with the way it is zipped. (and that is
strange because as far as i can tell the .ods is zipped in the same
way and that one is readable by LibreOffice.)

----------------------------------------------------------------------
BigChimp · 2014-07-07 22:30

Thanks.
I see the same problem with Laz trunk r45807, FPC trunk x86, Win,
fpspreadsheet r3296 with the .xlsx file in LibreOffice 4.2.5.2...
which should be using the regular FPC zipper code instead of the
fpspreadsheet-supplied fpszipper

Will investigate further...

----------------------------------------------------------------------
rvk · 2014-07-08 09:07

Wauw... I found the "bug"    (my third bugfix-assist for fpspreadsheet
in a week)

I tried to re-zip the files with this code to see if the order of the
files within the zip would make a difference. But i accidentally used
forward slashes instead of backslashes (as is normal in Windows).

AND IT WORKED !! (I could read the file in LibreOffice)

Code: [Select]procedure TForm1.Button3Click(Sender: TObject);
var
  Zip: TZipper;
  Ts: TStringList;
begin
  SetCurrentDir('c:\temp\test\');
  Ts := TStringList.Create;
  Ts.Add('[Content_Types].xml');
  Ts.Add('_rels/.rels');
  Ts.Add('xl/_rels/workbook.xml.rels');  // <--- Oops... / = lucky
typo ;)
  Ts.Add('xl/workbook.xml');
  Ts.Add('xl/styles.xml');
  Ts.Add('xl/sharedStrings.xml');
  Ts.Add('xl/worksheets/sheet1.xml');
  Zip := TZipper.Create;
  Zip.ZipFiles('c:\temp\test2.xlsx', Ts);
  Zip.Free;
  Ts.Free;
end;

Attached is the test.xlsx created by fpspreadsheet and a newly zipped
test2.xlsx (zipped by zipper.pp).
If i scroll down in the files with a hexeditor and compare them you
clearly see the only difference is the / and \.

The only file with slashes (and subdir) in an ODS-file is
OPENDOC_PATH_METAINF_MANIFEST and that one is also defined with a
forward slash (/) regardless of OS. The OOXML_PATH_xxxx's in
xlsxooxml.pas are defined with the PathDelim (from sysutilh.inc) which
is the system-delimiter. (Which is the backslash on Windows)

So LibreOffice has a problem with files zipped with backslahes
(internally within the zip).
7-zip also zips files with forward slashes (that's why rezipping with
7-zip worked).

Is it a standard that all zip-files should have forward slashes as
delimiter????
If so, then the Path variables (OOXML_PATH_xxxx) in xlsxooxml.pas are
wrong.

Edit: Yeah... The OOXML_PATH_xxxx's should be adjusted with /.
(Or even better: the zipper-unit should replace the \ with /)

.ZIP File Format Specification

Quote4.4.17 file name: (Variable)
4.4.17.1 The name of the file, with optional relative path.
    The path stored MUST not contain a drive or device letter, or a
leading slash.  All slashes MUST be forward slashes '/' as opposed to
backwards slashes '\' for compatibility with Amiga and UNIX file
systems etc.(source:
http://www.pkware.com/documents/casestudies/APPNOTE.TXT)

----------------------------------------------------------------------
BigChimp · 2014-07-08 09:25

Interesting - thanks! I was figuring out the file structure and why
there are 11 file entries in the 7zip versions but only 7 in the
original one (probably directories). No need to go further, looks like

I'll have a detailed look at your post/links later.

However, it's weird that Excel does open this file - perhaps they have
a... how to put it... more Windows-centric idea of what path
delimiters should look like

----------------------------------------------------------------------
rvk · 2014-07-08 10:28

It's probably that strictly speaking a backslash in a zip is forbidden
but because some program's do use them, that Excel (and 7-zip as well
as the standard zip of Windows) allow them to be read. (It would be
easy to implement reading \ and interpreting them as /).

LibreOffice apparently is somewhat stricter in the reading of zipped
.xlsx and does not allow backslashes in internal filenames. (Excel
also creates them with the forward slash as it should be done)

(We could of course try to change LibreOffice to be somewhat easier on
backslashes in zips but it would be easier to stay true to the
standard and use forward slashes in xlsxooxml.pas or zipper.pp.)

I changed this in xlsxooxml.pas and it works. I can read the file in
LibreOffice.
Code: [Select]  PathDelim = '/'; // <-- overruling of the one used in
sysutilh.inc
  { OOXML Directory structure constants }
  OOXML_PATH_TYPES     = '[Content_Types].xml';
  OOXML_PATH_RELS      = '_rels' + PathDelim;
  ...
(Of course instead of using the PathDelim in the constants we could
just use / hardcoded same as it is done in fpsopendocument.pas but
that's up to the experts )

Maybe it would be better to change this at zipper.pp level (so it
doesn't happen with other uses of this unit) but i'm not well enough
known with the source to make that determination.

----------------------------------------------------------------------
marcov · 2014-07-08 10:33

Probably zipper should have a force forward slashes option.

----------------------------------------------------------------------
wp · 2014-07-08 10:41

Since currently there is only a writer for xlsx we definitely should
hard-code the forward slash in order to get a file according to
specification. BTW if even Excel writes forward slashes are there any
programs at all which write a backslash?

----------------------------------------------------------------------
marcov · 2014-07-08 10:57

Quote from: wp on July 08, 2014, 12:41:14 pmBTW if even Excel writes
forward slashes are there any programs at all which write a backslash?

Seems old winzips did.   http://www.info-zip.org/FAQ.html#backslashes

Still it would be wise to have a passthrough (write as user
specificies), in case sb has to live with an app that really wants
it.  But that can be disabled by default.

----------------------------------------------------------------------
wp · 2014-07-08 19:06

Fixed in rev 3299 (replaced all "PathDelim" by "/").

@rvk: Hoping you find more of these. So far, I did not yet go into
details of the xlsx files, but this will happen soon.

----------------------------------------------------------------------
marcov · 2014-07-08 19:20

Well,....   On a Dutch forum we were discussing escaping of HTML
entities.  I think fpspreadsheet should do that for XLSX ?

----------------------------------------------------------------------
wp · 2014-07-08 19:30

Which forum posting is that? Google will certainly translate it for
me.

Maybe a stupid question: which kind of HTML entities do you mean?

----------------------------------------------------------------------
rvk · 2014-07-08 19:42

That would have been my next topic 
(The dutch forum-topic is here)

When writing a &, < or > in a cell the .xlsx version doesn't work.
.ods does translate it correctly to &amp;

Example:
Code: [Select]procedure TForm1.Button3Click(Sender: TObject);
var
  MyWorkbook: TsWorkbook;
  MyWorksheet: TsWorksheet;
  S: String;
begin
  MyWorkbook := TsWorkbook.Create;
  MyWorksheet := MyWorkbook.AddWorksheet('My Worksheet');

  S := 'Me & you < and >';
  MyWorksheet.WriteUTF8Text(0, 0, S);

  S := 'België';
  MyWorksheet.WriteUTF8Text(0, 1, S);

  MyWorkbook.WriteToFile('c:\temp\test' + STR_EXCEL_EXTENSION,
sfExcel8, true);
  MyWorkbook.WriteToFile('c:\temp\test' + STR_OOXML_EXCEL_EXTENSION,
sfOOXML);
  MyWorkbook.WriteToFile('c:\temp\test' +
STR_OPENDOCUMENT_CALC_EXTENSION, sfOpenDocument);
  MyWorkbook.Free;
  Close;
end;

The "Me & you < and >" is correctly transferred to an .ods but i get
problems with a .xlsx.
(LibreOffice doesn't show the .xlsx file at all and at this moment i
don't have Excel handy)

I could translate the &, < and > myself with something like this:
Code: [Select]S := StringReplace(s, '&', '&amp;', [rfReplaceAll]);
Then the .ods stays ok and the .xlsx is ok too but the .xls has &amp;
as text.

----------------------------------------------------------------------
marcov · 2014-07-08 19:44

Quote from: wp on July 08, 2014, 09:30:50 pm Maybe a stupid question:
which kind of HTML entities do you mean?

Characters like  & and > in cell texts

Forum: http://www.nldelphi.com/showthread.php?40666-fpspreadsheet-
met-75000-regels/page2

which quotes

The only illegal characters are &, < and > (as well as " or ' in
attributes).
They're escaped using XML entities, in this case you want &amp; for &.

from

http://stackoverflow.com/questions/730133/invalid-characters-in-xml

----------------------------------------------------------------------
rvk · 2014-07-08 19:54

fpsopendocument.pas uses a TDOMNode to write strings to the sheet.
That's why the translation of these illegal characters (&, <, >
etc...) are automatically taken care of.

xlsxooxml.pas writes its cells directly so the illegal characters are
not translated to their save (XML-compliant) counterparts like &amp;,
&lt; etc...

----------------------------------------------------------------------
wp · 2014-07-08 20:06

I see. Seems not too difficult.

Another more fundamental question in this context since I am not too
experienced with xml: The ods and xlsx writers construct a super-
super-long xml string which is written to file, every new part is
added by string concatenation. Wouldn't it be better to construct a
tree of DOM nodes?

----------------------------------------------------------------------
rvk · 2014-07-08 20:20

Quote from: wp on July 08, 2014, 10:06:26 pmI see. Seems not too
difficult.Aaah, now i look deeper i can see the ods writer uses
UTF8TextToXMLText. So, indeed using that it would be simple to fix
this in the .xlsx-writer as well.

fpsopendocument.pas does use TDOMNode a lot but it's mainly used for
reading an ods.

Quote from: wp on July 08, 2014, 10:06:26 pmAnother more fundamental
question in this context since I am not too experienced with xml: The
ods and xlsx writers construct a super-super-long xml string which is
written to file, every new part is added by string concatenation.
Wouldn't it be better to construct a tree of DOM nodes?
That was my first thought too... when diving in the sourcecode of
xlsxooxml.pas. (Where are the DOM nodes ?? )

But wouldn't this be a very, very big (almost total) rewrite (to use
DOM nodes)?.

----------------------------------------------------------------------
wp · 2014-07-08 20:28

QuoteBut wouldn't this be a very, very big (almost total) rewrite (to
use DOM nodes)?.Certainly, but if there were benefits it would be
worth the effort, in particular now when the unit is not yet too long
(I remember how short fpopendocument was before I added formatting and
all these things...).

So: are there any benefits?

----------------------------------------------------------------------
wp · 2014-07-08 20:54

The illegal xml character issue is fixed in rev 3300.

----------------------------------------------------------------------
rvk · 2014-07-08 21:05

Quote from: wp on July 08, 2014, 10:28:40 pmSo: are there any
benefits?
Well, without being an expert in these things i could imagine that
there are benefits as well as drawbacks to using DOM Nodes.

I would begin by asking the question:
"Why does the TsSpreadOpenDocReader use TDOMNode?"
Is it because it's easier to use (to read an XML) than dismembering
the XML yourself with endless pos-functions? If so... that would be
the first benefit... simplicity

Using DOM Nodes has the advantage of always using the standard (XML)
and not having to worry if you set an > in the right place or not.

Another thing... On the dutch forum we also discussed the fact that
fpspreadsheet is not suitable for very large files. When creating a
file with 80.000 rows and 40 columns you'll get a out of memory error
for sure (>1.8GB memory usage). Maybe with DOM Nodes this would be
better (but i'm not sure). Maybe it could even be worse.

(I saw some mention about files >4GB possible with the new zipper.pp
but i don't see that happening with the current memory usage of
fpspreadsheet. In that case there should be some swapping to disc
mechanism build in)

fpspreadsheet is also slow with writing very large spreadsheet (i.e.
>40.000 rows 20 colums). Perhaps also due to the string build-up (but
again i'm not sure).

As a possible drawback... You are adding another layer to the process
which could contain bugs, but my short experience here is that these
are very quickly resolved.

Quote from: wp on July 08, 2014, 10:54:11 pmThe illegal xml character
issue is fixed in rev 3300.
Thanks

----------------------------------------------------------------------
BigChimp · 2014-07-08 21:56

Quote from: rvk on July 08, 2014, 11:05:32 pmI saw some mention about
files >4GB possible with the new zipper.pp but i don't see that
happening with the current memory usage of fpspreadsheet.
While the zip64 support I added for FPC trunk does support this kind
of file sizes, TZipper uses streams which are fixed to 2GB IIRC... so
that's not a possibility at this time...

See http://wiki.lazarus.freepascal.org/FPC_New_Features_Trunk#TZipper_
support_for_zip64_format

----------------------------------------------------------------------
wp · 2014-07-08 21:59

Quote fpspreadsheet is not suitable for very large filesI would not
say that in general: I just tried a 65000 rows x 100 columns xls file
(biff8), and it saves fine within 10 seconds; if I re-save it from
Excel2007 or LibreOffice it takes 17 seconds - not too bad, although
in task manager they hardly show an increase of memory usage while
fpspreadsheet has a nasty peak probably because we have a very
inefficient memory consumption per cell (all formatting saved per cell
instead of referencing a format list).

----------------------------------------------------------------------
rvk · 2014-07-08 23:02

Quote from: wp on July 08, 2014, 11:59:36 pmI just tried a 65000 rows
x 100 columns xls file (biff8), and it saves fine within 10
seconds;What is the memory usage just before the .WriteToFile? For
65000x100 cells it is about 1,2GB here.

Quote from: wp on July 08, 2014, 11:59:36 pmif I re-save it from
Excel2007 or LibreOffice it takes 17 seconds - not too badDid you try
sfOOXML 

Here the writing of biff8 is no problem (in 6 seconds) but during the
.WriteToFile (sfOOXML) i get an out of memory error AFTER 5.5 minutes
(330 seconds !!!!). (below some sample code)
(i also just tried sfOpenDocument and that one also seems to be taking
forever, it's still going... i will investigate this further tomorrow)

So it seems spreadsheets of 70.000 to 80.000 rows are about as much as
fpspreadsheet can handle. If not for the slowness of writing (which
only occurs with .xlsx and .ods for me) then certainly for the memory
usage.

(Using WriteNumber instead of WriteUTF8Text the memory usage is
somewhat better, about 1.0GB, but it still craps out when writing to
sfOOXML)

Code: [Select]procedure TForm1.Button1Click(Sender: TObject);
var
  MyWorkbook: TsWorkbook;
  MyWorksheet: TsWorksheet;
  i, j: integer;
begin
  MyWorkbook := TsWorkbook.Create;
  MyWorksheet := MyWorkbook.AddWorksheet('My Worksheet');
  for i := 0 to 65000 do
  begin
    Label1.Caption := IntToStr(i);
    Application.ProcessMessages;
    for j := 0 to 100 do
    begin
      MyWorksheet.WriteUTF8Text(i, j, 'aj' + IntToStr(i) + ' ' +
IntToStr(j));
    end;
  end;
  ShowMessage('ready #1');
  //MyWorkbook.WriteToFile('c:\temp\test' + STR_EXCEL_EXTENSION,
sfExcel8, true);
  //MyWorkbook.WriteToFile('c:\temp\test' +
STR_OPENDOCUMENT_CALC_EXTENSION, sfOpenDocument);
  MyWorkbook.WriteToFile('c:\temp\test' + STR_OOXML_EXCEL_EXTENSION,
sfOOXML);
  ShowMessage('ready #2');
  MyWorkbook.Free;
  Close;
end;

I'm not well enough known with DOM to know if it has memory swapping
capabilities. I only know that SAX does handle XML differently by
reading it sequentially thus using less memory but that has its
disadvantages too.

(wow... the sfOpenDocument-write is still busy after 8 minutes....
with 25% usage for project1.exe... i'll check on this tomorrow)

----------------------------------------------------------------------
wp · 2014-07-09 07:14

QuoteDid you try sfOOXMNo, because I had no reason to question your
observations.

I think I try the step to replace the string operations of the current
OOXML writer with nodes. I'm no specialist with the internal details
of memory management, but to me a string is a contiguous array of
bytes. When a second string is appended to the first one the memory
allocated to the first string is not sufficient any more. Therefore,
memory corresponding to the sum of both strings (plus, maybe, some
spare) is allocated, the first string is copied to the new locattion,
and the second string is copied to where the first string ends.
Therefore, during this time we need twice the memory! In case of DOM
nodes, however, I'd suppose that there is no need for a contiguous
array in memory, the nodes can be anywhere. Certainly, there are
additional pointers to the nodes which would increase memory load
again. But I'd guess that the memory available for very big files is
increased by roughly a factor 2.

----------------------------------------------------------------------
marcov · 2014-07-09 07:59

You might also want to check out tstringbuilder  IIRC it was meant for
webframeworks that constantly add strings to a big string.

----------------------------------------------------------------------
Mike.Cornflake · 2014-07-09 08:04

Quote from: wp on July 09, 2014, 09:14:35 amQuoteDid you try sfOOXMI
think I try the step to replace the string operations of the current
OOXML writer with nodes. I'm no specialist with the internal details
of memory management, but to me a string is a contiguous array of
bytes. When a second string is appended to the first one the memory
allocated to the first string is not sufficient any more. Therefore,
memory corresponding to the sum of both strings (plus, maybe, some
spare) is allocated, the first string is copied to the new locattion,
and the second string is copied to where the first string ends.
Therefore, during this time we need twice the memory! In case of DOM
nodes, however, I'd suppose that there is no need for a contiguous
array in memory, the nodes can be anywhere. Certainly, there are
additional pointers to the nodes which would increase memory load
again. But I'd guess that the memory available for very big files is
increased by roughly a factor 2.

I'd be very interested in the results of your work switching over to
DOM.  The work on ODT also works by building strings.  For the DOCX
writer, I switched to TStringLists instead (my trials at the time with
DOM resulted in significant slowdowns in the writer code (as in approx
10 times slower building up the DOM)).  Going on this conversation
here, I'm beginning to regret that decision.

You queried earlier about advantages or disadvantages of DOM verses
String.  One large advantage I can see using the DOM is increasing the
readability of the code (and thus the maintainability).  Another is
the reader is more easily coded with a DOM, and having both the writer
and reader using similar code again improves maintainability...

----------------------------------------------------------------------
rvk · 2014-07-09 08:13

At this moment i'm debugging fpsopendocument.pas to see where it is
slow writing to a .ods with 65000x100 cells.

If i do a bleep (debugging is a bitch ) at every 100 rows i get a
bleep every 12 seconds.
100 rows x 100 cells in 12 seconds ???
Yeah, i can imagine 65000 rows will take a while.

I'm still checking but i think the problem lies in the internal
structure of fpspreadsheet (TAVGLVLTreeNode ??).
For example in the writing of sfOpenDocument a .FindRow is done for
every row.
I can't find this call in sfExcel5 so i guess that format is just
written sequentially without calling .Findrow each time)

So if the way the writing of sfOpenDocument and sfOOXML is changed in
the same way as sfExcel5 the speed-problem will be solved (i think).

If there is going to be work done with DOM Nodes this needs to be
taken into consideration.
(No expensive searching of the internal treenodes because if there are
more than a few thousand nodes this structure is not efficient at all)

----------------------------------------------------------------------
wp · 2014-07-09 08:49

Quotea .FindRow is done for every rowNo, this can't be a blocking
point for standard files because rows are only created if their height
is different from default. Therefore, the FindRows should exit
immediately.

In my opinion, the slow speed here again is a consequence of the
string approach for writing - for every string added the total string
has to be copied to a new location! For a large file, this will grow
up to several hundreds of MB. The call to WriteNumber, for example,
contains 11 "+" signs! This is called again and again for every cell,
and the string to be copied every time is growing and growing... I am
more and more convinced that this must be bottle-neck.

I started to rewrite XLSXOOXML with DOM nodes. It's quite some typing
but it's doable.

----------------------------------------------------------------------
rvk · 2014-07-09 08:54

As a test i commented out the line 2839, rev.3301,
fpsopendocument.pas:
Code: [Select]//Result := Result + FCellContent;"
As we expected that one is increasingly slower when adding more to the
Result-string.
(Maybe a streamwriter would be more efficient)

But i still had a major slowdown.
I changed the following line to exclude the test of GetCellCountInRow
(line 2785, rev 3301, fpsopendocument.pas)
Code: [Select]if false and (ASheet.GetCellCountInRow(r) = 0) then
begin
Now the code was done in 11 seconds (instead of me having to abort
after 3 minutes)
(of course with an invalid .ods but it's just to show where the
slowdown is)

So besides the slowdown of adding string to string the
GetCellCountInRow is very inefficient.
(Internally that function consists of lots of FindCell and FindCol's)

Quote from: wp on July 09, 2014, 10:49:30 amI started to rewrite
XLSXOOXML with DOM nodes. It's quite some typing but it's doable.Can't
wait to test it

----------------------------------------------------------------------
wp · 2014-07-09 11:00

GetCellCountInRow = 0 seems to check if there is any cell in a given
row. Therefore I replaced this by "GetFirstCellOfRow" which runs along
the row and stops seeking when the first cell is found - this should
be faster, but have the same effect. In addition, this method called
"GetLastColIndex" which again iterates through all cells. Therefore I
am caching its result in FLastColIndex which is updated whenever a new
cell is added or when GetLastColIndex is called with the parameter
AForceCalculation = true.

I did some tests, but did not see that increase which you mention.

Could you try it yourself? I am attaching the modified
fpspreadsheet.pas and fpsopendocument.pas.

----------------------------------------------------------------------
wp · 2014-07-09 11:54

I should publish sources only after full testing... Fixed bugs causing
incorrect value of cached last column/row index. The xlsxooxml test
runs a bit faster afterwards, but maybe that's just wishful thinking.
At least, the unit test does not show any errors now.

----------------------------------------------------------------------
rvk · 2014-07-09 12:23

Quote from: wp on July 09, 2014, 01:00:37 pmI did some tests, but did
not see that increase which you mention.
...
Could you try it yourself? I am attaching the modified
fpspreadsheet.pas and fpsopendocument.pas.

With the original revision 3301 i only did these two changes i
mentioned:
Code: [Select]2785. if false and (ASheet.GetCellCountInRow(r) = 0)
then begin
2839. // Result := Result + FCellContent;

I get the following results (all columns are x100):
5.000 took 0 seconds (well rounded to 0)
10.000 took 1 seconds
20.000 took 2 seconds

With your changes i get (so both Result := Result + and
GetFirstCellOfRow stay intact):
5.000 took 10 seconds
10.000 took 40 seconds
20.000 took 160 seconds

So i stripped out the line 2841 again (the Result := Result + ).
Then i got:
5.000 took 3 seconds
10.000 took 14 seconds
20.000 took 59 seconds

So again i stripped out the GetFirstCellOfRow in line 2785:
Code: [Select]if false and (ASheet.GetFirstCellOfRow(r) = nil) then
begin5.000 took 0 seconds
10.000 took 1 seconds
20.000 took 2 seconds

You don't have these kind of delays at all??? (with sample code below)
I understand (if you don't have these kind of delays) debugging them
is an almost impossible task.

If this is something specific to my computers you wouldn't even know
where to begin.
I'll try to figure out where in those function the trouble is for me
but i'm guessing somewhere in the TAVLTreeNode.


Sample code:
Clean form whith one button and one label:
Code: [Select]uses
  fpspreadsheet, LclIntf;

procedure TForm1.Button1Click(Sender: TObject);
var
  MyWorkbook: TsWorkbook;
  MyWorksheet: TsWorksheet;
  i, j: integer;
  Tm: DWORD;
begin
  MyWorkbook := TsWorkbook.Create;
  MyWorksheet := MyWorkbook.AddWorksheet('My Worksheet');
  for i := 0 to 20000 do
  begin
    Label1.Caption := IntToStr(i);
    Application.ProcessMessages;
    for j := 0 to 100 do
    begin
      MyWorksheet.WriteUTF8Text(i, j, 'aj' + IntToStr(i) + ' ' +
IntToStr(j));
    end;
  end;
  ShowMessage('ready #1');
  Tm := GetTickCount;
  MyWorkbook.WriteToFile('c:\temp\test' +
STR_OPENDOCUMENT_CALC_EXTENSION, sfOpenDocument);
  //MyWorkbook.WriteToFile('c:\temp\test' + STR_OOXML_EXCEL_EXTENSION,
sfOOXML);
  //MyWorkbook.WriteToFile('c:\temp\test' + STR_EXCEL_EXTENSION,
sfExcel8, true);
  ShowMessage('ready #2 ' + inttostr((GetTickCount - Tm) div 1000));
  MyWorkbook.Free;
  Close;
end;

----------------------------------------------------------------------
BigChimp · 2014-07-09 12:30

Quote from: marcov on July 08, 2014, 12:57:24 pmQuote from: wp on July
08, 2014, 12:41:14 pmBTW if even Excel writes forward slashes are
there any programs at all which write a backslash?

Seems old winzips did.   http://www.info-zip.org/FAQ.html#backslashes
Looking at http://web.archive.org, that page started to get that
backslash info between beginning of 2004, and beginning of 2005. Even
then, those winzips were already "old".

Quote from: marcov on July 08, 2014, 12:57:24 pmStill it would be wise
to have a passthrough (write as user specificies), in case sb has to
live with an app that really wants it.  But that can be disabled by
default.
1. The fpc zip code only allowed \ when running on Windows, not on
Unix, so it wasn't supported cross-platform anyway
2. The standard requires use of / and the info-zip page does not
mention that those winzips could not read the standard / but
absolutely required the \.
3. With current trunk, if an application adds > <large amount of
files> (sorry, can't remember count), currently zip64 format is
automatically used. Those old utilities will not support that and
error out anyway.

Adding an option that allows people to deviate from the standard only
for very specific cases seems a bit too conservative (and prone to
error due to people tweaking that option without clue).

Edit: updated typos. Thanks rvk
I'd suggest still accepting \ on Windows for reading (as it is
currently done) but forcing / on writing on Windows as well, then
document the more strict adherence on FPC User changes trunk.

Currently working on a patch including test cases that does this...
Of course, if you do want to add some override to it... well, you're
one of the guys who have commit rights there, so I'm certainly open to
suggestions

----------------------------------------------------------------------
BigChimp · 2014-07-09 12:33

Quote from: rvk on July 09, 2014, 02:23:24 pmIf this is something
specific to my computers you wouldn't even know where to begin.
I'll try to figure out where in those function the trouble is for me
but i'm guessing somewhere in the TAVLTreeNode.
You're doing such a good job but perhaps you weren't aware of this
http://wiki.lazarus.freepascal.org/FPProfiler
http://wiki.lazarus.freepascal.org/Profiling
in case you didn't, just FYI...

Groeten,
BigChimp

----------------------------------------------------------------------
rvk · 2014-07-09 12:49

Quote from: BigChimp on July 09, 2014, 02:30:36 pmI'd suggest still
accepting / on Windows for reading (as it is currently done) but
forcing \ on writing on Windows as well, then document the more strict
adherence on FPC User changes trunk.I take it you mean the other way
around? Accepting \ (backslash) for reading and forcing / (forward
slash) for writing on Windows because the zip-standard nowadays is /
for zips (for all platforms).
(otherwise we would still get in trouble with LibreOffice which only
accepts / (forward slash) on all platforms)

Quote from: BigChimp on July 09, 2014, 02:33:05 pmin case you didn't,
just FYI...I wasn't aware of profiling options in Lazarus (I'm mainly
a "Delphi"-guy  ) but i'll definitely take a look.

----------------------------------------------------------------------
taazz · 2014-07-09 12:54

Quote from: rvk on July 09, 2014, 02:49:42 pm(otherwise we would still
get in trouble with LibreOffice which only accepts / (forward slash)
on all platforms)

That is something that needs to be reported on LibreOffice and no
forcing linux path delimeters on windows users is not acceptable just
because of libreoffice.

----------------------------------------------------------------------
BigChimp · 2014-07-09 12:58

Quote from: rvk on July 09, 2014, 02:49:42 pmQuote from: BigChimp on
July 09, 2014, 02:30:36 pmI'd suggest still accepting / on Windows for
reading (as it is currently done) but forcing \ on writing on Windows
as well, then document the more strict adherence on FPC User changes
trunk.I take it you mean the other way around? Accepting \ (backslash)
for reading and forcing / (forward slash) for writing on Windows
because the zip-standard nowadays is / for zips (for all platforms).
(otherwise we would still get in trouble with LibreOffice which only
accepts / (forward slash) on all platforms)
Yes, thanks, sorry for the typo, updated the post.

----------------------------------------------------------------------
BigChimp · 2014-07-09 13:00

Quote from: taazz on July 09, 2014, 02:54:11 pmThat is something that
needs to be reported on LibreOffice and no forcing linux path
delimeters on windows users is not acceptable just because of
libreoffice.
I think you're missing the fact that we're talking about how paths are
stored inside ZIP files (xlsx and ods are zip files containing defined
data files). These require / as path delimiter; see rvk's post linking
to the standard (appnote.txt).

fpspreadsheet has already been changed to use / instead of \ so
LibreOffice correctly opens fpspreadsheet-generated xlsx files - see
wp's post on that in this thread.

Edit:
However, if you want to report the bug that LibreOffice won't open an
invalid xlsx file (because it has \ instead of /) while Excel will
open it (see first posts in this thread)... well, please feel free.

----------------------------------------------------------------------
rvk · 2014-07-09 13:00

Quote from: taazz on July 09, 2014, 02:54:11 pmThat is something that
needs to be reported on LibreOffice and no forcing linux path
delimeters on windows users is not acceptable just because of
libreoffice.
The standard of path-delimiters within zip-files is / nowadays
regardless of OS.
So forcing / in zipper.pp would not be "forcing linux path delimiters
on Windows users" but it would be enforcing the standard of zip-format
for Windows users in zips created by zipper.

----------------------------------------------------------------------
wp · 2014-07-09 14:50

Coming back to the speed measurement:

rvk, I am using your test program now. I'm at a slow computer at the
moment, therefore I do only 5000 rows. Here are my results:

* with "GetFirstCellOfRow" (line 2785), with "Result := ..." (line
2841) --> 25 sec
* "GetFirstCellOfRow" deactivated by "and" with "false", with "Result
:= ..." --> 25 sec
* with "GetFirstCellOfRow", "Result := .." commented out --> 1 sec
* "GetFirstCellOfRow" deactivated, "Rest := ..." commented out --> 1
sec

In an additional test, I just exit the WriteCellCallback immediately
after entering it, such that the many string concatenations mentioned
above are not executed --> 0sec

So: it is clear to me: the reason for the slow writing performance is
in the many string operations of extremely long strings.

Give me some time until I have a preliminary version of xlsxooxml. I
guess we will be very surprised.

----------------------------------------------------------------------
rvk · 2014-07-09 15:30

Quote from: wp on July 09, 2014, 04:50:32 pmSo: it is clear to me: the
reason for the slow writing performance is in the many string
operations of extremely long strings.
Ok... so you're not seeing the same delays as me. No problems.
(If they continue after the changes i'll investigate further. or
before if i get some time)

Quote from: wp on July 09, 2014, 04:50:32 pmGive me some time until I
have a preliminary version of xlsxooxml. I guess we will be very
surprised.
Can't wait... but take your time... good programming needs to be done
with care

Then the only problem left (my problem) would be the memory hungry
TsWorksheet.
I'm wondering if parts of something like that couldn't be swapped to
disc if not used.
(I imagine Excel and LibreOffice would do the same)
I'll try to gather some info on that possibility (and work on that
idea some more).

If the DOM version is more optimal with memory you could even
completely replace the whole TCell-structure with nodes. In that case
you would read the properties of a cell from the cell-node. Default
values would not take any space. Reading files into it and changing
data and writing back would result in exactly the same file (with
changes) but unsupported features would stay intact. Don't worry....
I'm daydreaming a little bit   That would mean a massive rewrite of
the core TsWorksheet and have problems with cross-version
(ods/xls/xlsx, etc).

But while daydreaming that... The TCell structure could be transformed
to a sort of TList of properties. When you don't need a certain
property it won't take any space because it's not defined yet in that
cell. If i don't use Border, BorderStyles, BackgroundColor etc it
wouldn't take space in TCell. It would mean a TCell could shrink from
a minimum of 112 bytes to just a few bytes (if only a few properties
are used). (Just spitballing  )

----------------------------------------------------------------------
marcov · 2014-07-09 21:12

Quote from: rvk on July 09, 2014, 02:23:24 pm You don't have these
kind of delays at all??? (with sample code below)

Note that the delays are depending on the chance that the
memorymanager has to actually reallocate the block if the current runs
out of space.

This means that switching memorymanager can have a big effect.
(sharemem, cmem or default of FPC 2.6 or 2.7)

----------------------------------------------------------------------
marcov · 2014-07-09 21:21

Quote from: BigChimp on July 09, 2014, 02:30:36 pm1. The fpc zip code
only allowed \ when running on Windows, not on Unix, so it wasn't
supported cross-platform anyway

True, but if the user delivers backspaces on *nix he does it with a
purpose. So following that logic you wouldn't even have to force it.

Quote2. The standard requires use of / and the info-zip page does not
mention that those winzips could not read the standard / but
absolutely required the \.

Flogging a standard at a binary a previous employee of your company
left you without source is kinda useless. I

Quote3. With current trunk, if an application adds > <large amount of
files> (sorry, can't remember count), currently zip64 format is
automatically used. Those old utilities will not support that and
error out anyway.

Good point. So that needs a switch to disable too.

QuoteAdding an option that allows people to deviate from the standard
only for very specific cases seems a bit too conservative (and prone
to error due to people tweaking that option without clue).

It's a matter of chance. I don't see it so far fetched. Moreover those
sleeping booleans are not that bad. Inited to false by default, two
extra bytes for the class, a handfull of places to test them, one or
two instructions each.

QuoteCurrently working on a patch including test cases that does
this...
Of course, if you do want to add some override to it... well, you're
one of the guys who have commit rights there, so I'm certainly open to
suggestions

IIRC I also committed zip64? :-)

----------------------------------------------------------------------
wp · 2014-07-09 21:39

Totally confused...

In xlsxooxml.pas I replaced the cell writing string operations by DOM
nodes. But it's getting worse! In rvk's speed test the old code takes
about 15 sec for saving 10000 rows, the task manager shows a little
increase of memory usage. The new code which I am adding in the
attachment, however, needs about 35 sec with a clear increase of
memory usage.

----------------------------------------------------------------------
Mike.Cornflake · 2014-07-10 06:14

Quote from: wp on July 09, 2014, 11:39:04 pmTotally confused...

In xlsxooxml.pas I replaced the cell writing string operations by DOM
nodes. But it's getting worse! In rvk's speed test the old code takes
about 15 sec for saving 10000 rows, the task manager shows a little
increase of memory usage. The new code which I am adding in the
attachment, however, needs about 35 sec with a clear increase of
memory usage.

Hmmm :-(  I had similar slowdowns when I tried using XML DOM with the
docx writer, but I had high hopes that it was just me doing something
wrong (I'd never used XML DOM before).   For that writer I switched to
using a TStringList for each file instead of doing those large string
concatenations.   Maybe you could try that?

http://svn.freepascal.org/svn/lazarus/trunk/components/fpvectorial/doc
xvectorialwriter.pas

(And sure, you don't need the indenting stuff, I just introduced that
to assist debugging the resulting files...)

There's also a few fast xml libraries around, I considered using
those, but rejected it on the basis that I didn't want to introduce
extra dependencies...

----------------------------------------------------------------------
rvk · 2014-07-10 08:08

For me... your code results in:
5.000x100 cells: 11 seconds
10.000x100 cells: 28 seconds
20.000x100 cells Out of memory error

The clear memory increase was to be expected. DOM also uses lots of
memory during buildup.

Question aside... I see you used the DOM nodes for the sheets... but
what about the FSharedStrings??
In my example i only write strings to the cells (not numbers).
With this the FSharedStrings is being build up. (Numbers go directly
in the sheet.xml and strings go to the SharedStrings.xml with a
reference in the sheet.xml)

So if you're still using the FSharedString there is still some
expensive string building going on.
(With a very large FSharedStrings, when using lots of string instead
of numbers)

As a further note... Filling the DOM Nodes is expensive in time too.
The two greatest advantages of using DOM is simplicity (for
reading/writing the nodes) and compatibility (not needing to care
about XML). But in this case DOM is only being used for writing. There
is no need for reading the DOM nodes once they are written to memory.
So one of the biggest advantages is not being used and only the
compatibility (not caring about XML) remains.

But the biggest disadvantage is memory usage. So maybe this does not
stand up against the advantage of compatibility.

So you could try (one more time) to include the FSharedStrings but
otherwise it would be back to the drawing-board:
(considering the memory usage maybe that's best because in this case i
couldn't even write 20.000 rows)

I do have some other thoughts about writing more efficiently:
In the other TsCustomSpreadWriter (like biff8 etc), all except
xlsxooxml and fpsopendocument, i see there is a TMemoryStream being
used. Why was this not used in the other two???

If TMemoryStream is more efficient and faster maybe that one should be
used. (It would be easier to implement than the DOM Nodes.)

If it is faster there could also be a switch added (very easily) to
not use memory during writing. The TMemoryStream could be substituted
for a TFileStream for example to write to temporary files so
absolutely no memory would be used (just as an option).


Edit: Microsoft has a nice article about "How To Improve String
Concatenation Performance"

----------------------------------------------------------------------
wp · 2014-07-10 09:03

Thanks Mike - this is a very good point. There is marginal improvement
(10%) for 1000 rows, but the speed increase becomes dramatic the more
rows are in the worksheet. 20000 is the maximum that I can do on this
computer here, and the writing time decreases from 150 sec to 33
(comparing with release version of xlsxooxml). A significant
improvement came from putting the shared strings into the stringlist.

I still have to study memory usage in detail, but I am a bit concerned
about the zipper which seems to require data in streams. Therefore the
stringlist has to be written to a stream - xlmxooxml uses a
TStringStream for this purpose. Don't I duplicate memory consumption
temporarily when I call TStringList.Text for passing it to the
constructor of the StringStream? Or is there a way to plug the
StringList into the zipper directly, without using a stream?

I tried a temporary FileStream to store the string list and clear the
string list afterwards. The fileStream is used by the zipper. This
costs a few seconds (42 sec writing time for 20000 rows).

Another idea: Using a file stream altogether, even instead of the
string list, would have the benefit to have no limitation on worksheet
size. We could provide some kind of "transparent mode" in which the
data are not stored in the worksheet, but are passed to the writer by
means of an event such as "OnGetRowData". For storing a large database
in xlsx, for example, you could open the database before calling
"WriteToFile" and then hook into this event to read the next record
and pass it to the writer which stuffs rows and sharedstring into the
corresponding files and re-reads them for zipping. It would be a bit
slower, though.

The attachment contains two versions: one using a stringlist for
collecting the xml strings of the sheets and sharedstrings and using
that in the zipper, and another one in which the stringlists are saved
to temporary files which are re-read by the zipper.

----------------------------------------------------------------------
rvk · 2014-07-10 09:23

I just tried the following and it got me from 48 seconds down to 5
(with 20.000x100 cells).
(with the original rev.3301)

I did it very quickly just to see what the performance of
AStream.WriteAnsiString(s) is.

Code: [Select]  //FSharedStrings := FSharedStrings +
  //        '  <si>' + LineEnding +
  // Format('    <t>%s</t>', [UTF8TextToXMLText(ResultingValue)]) +
LineEnding +
  //        '  </si>' + LineEnding;
  s :='  <si>' + LineEnding +
   Format('    <t>%s</t>', [UTF8TextToXMLText(ResultingValue)]) +
LineEnding +
          '  </si>' + LineEnding;
  AStream.WriteAnsiString(s);

and

Code: [Select]  //FSheets[FCurSheetNum] := FSheets[FCurSheetNum] +
  // Format('    <c r="%s" s="%d" t="s"><v>%d</v></c>', [CellPosText,
lStyleIndex, FSharedStringsCount]) + LineEnding;
  s := Format('    <c r="%s" s="%d" t="s"><v>%d</v></c>',
[CellPosText, lStyleIndex, FSharedStringsCount]) + LineEnding;
  AStream.WriteAnsiString(s);

In WriteWorksheet i created an AStream = TMemoryStream which i gave to
WriteCellCallback.

It doesn't result in a valid file but it was just to see if multiple
calls to .WriteAnsiString would be fast (and it is).

For good messure i did a  AStream.SaveToFile('c:\temp\test.txt')
before a AStream.Free to see if the file was indeed big
and it was 191 MB so... yeah... AStream.WriteAnsiString would seem to
be very fast.

And with these streams (there would be a need for several) you could
call zipper directly.
(Although an option to use a temporary file would still be a good
bonus)

----------------------------------------------------------------------
marcov · 2014-07-10 09:47

Quote from: wp on July 10, 2014, 11:03:44 amThanks Mike - this is a
very good point. There is marginal improvement (10%) for 1000 rows,
but the speed increase becomes dramatic the more rows are in the
worksheet. 20000 is the maximum that I can do on this computer here,
and the writing time decreases from 150 sec to 33 (comparing with
release version of xlsxooxml). A significant improvement came from
putting the shared strings into the stringlist.

(that will slow down too, only when in the hundreds of thousands.

QuoteI still have to study memory usage in detail, but I am a bit
concerned about the zipper which seems to require data in streams.
Therefore the stringlist has to be written to a stream - xlmxooxml
uses a TStringStream for this purpose. Don't I duplicate memory
consumption temporarily when I call TStringList.Text for passing it to
the constructor of the StringStream?

Yes.

Quote Or is there a way to plug the StringList into the zipper
directly, without using a stream?

No, you need to copy it to a stream first.

If your main operation on the big string is appending, then simply
make a custommemorystream that has an array of buffers. Add an
"appendstring"  method, and fill a buffer till it has a certain size
(say 1MB), then go to the next block. You only need strings for this
if you really need many string ops.

In the stream routine, simply read from the blocks using  block nr
(position div blocksize)  at position (position mod blocksize).

In general stay away from anything with a block that can get large and
with bad growth strategies. Even TStringlist (with its single block of
line pointers) can get slow. I ran into that once and created the
lightmap routines.

My proposal above has the same problem, but

(1) has constant block size
(2) blocksize considerably larger than a line and can be made
configurable.
(3) is a stream.

so in practice they can get awfully big.

----------------------------------------------------------------------
Mike.Cornflake · 2014-07-10 10:44

Quote from: wp on July 10, 2014, 11:03:44 amDon't I duplicate memory
consumption temporarily when I call TStringList.Text for passing it to
the constructor of the StringStream? Or is there a way to plug the
StringList into the zipper directly, without using a stream?

I use a TMemoryStream, and use TStringList.SaveToStream to populate
the TMemoryStream instead of using TStringList.Text.  But yeah, I
think you're right about the duplicate memory...

----------------------------------------------------------------------
rvk · 2014-07-10 10:49

I'm testing the writing via TMemoryString.
Don't use TStringStream to append strings because that is also slow.

I've attached an almost working copy of xlsxooxml.pas which uses the
original FSSharedStrings and FSSheets but i changed them to
TMemoryStreams and created them in WriteGlobalFiles and WriteWorksheet
so they could be used to do .WriteBuffer in the write process.

I got the speed down from 48 seconds to 5 seconds with 20.000x100
cells.

Two things...
First... I needed to use WriteBuffer because there is no WriteString
in TMemoryStream. (I think i did it the right way)

Second... The FSharedStrings needs a count in the beginning.
I think i solved this in WriteToStream by creating an extra
TMemoryStream (_complete) where i copied all thogheter.

Only problem i have left is that the FSSheet is 0 bytes in the
resulting .xlsx while the SaveToFile of FSSheet[0] does contain
information.

But... I'm i on the right track here????


Either way... i can't change the production-version of fpspreadsheet
thus you'll need to implement these changes if they are useful.

----------------------------------------------------------------------
marcov · 2014-07-10 10:52

Tmemorystream is still one single block. You don't want to allocate
1GB+ blocks from the heapallocator, which is why I suggested something
tmemorystream like but with multiple blocks.

----------------------------------------------------------------------
rvk · 2014-07-10 11:06

I've got a working copy of xlsxooxml.pas with TMemoryStream
implemented (attached).
The memory allocation of 20.000x100 cells would be around 180MB.
(That's the uncomressed .xlsx of course)

Writing the 20.000x100 cells is done in 10 seconds on my machine (with
equally amount of memory with the string+string method but much
faster).

I had some problems because i didn't set .position to 0 after writing
to the stream and zipper needs the .position of a stream to be 0
otherwise it will only write from that position. Is this a bug in
zipper? Or is it supposed to only write from .position of a stream ???
(a stream.position := 0 would be easy to insert there   )

Finally an option to use TFileStream instead of a TMemoryStream would
also be nice. The user would have the possibility to spare even more
memory. But as it is now the TMemoryStream option is preferred above
string + string handling (and much faster)

Also those LineEnding for every line are not necessary in XML. (I
added a const LineEnding = ''; and had no problems reading it.
(it would only serve purpose if you'r debugging the files)

(now for somebody to implement and test this in the production-
version  )

(Something similar would also needed to be done in fpsopendocument.pas
because that one is even slower)

----------------------------------------------------------------------
marcov · 2014-07-10 11:32

Quote from: rvk on July 10, 2014, 01:06:51 pmI had some problems
because i didn't set .position to 0 after writing to the stream and
zipper needs the .position of a stream to be 0 otherwise it will only
write from that position. Is this a bug in zipper? Or is it supposed
to only write from .position of a stream ??? (a stream.position := 0
would be easy to insert there   )

Yes. The position is part of the stream.  Though I've thought about
making a special copyfrom method that does this automatically, to
circumvent this very common mistake .

QuoteFinally an option to use TFileStream instead of a TMemoryStream
would also be nice. The user would have the possibility to spare even
more memory. But as it is now the TMemoryStream option is preferred
above string + string handling (and much faster)

Ideally you need to write your data (in blocks) directly to the
compressor that tosses them directly into the filestream. ZIP has its
TOC at the end to make this easy.

----------------------------------------------------------------------
rvk · 2014-07-10 11:39

Quote from: marcov on July 10, 2014, 01:32:55 pmIdeally you need to
write your data (in blocks) directly to the compressor that tosses
them directly into the filestream. ZIP has its TOC at the end to make
this easy.Directly from for example WriteLabel ?
That can't be done because the FSSheet is build at the same time as
FSSharedStrings.
And at the top of it, the beginning of FSSharedStrings needs a count-
number in the XML (which we only get after building it).
(Or we would need two passes over the worksheet)

I think writing to these TMemoryStreams (as in my last example)
instead of to the TMemoryStream to a TFileStream (temporary file) and
after that concatenating them to zipper would be the most we can get
(memory-efficiency wise).

----------------------------------------------------------------------
wp · 2014-07-10 11:54

rvk, you're faster than me...

A proposal to make things easier to read: Instead of
"WriteBuffer(Pointer(S)^, Length(S))" I'd prefer to have this hidden
in a procedure (like Marco suggested)

Code: [Select]procedure AppendToStream(AStream: TStream; const
AString: String);
begin
  // AStream.WriteBuffer(Pointer(AString)^, Length(AString));
  // I can't help - this looks kind of wrong to me - does the pointer
really point to the
  // first character? But obviously it is correct.
  AStream.WriteBuffer(AString[1], Length(AString));
end;

Maybe also overloaded with two and/or three strings which could save
another few + operations:
Code: [Select]procedure AppendToStream(AStream: TStream; const
AString1,AString2: String); overload;
begin
  AStream.WriteBuffer(AString1[1], Length(AString1));
  AStream.WriteBuffer(AString2[1], Length(AString2));
end;

procedure AppendToStream(AStream: TStream; const
AString1,AString2,AString3: String); overload;
begin
  AStream.WriteBuffer(AString1[1], Length(AString1));
  AStream.WriteBuffer(AString2[1], Length(AString2));
  AStream.WriteBuffer(AString3[1], Length(AString3));
end;

This should go into fputils to have it available from other units as
well.

Will you do the fpopenspreadsheet also? No problem for me to update
it, but I just want us to avoid duplicate work.

----------------------------------------------------------------------
marcov · 2014-07-10 12:37

Quote from: wp on July 10, 2014, 01:54:39 pmrvk, you're faster than
me...

A proposal to make things easier to read: Instead of
"WriteBuffer(Pointer(S)^, Length(S))" I'd prefer to have this hidden
in a procedure (like Marco suggested)

Typical Class helper.... To make reusable code, check for
length(astring)=0. Probably writebuffer does already, but better safe
than sorry.

QuoteCode: [Select]procedure AppendToStream(AStream: TStream; const
AString: String);
begin
  // AStream.WriteBuffer(Pointer(AString)^, Length(AString));
  // I can't help - this looks kind of wrong to me - does the pointer
really point to the
  // first character? But obviously it is correct.
  AStream.WriteBuffer(AString[1], Length(AString));

Empty strings point to a constant somewhere in the RTL to make this
possible. So yes this will work if length=0 because the astrings[1]
won't GPF and hopefully writebuffer doesn't do a thing.

QuoteMaybe also overloaded with two and/or three strings which could
save another few + operations:

Simply make it inline?

----------------------------------------------------------------------
rvk · 2014-07-10 12:44

Quote from: wp on July 10, 2014, 01:54:39 pmCode: [Select]procedure
AppendToStream(AStream: TStream; const AString: String);
procedure AppendToStream(AStream: TStream; const AString1,AString2:
String); overload;
procedure AppendToStream(AStream: TStream; const
AString1,AString2,AString3: String); overload;

Couldn't that also just be done with one procedure?
Like this:
Code: [Select]procedure AppendToStream(AStream: TStream; const
AString1: string;
   const AString2: string = ''; const AString3: String = '');
begin
  AStream.WriteBuffer(AString1[1], Length(AString1));
  if AString2 <> '' then AStream.WriteBuffer(AString2[1],
Length(AString2));
  if AString3 <> '' then AStream.WriteBuffer(AString3[1],
Length(AString3));
end;

Quote from: wp on July 10, 2014, 01:54:39 pmWill you do the
fpopenspreadsheet also? No problem for me to update it, but I just
want us to avoid duplicate work.
I programmed this really "quick and dirty". I'm not entirely sure i
didn't make any mistakes (but could read the result).

For instance... I'm not sure we can just take Length(S) to write to
the buffer. If the string is Unicode (?) Length is the wrong function,
i think.
(you see i'm a beginner with FPC) Shouldn't we be using UTF8Length(s)
????
(Edit: I'm just seeing Length does give the correct sizeofbytes,
instead of count of characters)

Further... maybe it would be easier to define the TMemoryStream in the
header as TStream (like it is done in zipper itself) and only using
the TMemoryStream to create the variable.
Code: [Select]    FSSharedStrings: TStream; // Stream instead of
stringStream
    FSSheets: array of TStream; // Stream instead of stringStream
and
Code: [Select]  if UseMemoryStream then
    FSSharedStrings := TMemoryStream.Create
 else
    FSSharedStrings := TFileStream.Create('c:\temp\temp.xml', fmCreate
or fmOpenWrite);
 // ... etc
for example (with correct temp-filenames of course).
That changed the time to 17 seconds for 20.000x100 and 34 seconds for
40.000x100. (so it is somewhat slower)
There should still be work done, which i gladly leave to the experts,
for creating the files (and determining the filenames) etc...
I also didn't figure out how the FSharedStrings_pre and _post could be
integrated in the TFileStream before handing it off to zipper.

BTW. Is there a TFileStream which has some buffering capability ??

So, i would be more comfortable if someone took a critical look at my
changes before implementing them 

I'm also not yet read up on the fpopenspreadsheet.pas other then
scanning through it.
(So you could go ahead and dive in that one)

----------------------------------------------------------------------
marcov · 2014-07-10 13:07

Quote from: rvk on July 10, 2014, 02:44:10 pmFor instance... I'm not
sure we can just take Length(S) to write to the buffer. If the string
is Unicode (?) Length is the wrong function, i think.

No we want to write bytes. That is 1*length(s) for 1-byte strings, and
2*length(s) for twobyte strings.

How it is encoded doesn't matter (since length always returns encoding
units, never characters).

Btw, a memory stream increases in blocks of 4k.

----------------------------------------------------------------------
wp · 2014-07-10 13:18

Quotei would be more comfortable if someone took a critical look at my
changes before implementing themNo problem, I'll have a look in the
evening when I have more time and then upload it to svn.

I think I'll remove the LineEndings completely. Having it as an empty
string in one unit is too confusing.

QuoteI'm not sure we can just take Length(S) to write to the bufferI
can fully understand your confusion. Strings have become the most
complicated part of the once so simple language Pascal...

----------------------------------------------------------------------
marcov · 2014-07-10 13:35

Quote from: wp on July 10, 2014, 03:18:25 pmQuoteI'm not sure we can
just take Length(S) to write to the bufferI can fully understand your
confusion. Strings have become the most complicated part of the once
so simple language Pascal...

But also its strength. Anyway if you are not confused yet, have a look
at http://www.stack.nl/~marcov/delphistringtypes.txt

----------------------------------------------------------------------
rvk · 2014-07-10 14:05

Quote from: wp on July 10, 2014, 03:18:25 pmNo problem, I'll have a
look in the evening when I have more time and then upload it to svn.
I used rev.3304 for this.
Attached my last one with TStream in the header instead of
TMemoryStream (in preparation of an TFileStream or another TStream
variant)
Also in the zip an extra .diff and a screenshot of the differences
between my version and the rev.3304 one.
(.diff made with ExamDiff. I'm not sure yet how to make a "proper"
patch-file with svn ).


To be done (by you at some point):
removing all LineEnding because they are not necessary in XMLremoving
all extra trailing and leading spaces for the same reason and to get
clean small xml'ssome rudimentary testingimplementing something
similar for fpsopendocument.pas
At a later stage we could look at the possibility to use TFileStream
but we need to find one with some better buffering mechanism because
the standard one is writing everything to file piece by piece and
slow.

edit: btw. All the other strings (like FContentTypes, FRelsRels,
FWorkbookRelsString, FWorkbookString and FStylesString) could of
course also be converted to TStream. In the header these TStreams are
already defined (like FSContentTypes... etc... while they are only
used in WriteToStream). So why not put them to use at the beginning so
there is no need for the strings anymore? They don't have much impact
speed-wise but it would be more consistent in the code.

----------------------------------------------------------------------
marcov · 2014-07-10 15:42

fyi: Attached is the unit I use at work to get Lazarus code running
under Delphi. (XE/XE3)

----------------------------------------------------------------------
rvk · 2014-07-10 15:53

Quote from: rvk on July 10, 2014, 04:05:24 pmedit: btw. All the other
strings (like FContentTypes, FRelsRels, FWorkbookRelsString,
FWorkbookString and FStylesString) could of course also be converted
to TStream. In the header these TStreams are already defined (like
FSContentTypes... etc... while they are only used in WriteToStream).
So why not put them to use at the beginning so there is no need for
the strings anymore? They don't have much impact speed-wise but it
would be more consistent in the code.
I just changed all FContentType etc. strings to TMemoryStream's.
(attached)

Like i expected it doesn't change anything time-wise but maybe you'll
find the code more consistent.
(also used a AppendToStream like discussed but it's still in the
xlsxooxml.pas so may need to be moved to the string-unit)

I'll leave the rest up to you... like cleaning up the LineEnding's.

I'm still no expert in Lazarus/FPC (i'm more used to Delphi and its
IDE) but i'm getting the hang of it  )


Edit: Haha... didn't know FPC also had class helpers.
I changed the AppendToStream to TStream.AppendString(S) like marcov
suggested.
In xlsxooxml_withstreamclasshelper.zip

----------------------------------------------------------------------
wp · 2014-07-10 16:08

I did not see all these last postings while working with the ooxml,
sorry, I just uploaded my version based on rvk's a few postings above
and with the following modifications:
Use AppendToStream to compose the xml strings in a streamMaybe a
little unusual, but I indented the strings to be written in the second
line of the AppendToStream in order see their hierachy level.Provide a
variable for the stream class. This prepares for later switching to a
file stream or for going to Marco's idea with the blocks. All (well,
most) streams are created and destroyed in a single method.Line
endings removedLeading spaces removed.Files can be opened by Excel2007
and LibreOffice (don't have OpenOffice here)Speed seems to be at the
same level as mentioned by rvk (although I cannot reproduce his
numbersexactly due to hardware differences, of course).

----------------------------------------------------------------------
rvk · 2014-07-10 16:31

It works (rev.3305)
Speed is good now.

10.000x100 cells 5 seconds
20.000x100 cells 10 seconds
30.000x100 cells 15 seconds
40.000x100 cells 20 seconds
So as far as i can see about 5 seconds per 10.000x100 cells.

LibreOffice is a bit slow opening large files (from .xlsx) but it
always has been.
Even Excel has some slowdown opening/writing a 40.000x100 cells
spreadsheet.

For now i think we can be happy with the current speed-boost 

(I wonder if you can get the same speed-boost with sfOpenDocument  )

----------------------------------------------------------------------
wp · 2014-07-10 18:36

@Marco, as I said above I did not read many of todays postings
carefully enough. Two questions:
Do you know an example of a memory stream which works with blocks?What
is the advantage of using a class helper except for the elegance of
extending an existing class? Is there a speed or memory advantage? The
currently used, inline procedure "AppendToStream" is called millions
of times, so I don't want to sacrifice anything for elegance.

----------------------------------------------------------------------
marcov · 2014-07-10 19:01

wp: don't expect performance improvements. In theory when inlined
there shouldn't be a difference at all I guess.

----------------------------------------------------------------------
rvk · 2014-07-10 20:04

Quote from: wp on July 10, 2014, 08:36:51 pmDo you know an example of
a memory stream which works with blocks?I'm still looking for a
TMemoryStream which would swap its data to disc if for example it
reaches 80% of the available memory (or something).

I did find a TFileStream example where there is a buffer before
writing chunks to disc but didn't try to implement it yet.
(TaaWriteBufferFilter but that source is a bit broken and i couldn't
find an official source. Maybe here)
I have yet to look at tJCLBufferedStream.

One downside is that you would need to determine in advance if you
would like to use memory or disc. I would much rather have a sort of
automatic mechanism of backup to disc only when needed in a
TMemoryStream descendant which could be disabled/enabled at a given
threshold. (It would depend on the performance of a replacement
TFileStream i need to find. If that one is almost as fast as
TMemoryStream there would be no need to do this)

Also this article also has some nice information about TMemoryStream
and its possible OutOfMemory errors. I' not sure if the inner workings
are the same for FPC but it's an interesting read.

(I wonder if Windows shouldn't be able to swap some of the
TMemoryStream to disc when needed)

----------------------------------------------------------------------
marcov · 2014-07-11 07:14

In 64-bit you could consider memory mapping. If you can set its
affinity to keep it in memory but make it the first to be swapped,
that would be a good fit for our problem (if memory enough, it stays
in mem). In 32-bit too, but since memory mapping takes address space
(even if not mapped by memory)

OTOH how that practically works out (and then specially the multi
platform angle) is hard to say, you risk the system paging everything
in just to unmap etc.

Basically that last .NET url is the same as my block proposal, only
they use 4kb blocksize, and I took 1MB as initial blocksize. 
(probably best is to use n*4k on 32-bit and n*8k on 64-bit with n
adjustable). I'll see if I can whip up something in the weekend. But
that is still all in memory.

----------------------------------------------------------------------
BigChimp · 2014-07-11 11:50

Quote from: marcov on July 09, 2014, 11:21:00 pmQuote from: BigChimp
on July 09, 2014, 02:30:36 pm1. The fpc zip code only allowed \ when
running on Windows, not on Unix, so it wasn't supported cross-platform
anyway

True, but if the user delivers backspaces on *nix he does it with a
purpose. So following that logic you wouldn't even have to force it.
Sorry, I don't understand what you're saying here. Do you mean
backslashes? Both the current code and my patch leave backslashes
alone on unix.
Asking windows users to use / to specify paths for files to be zipped
is very unexpected and I think most people would see it as a bug, if
that was your suggestion. Converting \ to / seems quite sensible.

Quote from: marcov on July 09, 2014, 11:21:00 pmQuote2. The standard
requires use of / and the info-zip page does not mention that those
winzips could not read the standard / but absolutely required the \.

Flogging a standard at a binary a previous employee of your company
left you without source is kinda useless.
Building in elaborate backward compatibility mechanisms to match non-
standard behaviour that has not even been seen in the wild is not good
either. As said, those binaries should not even be able to accept /
and there's no evidence of any of that.

IMO, adding an option that allows people to deviate from the standard
only for very specific cases is a bit too conservative (and prone to
error due to people tweaking that option without clue).
If backward compability is needed, people can take a private copy of
the zipper code; see suggested remarks for user changes trunk page in
bug report.

Quote from: marcov on July 09, 2014, 11:21:00 pmQuote3. With current
trunk, if an application adds > <large amount of files> (sorry, can't
remember count), currently zip64 format is automatically used. Those
old utilities will not support that and error out anyway.

Good point. So that needs a switch to disable too.
No, because falling back to the previous behaviour of silently
corrupting the offset pointers in the zip file is not an improvement
IIRC, I indicated that when submitting the original patch.

Quote from: marcov on July 09, 2014, 11:21:00 pmQuoteCurrently working
on a patch including test cases that does this...
Of course, if you do want to add some override to it... well, you're
one of the guys who have commit rights there, so I'm certainly open to
suggestions

IIRC I also committed zip64? :-)
Yes, that's why I'm optimistic
Have a look at
http://bugs.freepascal.org/view.php?id=26468
please.

Thanks.

----------------------------------------------------------------------
BigChimp · 2014-07-11 12:53

Quote from: BigChimp on July 11, 2014, 01:50:58 pmHave a look at
http://bugs.freepascal.org/view.php?id=26468
please.
Michael Van Canneyt just committed it; thanks!

----------------------------------------------------------------------
wp · 2014-07-12 21:55

Direct writing of xml strings to a memory stream has now been
implemented also for the ODS writer (rev 3311). Again extreme speed
improvement: 20.000 x 100 cells write in 7 seconds now, compared to
120 with the old code.