Customize Spliterator implementations for better parallelism

[daniel-shuy]

Why

There are many classes in Apache POI that implements Iterable, but only implements Iterable#iterator() without overriding Iterable#spliterator().

The default implementation of Iterable#spliterator() is:

default Spliterator<T> spliterator() {
    return Spliterators.spliteratorUnknownSize(iterator(), 0);
}

Spliterator#spliteratorUnknownSize(Iterator, int) returns a Spliterator with no initial size estimate, which has poor splitting capabilities. The default implementation has to do this because not all Iterables have a fixed size.

This is important when trying to convert the Iterable to a parallel Stream. For example, when trying to process Rows in a XSSFSheet (XSSFSheet implements Iterable<Row>) in parallel, the performance is terrible:

StreamSupport.stream(sheet.spliterator(), true)
    .forEach(row -> {
        // ...
    });

The XSSFSheet's Iterator is backed by a TreeMap#values():

poi/poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFSheet.java

Line 95 in cf1354d

private final SortedMap<Integer, XSSFRow> _rows = new TreeMap<>();

poi/poi-ooxml/src/main/java/org/apache/poi/xssf/usermodel/XSSFSheet.java

Lines 2049 to 2067 in cf1354d

	/**
	* @return an iterator of the PHYSICAL rows. Meaning the 3rd element may not
	* be the third row if say for instance the second row is undefined.
	* Call getRowNum() on each row if you care which one it is.
	*/
	@Override
	@SuppressWarnings("unchecked")
	public Iterator<Row> rowIterator() {
	return (Iterator<Row>)(Iterator<? extends Row>) _rows.values().iterator();
	}
	/**
	* Alias for {@link #rowIterator()} to
	* allow foreach loops
	*/
	@Override
	public Iterator<Row> iterator() {
	return rowIterator();
	}

TreeMap#values() returns a Collection, and thus has a fixed size. Its Spliterator is hence sized and has good splitting capabilities, allowing it to be properly parallelized. Therefore an easy way to customize XSSFSheet#spliterator() is to simply delegate to the underlying TreeMap#values(), eg.

@Override
@SuppressWarnings("unchecked")
public Spliterator<Row> spliterator() {
    return (Spliterator<Row>)(Spliterator<? extends Row>) _rows.values().spliterator();
}

How

• For classes that expose an Iterator factory method with a backing Collection/Iterable, I simply delegated the Spliterator factory method to the underlying Collection/Iterable.
• For classes that expose an Iterator factory method but do not have a backing Collection/Iterable, but have a fixed size, I customized the Spliterator factory method to return a sized Spliterator using Spliterators#spliterator(Iterator, long, int).
• Classes that expose an Iterator factory method but do not have a fixed size remain unchanged.

I also took the liberty to perform the following refactors:

• Move iterator() implementation in child classes to Workbook/Sheet/Row interface using default methods (the iterator() method is an alias of sheetIterator()/rowIterator()/cellIterator(), and is currently being duplicated in all child classes)
• Add Iterable interface to IntMapper and XDDFTextParagraph (so that they can be iterated using an enhanced for loop)

None of the changes break source/binary compatibility.

---

For anyone interested, the current workaround is create a sized Spliterator using Sheet#getPhysicalNumberOfRows(), eg.

Spliterator<Row> spliterator = Spliterators.spliterator(sheet.iterator(), sheet.getPhysicalNumberOfRows(), 0);
StreamSupport.stream(spliterator, true)
    .forEach(row -> {
        // ...
    });

This however, is still less optimal than using the backing Collection's Spliterator.

[pjfanning]

could you add @since POI 5.2.0 on any new public methods?

[daniel-shuy]

Sure

[daniel-shuy]

Done

[pjfanning]

can you keep the explicit imports and remove the * import?

[daniel-shuy]

Sorry, this was automatically formatted by my IDE, will remove it

[daniel-shuy]

Done

[pjfanning]

maybe, we only need spliterator() - I know we have 2 iterator methods but that is some legacy stuff - I don't think new stuff needs to add 2 separate methods

[daniel-shuy]

ok, will remove it, I agree its better to have 1 method

[daniel-shuy]

Done

[pjfanning]

@daniel-shuy generally looks good - but could you add some test coverage for some of the new methods? We don't necessarily need full coverage but some regression tests would be a pre-req for a merge.

[pjfanning]

Is this better than Spliterators.spliterator(sheets, Spliterator.ORDERED)?

[daniel-shuy]

Its actually the same, because SheetSpliterator is delegating to sheets.spliterator(), which is actually calling Spliterators.spliterator(this, Spliterator.ORDERED).

IMO its still better to delegate to the backing Collection's spliterator(), so that if the backing Collection changes, there is no risk of the Spliterator's characteristics going out of sync.

I added SheetSpliterator to be consistent with sheetIterator(), which creates an instance of SheetIterator to delegate sheets.iterator(). But now that I think about it, its probably better to simply do a cast, reducing an object instantiation, eg.

@Override
@SuppressWarnings("unchecked")
public Spliterator<Sheet> spliterator() {
    return (Spliterator<Sheet>)(Spliterator<? extends Sheet>) sheets.spliterator();
}

[pjfanning]

I don't think you'll need to cast.

[daniel-shuy]

Unfortunately sheets.spliterator() returns Spliterator<XSSFSheet>

[pjfanning]

can you try omitting the cast and see if it compiles? I just added spliterators in another project and was suprised that I didn't need to cast but it worked without the cast - pjfanning/excel-streaming-reader@4ac5fb8

[daniel-shuy]

I tried, it doesn't work. Using Spliterators.spliterator(this, Spliterator.ORDERED) avoids having to cast because Spliterators.spliterator(Collection, int) takes in Collection<? extends T> and returns Spliterator<T>. If only Iterable<T>#spliterator() returned Spliterator<? extends T> instead of Spliterator<T> 🤦‍♂️

[pjfanning]

fair enough - use the cast

[daniel-shuy]

Done