Re: svn commit: r1801806 - in /poi/trunk/src/ooxml: java/org/apache/poi/xssf/streaming/SXSSFWorkbook.java java/org/apache/poi/xssf/streaming/SheetDataWriter.java testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1801806 - in /poi/trunk/src/ooxml: java/org/apache/poi/xssf/streaming/SXSSFWorkbook.java java/org/apache/poi/xssf/streaming/SheetDataWriter.java testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java

Javen O'Neal-2
I like the idea of using XML-specific startElement, endElement, and
writeAttribute functions so that the code doesn't get littered with angle
brackets, quotes, and escaping attributes that contain quotes, ampersands,
and other characters.

I think Java will convert "a"+"b"+"c" into
StringBuilder("a").append("b").append("c").toString() during the
compilation process, so we probably don't need to make those replacements
unless they improve readability.

On Jul 13, 2017 12:14 AM, <[hidden email]> wrote:

> Author: fanningpj
> Date: Thu Jul 13 07:14:01 2017
> New Revision: 1801806
>
> URL: http://svn.apache.org/viewvc?rev=1801806&view=rev
> Log:
> avoid unnecessary string concats in SXSSF SheetDataWriter
>
> Modified:
>     poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/
> SXSSFWorkbook.java
>     poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/
> SheetDataWriter.java
>     poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/streaming/
> TestSXSSFWorkbook.java
>
> Modified: poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/
> SXSSFWorkbook.java
> URL: http://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/
> apache/poi/xssf/streaming/SXSSFWorkbook.java?rev=
> 1801806&r1=1801805&r2=1801806&view=diff
> ============================================================
> ==================
> --- poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFWorkbook.java
> (original)
> +++ poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/SXSSFWorkbook.java
> Thu Jul 13 07:14:01 2017
> @@ -406,7 +406,7 @@ public class SXSSFWorkbook implements Wo
>      }
>
>      private static void copyStreamAndInjectWorksheet(InputStream in,
> OutputStream out, InputStream worksheetData) throws IOException {
> -        InputStreamReader inReader=new InputStreamReader(in,"UTF-8");
> //TODO: Is it always UTF-8 or do we need to read the xml encoding
> declaration in the file? If not, we should perhaps use a SAX reader instead.
> +        InputStreamReader inReader=new InputStreamReader(in,"UTF-8");
>          OutputStreamWriter outWriter=new OutputStreamWriter(out,"UTF-8");
>          boolean needsStartTag = true;
>          int c;
>
> Modified: poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/
> SheetDataWriter.java
> URL: http://svn.apache.org/viewvc/poi/trunk/src/ooxml/java/org/
> apache/poi/xssf/streaming/SheetDataWriter.java?rev=
> 1801806&r1=1801805&r2=1801806&view=diff
> ============================================================
> ==================
> --- poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/SheetDataWriter.java
> (original)
> +++ poi/trunk/src/ooxml/java/org/apache/poi/xssf/streaming/SheetDataWriter.java
> Thu Jul 13 07:14:01 2017
> @@ -207,23 +207,27 @@ public class SheetDataWriter implements
>      }
>
>      void beginRow(int rownum, SXSSFRow row) throws IOException {
> -        _out.write("<row r=\"" + (rownum + 1) + "\"");
> -        if (row.hasCustomHeight())
> -            _out.write(" customHeight=\"true\"  ht=\"" +
> row.getHeightInPoints() + "\"");
> -        if (row.getZeroHeight())
> -            _out.write(" hidden=\"true\"");
> +        _out.write("<row");
> +        writeAttribute("r", Integer.toString(rownum + 1));
> +        if (row.hasCustomHeight()) {
> +            writeAttribute("customHeight", "true");
> +            writeAttribute("ht", Float.toString(row.
> getHeightInPoints()));
> +        }
> +        if (row.getZeroHeight()) {
> +            writeAttribute("hidden", "true");
> +        }
>          if (row.isFormatted()) {
> -            _out.write(" s=\"" + row.getRowStyleIndex() + "\"");
> -            _out.write(" customFormat=\"1\"");
> +            writeAttribute("s", Integer.toString(row.
> getRowStyleIndex()));
> +            writeAttribute("customFormat", "1");
>          }
>          if (row.getOutlineLevel() != 0) {
> -            _out.write(" outlineLevel=\"" + row.getOutlineLevel() + "\"");
> +            writeAttribute("outlineLevel", Integer.toString(row.
> getOutlineLevel()));
>          }
>          if(row.getHidden() != null) {
> -            _out.write(" hidden=\"" + (row.getHidden() ? "1" : "0") +
> "\"");
> +            writeAttribute("hidden", row.getHidden() ? "1" : "0");
>          }
>          if(row.getCollapsed() != null) {
> -            _out.write(" collapsed=\"" + (row.getCollapsed() ? "1" : "0")
> + "\"");
> +            writeAttribute("collapsed", row.getCollapsed() ? "1" : "0");
>          }
>
>          _out.write(">\n");
> @@ -239,30 +243,32 @@ public class SheetDataWriter implements
>              return;
>          }
>          String ref = new CellReference(_rownum,
> columnIndex).formatAsString();
> -        _out.write("<c r=\"" + ref + "\"");
> +        _out.write("<c");
> +        writeAttribute("r", ref);
>          CellStyle cellStyle = cell.getCellStyle();
>          if (cellStyle.getIndex() != 0) {
>              // need to convert the short to unsigned short as the indexes
> can be up to 64k
>              // ideally we would use int for this index, but that would
> need changes to some more
>              // APIs
> -            _out.write(" s=\"" + (cellStyle.getIndex() & 0xffff) + "\"");
> +            writeAttribute("s", Integer.toString(cellStyle.getIndex() &
> 0xffff));
>          }
>          CellType cellType = cell.getCellTypeEnum();
>          switch (cellType) {
>              case BLANK: {
> -                _out.write(">");
> +                _out.write('>');
>                  break;
>              }
>              case FORMULA: {
> -                _out.write(">");
> -                _out.write("<f>");
> +                _out.write("><f>");
>                  outputQuotedString(cell.getCellFormula());
>                  _out.write("</f>");
>                  switch (cell.getCachedFormulaResultTypeEnum()) {
>                      case NUMERIC:
>                          double nval = cell.getNumericCellValue();
>                          if (!Double.isNaN(nval)) {
> -                            _out.write("<v>" + nval + "</v>");
> +                            _out.write("<v>");
> +                            _out.write(Double.toString(nval));
> +                            _out.write("</v>");
>                          }
>                          break;
>                      default:
> @@ -275,15 +281,15 @@ public class SheetDataWriter implements
>                      XSSFRichTextString rt = new XSSFRichTextString(cell.
> getStringCellValue());
>                      int sRef = _sharedStringSource.addEntry(
> rt.getCTRst());
>
> -                    _out.write(" t=\"" + STCellType.S + "\">");
> -                    _out.write("<v>");
> +                    writeAttribute("t", STCellType.S.toString());
> +                    _out.write("><v>");
>                      _out.write(String.valueOf(sRef));
>                      _out.write("</v>");
>                  } else {
> -                    _out.write(" t=\"inlineStr\">");
> -                    _out.write("<is><t");
> +                    writeAttribute("t", "inlineStr");
> +                    _out.write("><is><t");
>                      if (hasLeadingTrailingSpaces(cell.getStringCellValue()))
> {
> -                        _out.write(" xml:space=\"preserve\"");
> +                        writeAttribute("xml:space", "preserve");
>                      }
>                      _out.write(">");
>                      outputQuotedString(cell.getStringCellValue());
> @@ -292,20 +298,26 @@ public class SheetDataWriter implements
>                  break;
>              }
>              case NUMERIC: {
> -                _out.write(" t=\"n\">");
> -                _out.write("<v>" + cell.getNumericCellValue() + "</v>");
> +                writeAttribute("t", "n");
> +                _out.write("><v>");
> +                _out.write(Double.toString(cell.getNumericCellValue()));
> +                _out.write("</v>");
>                  break;
>              }
>              case BOOLEAN: {
> -                _out.write(" t=\"b\">");
> -                _out.write("<v>" + (cell.getBooleanCellValue() ? "1" :
> "0") + "</v>");
> +                writeAttribute("t", "b");
> +                _out.write("><v>");
> +                _out.write(cell.getBooleanCellValue() ? "1" : "0");
> +                _out.write("</v>");
>                  break;
>              }
>              case ERROR: {
>                  FormulaError error = FormulaError.forInt(cell.
> getErrorCellValue());
>
> -                _out.write(" t=\"e\">");
> -                _out.write("<v>" + error.getString() + "</v>");
> +                writeAttribute("t", "e");
> +                _out.write("><v>");
> +                _out.write(error.getString());
> +                _out.write("</v>");
>                  break;
>              }
>              default: {
> @@ -315,6 +327,13 @@ public class SheetDataWriter implements
>          _out.write("</c>");
>      }
>
> +    private void writeAttribute(String name, String value) throws
> IOException {
> +        _out.write(' ');
> +        _out.write(name);
> +        _out.write("=\"");
> +        _out.write(value);
> +        _out.write('\"');
> +    }
>
>      /**
>       * @return  whether the string has leading / trailing spaces that
>
> Modified: poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/streaming/
> TestSXSSFWorkbook.java
> URL: http://svn.apache.org/viewvc/poi/trunk/src/ooxml/testcases/
> org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java?rev=
> 1801806&r1=1801805&r2=1801806&view=diff
> ============================================================
> ==================
> --- poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java
> (original)
> +++ poi/trunk/src/ooxml/testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java
> Thu Jul 13 07:14:01 2017
> @@ -132,7 +132,7 @@ public final class TestSXSSFWorkbook ext
>      public void useSharedStringsTable() throws Exception {
>          SXSSFWorkbook wb = new SXSSFWorkbook(null, 10, false, true);
>
> -        SharedStringsTable sss =  POITestCase.getFieldValue(SXSSFWorkbook.class,
> wb, SharedStringsTable.class, "_sharedStringSource");
> +        SharedStringsTable sss = POITestCase.getFieldValue(SXSSFWorkbook.class,
> wb, SharedStringsTable.class, "_sharedStringSource");
>
>          assertNotNull(sss);
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: svn commit: r1801806 - in /poi/trunk/src/ooxml: java/org/apache/poi/xssf/streaming/SXSSFWorkbook.java java/org/apache/poi/xssf/streaming/SheetDataWriter.java testcases/org/apache/poi/xssf/streaming/TestSXSSFWorkbook.java

pj.fanning
I thought that the writeAttribute made the code a bit easier to read.
Also, concatenating strings before writing them to the Writer does add overhead.
I did some similar work (with others) on a similar writer implementation in February and removing the string concats and using additional write calls instead did lead to better performance.

These changes were in:
https://github.com/prometheus/client_java/commits/master/simpleclient_common/src/main/java/io/prometheus/client/exporter/common/TextFormat.java

The performance test: https://github.com/pkubowicz/prometheus-client-test

I'm not wedded to the r1801806 change, so if there are objections, I can revert it.
Loading...