XWIKI-19223: Improve xobject memory storage in XWikidocument

Author: tmortagne

xwiki-platform-core/xwiki-platform-oldcore/pom.xml

@@ -607,6 +607,10 @@
  <version>1.6</version>
  <scope>test</scope>
  </dependency>
+ <dependency>
+ <groupId>com.google.guava</groupId>
+ <artifactId>guava-testlib</artifactId>
+ </dependency>
 
  <dependency>
  <groupId>org.xwiki.commons</groupId>

xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/doc/XWikiDocument.java

@@ -49,6 +49,7 @@
 importjava.util.SortedMap;
 importjava.util.TreeMap;
 importjava.util.Vector;
+importjava.util.concurrent.ConcurrentSkipListMap;
 importjava.util.regex.Matcher;
 importjava.util.regex.Pattern;
 importjava.util.zip.ZipEntry;
@@ -167,6 +168,7 @@
 importcom.xpn.xwiki.doc.merge.MergeResult;
 importcom.xpn.xwiki.doc.rcs.XWikiRCSNodeInfo;
 importcom.xpn.xwiki.internal.cache.rendering.RenderingCache;
+importcom.xpn.xwiki.internal.doc.BaseObjects;
 importcom.xpn.xwiki.internal.doc.XWikiAttachmentList;
 importcom.xpn.xwiki.internal.filter.XWikiDocumentFilterUtils;
 importcom.xpn.xwiki.internal.render.OldRendering;
