XLSX document created by FPSpreadsheet not readable by LibreOffice

[Mike.Cornflake]

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.

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/docxvectorialwriter.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]

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]

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]

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]

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.

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

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?

Yes.

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]

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 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]

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]

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]

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]

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  )

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 .

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)

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]

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.

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]

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]

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)

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

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));


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.

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

Simply make it inline?

[rvk]

Code: [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;


Will 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]

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.

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.