From c26b958ac32c20226db4cb41fb7dda8bc3e9a34f Mon Sep 17 00:00:00 2001 From: Tim Helmstedt Date: Mon, 24 Oct 2016 06:59:16 +1000 Subject: [PATCH 1/5] Benchmark adding images --- build.gradle | 4 +- build.xml | 13 ++++ .../testcases/org/apache/poi/benchmark/45829.png | Bin 0 -> 259 bytes .../org/apache/poi/benchmark/AddImageBench.java | 79 +++++++++++++++++++++ 4 files changed, 95 insertions(+), 1 deletion(-) create mode 100644 src/ooxml/testcases/org/apache/poi/benchmark/45829.png create mode 100644 src/ooxml/testcases/org/apache/poi/benchmark/AddImageBench.java diff --git a/build.gradle b/build.gradle index 4acabe0f1d..526cff0405 100644 --- a/build.gradle +++ b/build.gradle @@ -199,7 +199,7 @@ project('ooxml') { compile 'org.apache.santuario:xmlsec:2.0.6' compile 'org.bouncycastle:bcpkix-jdk15on:1.54' compile 'com.github.virtuald:curvesapi:1.04' - + // for ooxml-lite, should we move this somewhere else? compile 'junit:junit:4.12' @@ -210,6 +210,8 @@ project('ooxml') { testCompile 'junit:junit:4.12' testCompile project(path: ':main', configuration: 'tests') + testCompile 'org.openjdk.jmh:jmh-core:1.15' + testCompile 'org.openjdk.jmh:jmh-generator-annprocess:1.15' } // TOOD: we should not duplicate this task in each project, but I did not figure out how to inject the artifactId for each project diff --git a/build.xml b/build.xml index 0f28471f12..bbe233bed7 100644 --- a/build.xml +++ b/build.xml @@ -163,6 +163,11 @@ under the License. + + + + + @@ -321,6 +326,8 @@ under the License. + + @@ -599,6 +606,8 @@ under the License. + + @@ -624,6 +633,8 @@ under the License. + + @@ -2264,6 +2275,8 @@ under the License. + + diff --git a/src/ooxml/testcases/org/apache/poi/benchmark/45829.png b/src/ooxml/testcases/org/apache/poi/benchmark/45829.png new file mode 100644 index 0000000000000000000000000000000000000000..eccaf30b266f739dd18eceb5e045fa9d47e37d70 GIT binary patch literal 259 zcmeAS@N?(olHy`uVBq!ia0vp^0wB!61SBU+%rFB|oCO|{#XuSi7+lce zSn}G}QBB2uewU{d%N^g?t#>+T^EIgkowJqm&MeGWI@>|EE$El`i$y(ohqmS3m6YI3 z*s<>Q^`dts41!J^ihGtYb`%+$l~_FG6<723d9kv`1B<;n_j+B6&VG4pO3)d>SSg1d z!($P1_BmuO3v{=+5OUgbd%eN22eC`1JlXgD$8x)QRuLZpFN-LHyyEHV=d#Wzp$P!+ CA82C$ literal 0 HcmV?d00001 diff --git a/src/ooxml/testcases/org/apache/poi/benchmark/AddImageBench.java b/src/ooxml/testcases/org/apache/poi/benchmark/AddImageBench.java new file mode 100644 index 0000000000..c910a7fe95 --- /dev/null +++ b/src/ooxml/testcases/org/apache/poi/benchmark/AddImageBench.java @@ -0,0 +1,79 @@ +package org.apache.poi.benchmark;/* ==================================================================== + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +==================================================================== */ + +import org.apache.poi.hssf.HSSFTestDataSamples; +import org.apache.poi.ss.usermodel.*; +import org.apache.poi.util.IOUtils; +import org.apache.poi.xssf.usermodel.XSSFWorkbook; +import org.openjdk.jmh.annotations.*; +import org.openjdk.jmh.profile.GCProfiler; +import org.openjdk.jmh.profile.StackProfiler; +import org.openjdk.jmh.runner.Runner; +import org.openjdk.jmh.runner.RunnerException; +import org.openjdk.jmh.runner.options.Options; +import org.openjdk.jmh.runner.options.OptionsBuilder; + +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.util.concurrent.TimeUnit; + + +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.MICROSECONDS) +@Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS ) +@Measurement(iterations = 10, time = 2, timeUnit = TimeUnit.SECONDS) +@State(Scope.Benchmark) +public class AddImageBench { + + private Workbook wb; + private CreationHelper helper; + private byte[] bytes; + private Sheet sheet; + + @Setup(Level.Iteration) + public void doit() { + wb = new XSSFWorkbook(); + helper = wb.getCreationHelper(); + //add a picture in this workbook. + + bytes = HSSFTestDataSamples.getTestDataFileContent("45829.png"); + sheet = wb.createSheet(); + } + + @Benchmark + public void benchCreatePicture() throws Exception { + Drawing drawing = sheet.createDrawingPatriarch(); + + ClientAnchor anchor = helper.createClientAnchor(); + anchor.setCol1(1); + anchor.setRow1(1); + drawing.createPicture(anchor, wb.addPicture(bytes, Workbook.PICTURE_TYPE_JPEG)); + + } + + public static void main(String[] args) throws RunnerException { + Options opt = new OptionsBuilder() + .include(".*" + AddImageBench.class.getSimpleName() + ".*") + .addProfiler(StackProfiler.class) + .addProfiler(GCProfiler.class) + .forks(1) + .build(); + + new Runner(opt).run(); + } +} From 0fc24637893758abb9f188ea451844bc703f33d1 Mon Sep 17 00:00:00 2001 From: Tim Helmstedt Date: Mon, 24 Oct 2016 17:28:29 +1000 Subject: [PATCH 2/5] Drawing test --- .../apache/poi/xssf/usermodel/TestXSSFDrawing.java | 72 +++++++++++++++++++++- 1 file changed, 69 insertions(+), 3 deletions(-) diff --git a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDrawing.java b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDrawing.java index c5b5d6fa4f..51c0796198 100644 --- a/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDrawing.java +++ b/src/ooxml/testcases/org/apache/poi/xssf/usermodel/TestXSSFDrawing.java @@ -18,10 +18,12 @@ Licensed to the Apache Software Foundation (ASF) under one or more import org.apache.poi.POIXMLDocumentPart; import org.apache.poi.POIXMLDocumentPart.RelationPart; +import org.apache.poi.hssf.HSSFTestDataSamples; import org.apache.poi.openxml4j.opc.OPCPackage; import org.apache.poi.ss.usermodel.ClientAnchor; import org.apache.poi.ss.usermodel.FontUnderline; import org.apache.poi.ss.usermodel.ShapeTypes; +import org.apache.poi.ss.usermodel.Workbook; import org.apache.poi.util.Units; import org.apache.poi.xssf.XSSFTestDataSamples; import org.junit.Test; @@ -31,7 +33,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more import org.openxmlformats.schemas.drawingml.x2006.main.STTextUnderlineType; import org.openxmlformats.schemas.drawingml.x2006.spreadsheetDrawing.CTDrawing; -import java.awt.Color; +import java.awt.*; import java.io.IOException; import java.util.List; @@ -755,7 +757,71 @@ public void testXSSFSimpleShapeCausesNPE56514() throws IOException { shapes = drawing.getShapes(); assertEquals(4, shapes.size()); wb2.close(); + } + + @Test + public void testXSSFSAddPicture() throws Exception { + XSSFWorkbook wb1 = new XSSFWorkbook(); + XSSFSheet sheet = wb1.createSheet(); + //multiple calls of createDrawingPatriarch should return the same instance of XSSFDrawing + XSSFDrawing dr1 = sheet.createDrawingPatriarch(); + XSSFDrawing dr2 = sheet.createDrawingPatriarch(); + assertSame(dr1, dr2); + + List rels = sheet.getRelationParts(); + assertEquals(1, rels.size()); + RelationPart rp = rels.get(0); + assertTrue(rp.getDocumentPart() instanceof XSSFDrawing); + + assertEquals(0, rp.getDocumentPart().getRelations().size()); + + XSSFDrawing drawing = rp.getDocumentPart(); + String drawingId = rp.getRelationship().getId(); + + //there should be a relation to this drawing in the worksheet + assertTrue(sheet.getCTWorksheet().isSetDrawing()); + assertEquals(drawingId, sheet.getCTWorksheet().getDrawing().getId()); + + byte[] pictureData = HSSFTestDataSamples.getTestDataFileContent("45829.png"); + + ClientAnchor anchor = wb1.getCreationHelper().createClientAnchor(); + anchor.setCol1(1); + anchor.setRow1(1); + + drawing.createPicture(anchor, wb1.addPicture(pictureData, Workbook.PICTURE_TYPE_JPEG)); + final int pictureIndex = wb1.addPicture(pictureData, Workbook.PICTURE_TYPE_JPEG); + drawing.createPicture(anchor, pictureIndex); + drawing.createPicture(anchor, pictureIndex); + + // repeated additions of same share package relationship + assertEquals(2, rp.getDocumentPart().getPackagePart().getRelationships().size()); + List shapes = drawing.getShapes(); + assertEquals(3, shapes.size()); + assertTrue(shapes.get(0) instanceof XSSFPicture); + assertTrue(shapes.get(1) instanceof XSSFPicture); + + // Save and re-load it + XSSFWorkbook wb2 = XSSFTestDataSamples.writeOutAndReadBack(wb1); + wb1.close(); + sheet = wb2.getSheetAt(0); + + // Check + dr1 = sheet.createDrawingPatriarch(); + CTDrawing ctDrawing = dr1.getCTDrawing(); + + // Connector, shapes and text boxes are all two cell anchors + assertEquals(0, ctDrawing.sizeOfAbsoluteAnchorArray()); + assertEquals(0, ctDrawing.sizeOfOneCellAnchorArray()); + assertEquals(3, ctDrawing.sizeOfTwoCellAnchorArray()); + + shapes = dr1.getShapes(); + assertEquals(3, shapes.size()); + assertTrue(shapes.get(0) instanceof XSSFPicture); + assertTrue(shapes.get(1) instanceof XSSFPicture); + + checkRewrite(wb2); + wb2.close(); } @Test(expected=IllegalArgumentException.class) @@ -806,7 +872,7 @@ public void testGroupShape() throws Exception { (int)(xfrmG1.getChExt().getCy()*0.8) )); CTGroupTransform2D xfrmG2 = g2.getCTGroupShape().getGrpSpPr().getXfrm(); - + XSSFSimpleShape s2 = g2.createSimpleShape(new XSSFChildAnchor( (int)(xfrmG2.getChExt().getCx()*0.1), (int)(xfrmG2.getChExt().getCy()*0.1), @@ -818,7 +884,7 @@ public void testGroupShape() throws Exception { XSSFWorkbook wb2 = XSSFTestDataSamples.writeOutAndReadBack(wb1); wb1.close(); - + XSSFDrawing draw = wb2.getSheetAt(0).getDrawingPatriarch(); List shapes = draw.getShapes(); assertEquals(2, shapes.size()); From 1d7cf3574016e64e0631556bb50cb466a930c18f Mon Sep 17 00:00:00 2001 From: Tim Helmstedt Date: Sat, 22 Oct 2016 21:06:53 +1000 Subject: [PATCH 3/5] PackageRelationshipCollection caches lookup by targetPart Building partnames for all relationships is expensive. Here we avoid this in findExistingRelation, which is used every time we add a relation to a DocumentPart. --- .../java/org/apache/poi/POIXMLDocumentPart.java | 26 +--------- .../org/apache/poi/openxml4j/opc/PackagePart.java | 59 ++++++++++++---------- .../opc/PackageRelationshipCollection.java | 36 +++++++------ 3 files changed, 51 insertions(+), 70 deletions(-) diff --git a/src/ooxml/java/org/apache/poi/POIXMLDocumentPart.java b/src/ooxml/java/org/apache/poi/POIXMLDocumentPart.java index 163ace54a0..41435ec4a6 100644 --- a/src/ooxml/java/org/apache/poi/POIXMLDocumentPart.java +++ b/src/ooxml/java/org/apache/poi/POIXMLDocumentPart.java @@ -268,7 +268,7 @@ public final String getRelationId(POIXMLDocumentPart part) { * @since 3.14-Beta1 */ public final RelationPart addRelation(String relId, POIXMLRelation relationshipType, POIXMLDocumentPart part) { - PackageRelationship pr = findExistingRelation(part); + PackageRelationship pr = this.packagePart.findExistingRelation(part.getPackagePart()); if (pr == null) { PackagePartName ppn = part.getPackagePart().getPartName(); String relType = relationshipType.getRelation(); @@ -291,30 +291,6 @@ private void addRelation(PackageRelationship pr, POIXMLDocumentPart part) { } /** - * Check if the new part was already added before via PackagePart.addRelationship() - * - * @param part to find the relationship for - * @return The existing relationship, or null if there isn't yet one - */ - private PackageRelationship findExistingRelation(POIXMLDocumentPart part) { - String ppn = part.getPackagePart().getPartName().getName(); - try { - for (PackageRelationship pr : packagePart.getRelationships()) { - if (pr.getTargetMode() == TargetMode.EXTERNAL) { - continue; - } - PackagePart pp = packagePart.getRelatedPart(pr); - if (ppn.equals(pp.getPartName().getName())) { - return pr; - } - } - } catch (InvalidFormatException e) { - throw new POIXMLException("invalid package relationships", e); - } - return null; - } - - /** * Remove the relation to the specified part in this package and remove the * part, if it is no longer needed. * diff --git a/src/ooxml/java/org/apache/poi/openxml4j/opc/PackagePart.java b/src/ooxml/java/org/apache/poi/openxml4j/opc/PackagePart.java index 641e1bc196..d93ebc359a 100644 --- a/src/ooxml/java/org/apache/poi/openxml4j/opc/PackagePart.java +++ b/src/ooxml/java/org/apache/poi/openxml4j/opc/PackagePart.java @@ -22,6 +22,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more import java.io.OutputStream; import java.net.URI; import java.net.URISyntaxException; +import java.util.HashMap; import org.apache.poi.openxml4j.exceptions.InvalidFormatException; import org.apache.poi.openxml4j.exceptions.InvalidOperationException; @@ -63,6 +64,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more */ private PackageRelationshipCollection _relationships; + /** * Constructor. * @@ -126,6 +128,16 @@ public PackagePart(OPCPackage pack, PackagePartName partName, } /** + * Check if the new part was already added before via PackagePart.addRelationship() + * + * @param packagePart to find the relationship for + * @return The existing relationship, or null if there isn't yet one + */ + public PackageRelationship findExistingRelation(PackagePart packagePart) { + return _relationships.findExistingInternalRelation(packagePart); + } + + /** * Adds an external relationship to a part (except relationships part). * * The targets of external relationships are not subject to the same @@ -446,11 +458,7 @@ public boolean hasRelationships() { * @see org.apache.poi.openxml4j.opc.RelationshipSource#isRelationshipExists(org.apache.poi.openxml4j.opc.PackageRelationship) */ public boolean isRelationshipExists(PackageRelationship rel) { - for (PackageRelationship r : _relationships) { - if (r == rel) - return true; - } - return false; + return _relationships.getRelationshipByID(rel.getId()) != null; } /** @@ -464,27 +472,26 @@ public PackagePart getRelatedPart(PackageRelationship rel) throws InvalidFormatE if(! isRelationshipExists(rel)) { throw new IllegalArgumentException("Relationship " + rel + " doesn't start with this part " + _partName); } - - // Get the target URI, excluding any relative fragments - URI target = rel.getTargetURI(); - if(target.getFragment() != null) { - String t = target.toString(); - try { - target = new URI( t.substring(0, t.indexOf('#')) ); - } catch(URISyntaxException e) { - throw new InvalidFormatException("Invalid target URI: " + target); - } - } - - // Turn that into a name, and fetch - PackagePartName relName = PackagingURIHelper.createPartName(target); - PackagePart part = _container.getPart(relName); - if (part == null) { - throw new IllegalArgumentException("No part found for relationship " + rel); - } - return part; - } - + // Get the target URI, excluding any relative fragments + URI target = rel.getTargetURI(); + if(target.getFragment() != null) { + String t = target.toString(); + try { + target = new URI( t.substring(0, t.indexOf('#')) ); + } catch(URISyntaxException e) { + throw new InvalidFormatException("Invalid target URI: " + target); + } + } + + // Turn that into a name, and fetch + PackagePartName relName = PackagingURIHelper.createPartName(target); + PackagePart part = _container.getPart(relName); + if (part == null) { + throw new IllegalArgumentException("No part found for relationship " + rel); + } + return part; + } + /** * Get the input stream of this part to read its content. * diff --git a/src/ooxml/java/org/apache/poi/openxml4j/opc/PackageRelationshipCollection.java b/src/ooxml/java/org/apache/poi/openxml4j/opc/PackageRelationshipCollection.java index 640063802e..912a3d5b8c 100644 --- a/src/ooxml/java/org/apache/poi/openxml4j/opc/PackageRelationshipCollection.java +++ b/src/ooxml/java/org/apache/poi/openxml4j/opc/PackageRelationshipCollection.java @@ -18,10 +18,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more import java.net.URI; import java.net.URISyntaxException; -import java.util.ArrayList; -import java.util.Iterator; -import java.util.Locale; -import java.util.TreeMap; +import java.util.*; import org.apache.poi.openxml4j.exceptions.InvalidFormatException; import org.apache.poi.openxml4j.exceptions.InvalidOperationException; @@ -56,6 +53,12 @@ Licensed to the Apache Software Foundation (ASF) under one or more private TreeMap relationshipsByType; /** + * A lookup of internal relationships to avoid + */ + private HashMap internalRelationshipsByTargetName = new HashMap(); + + + /** * This relationshipPart. */ private PackagePart relationshipPart; @@ -74,7 +77,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more * Reference to the package. */ private OPCPackage container; - + /** * The ID number of the next rID# to generate, or -1 * if that is still to be determined. @@ -230,6 +233,9 @@ public PackageRelationship addRelationship(URI targetUri, sourcePart, targetUri, targetMode, relationshipType, id); relationshipsByID.put(rel.getId(), rel); relationshipsByType.put(rel.getRelationshipType(), rel); + if (targetMode == TargetMode.INTERNAL){ + internalRelationshipsByTargetName.put(targetUri.toASCIIString(), rel); + } return rel; } @@ -245,25 +251,12 @@ public void removeRelationship(String id) { if (rel != null) { relationshipsByID.remove(rel.getId()); relationshipsByType.values().remove(rel); + internalRelationshipsByTargetName.values().remove(rel); } } } /** - * Remove a relationship by its reference. - * - * @param rel - * The relationship to delete. - */ - public void removeRelationship(PackageRelationship rel) { - if (rel == null) - throw new IllegalArgumentException("rel"); - - relationshipsByID.values().remove(rel); - relationshipsByType.values().remove(rel); - } - - /** * Retrieves a relationship by its index in the collection. * * @param index @@ -413,6 +406,11 @@ public PackageRelationshipCollection getRelationships(String typeFilter) { public void clear() { relationshipsByID.clear(); relationshipsByType.clear(); + internalRelationshipsByTargetName.clear(); + } + + public PackageRelationship findExistingInternalRelation(PackagePart packagePart) { + return internalRelationshipsByTargetName.get(packagePart.getPartName().getName()); } @Override From d4f01a949f0f4dc9b34bdb32ef54e7a3e6f37f37 Mon Sep 17 00:00:00 2001 From: Tim Helmstedt Date: Mon, 15 May 2017 14:57:16 +1000 Subject: [PATCH 4/5] PackagePartCollection optimisations Instead of extending a lookup TreeMap and incurring the natural ordering cost for each insertion, we wrap a HashMap and ensure calls to .values() are sorted. --- .../org/apache/poi/openxml4j/opc/OPCPackage.java | 10 ++-- .../poi/openxml4j/opc/PackagePartCollection.java | 58 ++++++++++++++-------- .../org/apache/poi/openxml4j/opc/ZipPackage.java | 6 +-- 3 files changed, 44 insertions(+), 30 deletions(-) diff --git a/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java b/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java index 6b6c5e85cb..1389fb7426 100644 --- a/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java +++ b/src/ooxml/java/org/apache/poi/openxml4j/opc/OPCPackage.java @@ -649,12 +649,11 @@ public PackagePart getPart(PackagePartName partName) { */ public ArrayList getPartsByContentType(String contentType) { ArrayList retArr = new ArrayList(); - for (PackagePart part : partList.values()) { + for (PackagePart part : partList.sortedValues()) { if (part.getContentType().equals(contentType)) { retArr.add(part); } } - Collections.sort(retArr); return retArr; } @@ -697,13 +696,12 @@ public PackagePart getPart(PackagePartName partName) { } Matcher matcher = namePattern.matcher(""); ArrayList result = new ArrayList(); - for (PackagePart part : partList.values()) { + for (PackagePart part : partList.sortedValues()) { PackagePartName partName = part.getPartName(); if (matcher.reset(partName.getName()).matches()) { result.add(part); } } - Collections.sort(result); return result; } @@ -813,9 +811,7 @@ public PackagePart getPart(PackageRelationship partRel) { } } } - ArrayList result = new ArrayList(partList.values()); - java.util.Collections.sort(result); - return result; + return new ArrayList(partList.sortedValues()); } /** diff --git a/src/ooxml/java/org/apache/poi/openxml4j/opc/PackagePartCollection.java b/src/ooxml/java/org/apache/poi/openxml4j/opc/PackagePartCollection.java index e4ecbd44b8..71cb6e7fd6 100644 --- a/src/ooxml/java/org/apache/poi/openxml4j/opc/PackagePartCollection.java +++ b/src/ooxml/java/org/apache/poi/openxml4j/opc/PackagePartCollection.java @@ -17,8 +17,8 @@ Licensed to the Apache Software Foundation (ASF) under one or more package org.apache.poi.openxml4j.opc; -import java.util.ArrayList; -import java.util.TreeMap; +import java.io.Serializable; +import java.util.*; import org.apache.poi.openxml4j.exceptions.InvalidOperationException; @@ -28,21 +28,19 @@ Licensed to the Apache Software Foundation (ASF) under one or more * @author Julien Chable * @version 0.1 */ -public final class PackagePartCollection extends - TreeMap { +public final class PackagePartCollection implements Serializable { - private static final long serialVersionUID = 2515031135957635515L; + private static final long serialVersionUID = 2515031135957635517L; /** - * Arraylist use to store this collection part names as string for rule + * HashSet use to store this collection part names as string for rule * M1.11 optimized checking. */ - private ArrayList registerPartNameStr = new ArrayList(); + private HashSet registerPartNameStr = new HashSet(); + + + private final HashMap packagePartLookup = new HashMap(); - @Override - public Object clone() { - return super.clone(); - } /** * Check rule [M1.11]: a package implementer shall neither create nor @@ -53,11 +51,10 @@ public Object clone() { * Throws if you try to add a part with a name derived from * another part name. */ - @Override public PackagePart put(PackagePartName partName, PackagePart part) { String[] segments = partName.getURI().toASCIIString().split( PackagingURIHelper.FORWARD_SLASH_STRING); - StringBuffer concatSeg = new StringBuffer(); + StringBuilder concatSeg = new StringBuilder(); for (String seg : segments) { if (!seg.equals("")) concatSeg.append(PackagingURIHelper.FORWARD_SLASH_CHAR); @@ -68,14 +65,35 @@ public PackagePart put(PackagePartName partName, PackagePart part) { } } this.registerPartNameStr.add(partName.getName()); - return super.put(partName, part); + return packagePartLookup.put(partName, part); } - @Override - public PackagePart remove(Object key) { - if (key instanceof PackagePartName) { - this.registerPartNameStr.remove(((PackagePartName) key).getName()); - } - return super.remove(key); + public PackagePart remove(PackagePartName key) { + this.registerPartNameStr.remove(key.getName()); + return packagePartLookup.remove(key); + } + + + /** + * The values themselves should be returned in sorted order. Doing it here + * avoids paying the high cost of Natural Ordering per insertion. + */ + public Collection sortedValues() { + ArrayList packageParts = new ArrayList(packagePartLookup.values()); + Collections.sort(packageParts); + return packageParts; + + } + + public boolean containsKey(PackagePartName partName) { + return packagePartLookup.containsKey(partName); + } + + public PackagePart get(PackagePartName partName) { + return packagePartLookup.get(partName); + } + + public int size() { + return packagePartLookup.size(); } } diff --git a/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java b/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java index 5b9e2141e4..922d8af92f 100644 --- a/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java +++ b/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java @@ -239,8 +239,8 @@ private static ZipEntrySource openZipEntrySourceStream(ThresholdInputStream zis) } if (this.zipArchive == null) { - return this.partList.values().toArray( - new PackagePart[this.partList.values().size()]); + return this.partList.sortedValues().toArray( + new PackagePart[this.partList.sortedValues().size()]); } // First we need to parse the content type part @@ -344,7 +344,7 @@ else if (contentType != null) { } } - return partList.values().toArray(new ZipPackagePart[partList.size()]); + return partList.sortedValues().toArray(new PackagePart[partList.size()]); } /** From 12f836db9cb0b21ecc106e012532965e0df0b43c Mon Sep 17 00:00:00 2001 From: Tim Helmstedt Date: Tue, 16 May 2017 08:36:10 +1000 Subject: [PATCH 5/5] Avoid double sorting part lists --- src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java b/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java index 922d8af92f..17046da357 100644 --- a/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java +++ b/src/ooxml/java/org/apache/poi/openxml4j/opc/ZipPackage.java @@ -240,7 +240,7 @@ private static ZipEntrySource openZipEntrySourceStream(ThresholdInputStream zis) if (this.zipArchive == null) { return this.partList.sortedValues().toArray( - new PackagePart[this.partList.sortedValues().size()]); + new PackagePart[this.partList.size()]); } // First we need to parse the content type part