@@ -504,10 +506,10 @@ private static LinkParser getLinkParser(){
 
 /**
  * Map holding document objects indexed by XClass references (i.e. Document References since a XClass reference
- * points to a document). The map is not synchronized, and uses a TreeMap implementation to preserve index ordering
- * (consistent sorted order for output to XML, rendering in velocity, etc.)
+ * points to a document). The preserve index ordering (consistent sorted order for output to XML, rendering in
+ * velocity, etc.)
  */
-privateMap<DocumentReference, List<BaseObject>>xObjects = newTreeMap<DocumentReference, List<BaseObject>>();
+privateMap<DocumentReference, BaseObjects>xObjects = newConcurrentSkipListMap<>();
 
 privatefinalXWikiAttachmentListattachmentList = newXWikiAttachmentList(XWikiDocument.this);
 
@@ -2544,7 +2546,7 @@ public void setXClass(BaseClass xwikiClass)
  */
 publicMap<DocumentReference, List<BaseObject>> getXObjects()
 {
-returnthis.xObjects;
+return(Map) this.xObjects;
  }
 
 /**
@@ -2572,7 +2574,9 @@ public void setXObjects(Map<DocumentReference, List<BaseObject>> objects)
  }
 
 // Replace the current objects with the provided ones.
-this.xObjects = objects;
+Map<DocumentReference, BaseObjects> objectsCopy = newConcurrentSkipListMap<>();
+objects.forEach((k, v) -> objectsCopy.put(k, newBaseObjects(v)));
+this.xObjects = objectsCopy;
  }
 
 /**
@@ -2636,9 +2640,9 @@ public int createXObject(EntityReference classReference, XWikiContext context) t
 BaseObjectobject = BaseClass.newCustomClassInstance(absoluteClassReference, context);
 object.setOwnerDocument(this);
 object.setXClassReference(classReference);
-List<BaseObject>objects = this.xObjects.get(absoluteClassReference);
+BaseObjectsobjects = this.xObjects.get(absoluteClassReference);
 if (objects == null){
-objects = newArrayList<BaseObject>();
+objects = newBaseObjects();
 this.xObjects.put(absoluteClassReference, objects);
  }
 objects.add(object);
@@ -2758,16 +2762,7 @@ public void setXObjects(DocumentReference classReference, List<BaseObject> objec
  }
 
 // Add new objects
-if (objects.isEmpty()){
-// Pretty wrong but can't remove that for retro compatibility reasons...
-// Note that it means that someone can put an unmodifiable list here make impossible to add any object of
-// this class.
-this.xObjects.put(classReference, objects);
- } else{
-for (BaseObjectbaseObject : objects){
-addXObject(classReference, baseObject);
- }
- }
+this.xObjects.put(classReference, newBaseObjects(objects));
 
 setMetaDataDirty(true);
  }
@@ -3062,9 +3057,9 @@ public void setXObject(DocumentReference classReference, int nb, BaseObject obje
 object.setNumber(nb);
  }
 
-List<BaseObject>objects = this.xObjects.get(classReference);
+BaseObjectsobjects = this.xObjects.get(classReference);
 if (objects == null){
-objects = newArrayList<BaseObject>();
+objects = newBaseObjects();
 this.xObjects.put(classReference, objects);
  }
 while (nb >= objects.size()){
@@ -3088,9 +3083,9 @@ public void setXObject(int nb, BaseObject object)
 object.setOwnerDocument(this);
 object.setNumber(nb);
 
-List<BaseObject>objects = this.xObjects.get(object.getXClassReference());
+BaseObjectsobjects = this.xObjects.get(object.getXClassReference());
 if (objects == null){
-objects = newArrayList<BaseObject>();
+objects = newBaseObjects();
 this.xObjects.put(object.getXClassReference(), objects);
  }
 while (nb >= objects.size()){

xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/internal/doc/BaseObjects.java

@@ -0,0 +1,152 @@
+/*
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+packagecom.xpn.xwiki.internal.doc;
+
+importjava.util.AbstractList;
+importjava.util.Collection;
+importjava.util.Map;
+importjava.util.concurrent.ConcurrentSkipListMap;
+
+importcom.xpn.xwiki.objects.BaseObject;
+
+/**
+ * Implement a list of{@link BaseObject} as seen by a document (meaning that the list index matched the object number)
+ * using a{@link Map} storage to avoid wasting memory with null entries.
+ * 
+ * @version $Id$
+ * @since 14.0RC1
+ */
+// TODO: expose APIs to navigate non null objects directly instead of going through each entry and skipping explicitly
+// null ones
+publicclassBaseObjectsextendsAbstractList<BaseObject>
+{
+// Sort keys so that it's possible to navigate non null entries without going through them all
+privateMap<Integer, BaseObject> map = newConcurrentSkipListMap<>();
+
+privateintsize;
+
+/**
+ * Constructs an empty list.
+ */
+publicBaseObjects()
+{
+
+ }
+
+/**
+ * Constructs a list containing the elements of the specified collection, in the order they are returned by the
+ * collection's iterator.
+ * 
+ * @param collection the collection to copy
+ */
+publicBaseObjects(Collection<BaseObject> collection)
+{
+collection.forEach(this::add);
+ }
+
+@Override
+publicBaseObjectget(intindex)
+{
+rangeCheck(index);
+
+returnthis.map.get(index);
+ }
+
+@Override
+publicintsize()
+{
+returnthis.size;
+ }
+
+privateBaseObjectput(intindex, BaseObjectelement)
+{
+BaseObjectold;
+if (element == null){
+// We don't want to keep null values in memory
+old = this.map.remove(index);
+ } else{
+// Make sure the object number is right
+element.setNumber(index);
+
+old = this.map.put(index, element);
+ }
+
+// Increment size if needed
+if (this.size <= index){
+this.size = index + 1;
+ }
+
+returnold;
+ }
+
+@Override
+publicvoidadd(intindex, BaseObjectelement)
+{
+// Check if the index is valid
+rangeCheckForAdd(index);
+
+// Move right values
+if (index < this.size){
+for (inti = this.size - 1; i >= index; --i){
+put(i + 1, get(i));
+ }
+ }
+
+// Insert new value
+put(index, element);
+ }
+
+@Override
+publicBaseObjectset(intindex, BaseObjectelement)
+{
+// Check if the index is valid
+rangeCheck(index);
+
+// Set the value and remember the old one
+returnput(index, element);
+ }
+
+@Override
+publicBaseObjectremove(intindex)
+{
+rangeCheck(index);
+
+returnthis.map.remove(index);
+ }
+
+privatevoidrangeCheck(intindex)
+{
+if (index < 0 || index >= this.size){
+thrownewIndexOutOfBoundsException(outOfBoundsMsg(index));
+ }
+ }
+
+privatevoidrangeCheckForAdd(intindex)
+{
+if (index < 0 || index > this.size || index == Integer.MAX_VALUE){
+thrownewIndexOutOfBoundsException(outOfBoundsMsg(index));
+ }
+ }
+
+privateStringoutOfBoundsMsg(intindex)
+{
+return"Index: " + index + ", Size: " + size;
+ }
+}

xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/internal/doc/BaseObjectsTest.java

@@ -0,0 +1,131 @@
+/*
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+packagecom.xpn.xwiki.internal.doc;
+
+importjava.util.Arrays;
+
+importorg.junit.jupiter.api.Test;
+
+importcom.xpn.xwiki.objects.BaseObject;
+
+importstaticorg.junit.jupiter.api.Assertions.assertEquals;
+importstaticorg.junit.jupiter.api.Assertions.assertNull;
+importstaticorg.junit.jupiter.api.Assertions.assertSame;
+importstaticorg.junit.jupiter.api.Assertions.assertThrows;
+
+/**
+ * Validate{@link BaseObjects}.
+ * 
+ * @version $Id$
+ */
+publicclassBaseObjectsTest
+{
+privatestaticclassTestBaseObjectextendsBaseObject
+{
+@Override
+publicStringtoString()
+{
+returnString.valueOf(getNumber());
+ }
+ }
+
+privatestaticfinalBaseObjectXOBJ1 = newTestBaseObject();
+
+privatestaticfinalBaseObjectXOBJ2 = newTestBaseObject();
+
+privatestaticfinalBaseObjectXOBJ3 = newTestBaseObject();
+
+privatestaticfinalBaseObjectXOBJ4 = newTestBaseObject();
+
+@Test
+voidadd()
+{
+BaseObjectsobjects = newBaseObjects();
+
+assertEquals(0, objects.size());
+
+objects.add(XOBJ1);
+
+assertEquals(1, objects.size());
+
+objects.add(null);
+
+assertEquals(2, objects.size());
+
+objects.add(XOBJ2);
+
+assertEquals(3, objects.size());
+assertSame(XOBJ1, objects.get(0));
+assertEquals(0, objects.get(0).getNumber());
+assertNull(objects.get(1));
+assertSame(XOBJ2, objects.get(2));
+assertEquals(2, objects.get(2).getNumber());
+
+objects.add(1, XOBJ3);
+
+assertEquals(4, objects.size());
+assertSame(XOBJ1, objects.get(0));
+assertEquals(0, objects.get(0).getNumber());
+assertSame(XOBJ3, objects.get(1));
+assertEquals(1, objects.get(1).getNumber());
+assertNull(objects.get(2));
+assertSame(XOBJ2, objects.get(3));
+assertEquals(3, objects.get(3).getNumber());
+
+assertThrows(IndexOutOfBoundsException.class, () -> objects.add(Integer.MAX_VALUE, XOBJ4));
+ }
+
+@Test
+voidset()
+{
+BaseObjectsobjects = newBaseObjects();
+
+assertEquals(0, objects.size());
+
+assertThrows(IndexOutOfBoundsException.class, () -> objects.set(0, XOBJ1));
+
+objects.add(XOBJ1);
+objects.add(XOBJ2);
+objects.add(XOBJ3);
+
+objects.set(1, XOBJ4);
+
+assertSame(XOBJ1, objects.get(0));
+assertSame(XOBJ4, objects.get(1));
+assertSame(XOBJ3, objects.get(2));
+ }
+
+@Test
+voidremove()
+{
+BaseObjectsobjects = newBaseObjects(Arrays.asList(XOBJ1, XOBJ2, XOBJ3));
+
+assertEquals(3, objects.size());
+
+objects.remove(0);
+objects.remove(2);
+
+assertNull(objects.get(0));
+assertSame(XOBJ2, objects.get(1));
+assertNull(objects.get(2));
+
+assertEquals(3, objects.size());
+ }
+}