Saxon

The method that builds the prior pointer index is synchronized, so it's not a completely crass oversight.

I suspect it has something to do with the way that a single TinyTree data structure is used to hold a sequence of separate trees (a forest). Although the creation of a prior index is synchronized, the addition of new trees to the forest is not.

There's certainly a theoretical problem here, which may or may not be the cause of this crash. A SequenceOutputter can be reused to write a sequence of trees to the same TinyTree structure; this process is synchronized on the Controller to make it thread-safe. But the process which allocates prior pointers is synchronized on the TinyTree itself, so the two processes are not protected from each other. The latter process accesses the TinyTree field numberOfNodes, which can change value if someone else is appending to the tree while the index is being built, and indeed the AIOOB exception strongly indicates that this is happening.

I'll try and create a test case to trigger this condition so that I can test any fix.

There are several possible approaches to fixing it. One (already suggested by a TODO in the code) is to build the prior index only for one document at a time. Another is to synchronize both operations on the same object. Another is to abandon the idea of re-using the SequenceOutputter and the TinyTree structure for multiple trees (I recall the performance benefits were significant at the time, but this is not necessarily still the case, especially as we are probably now smarter in terms of deciding how much space o allocate).

It may be a case where the "quick fix" solution for a maintenance release is different from the permanent solution.

I have reproduced the problem with the following stylesheet:

<xsl:stylesheet version="2.0" 
  xmlns:xsl="http://www.w3.org/1999/XSL/Transform" 
  xmlns:xs="http://www.w3.org/2001/XMLSchema" 
  xmlns:t="this"> 
  <xsl:output indent="yes"/>
  <xsl:template match="/" name="main">
    <out>
     <xsl:for-each select="1 to 1000" saxon:threads="4" xmlns:saxon="http://saxon.sf.net/" exclude-result-prefixes="saxon">
       <xsl:variable name="temp" as="element()">
         <doc>
           <xsl:for-each select="1 to .+50">
              <ele><xsl:value-of select="."/></ele>
           </xsl:for-each>
         </doc>
       </xsl:variable>
       <in top="{name($temp)}"><xsl:value-of select="count($temp/ele[last()]/preceding-sibling::*)"/></in>
     </xsl:for-each>
    </out> 
  </xsl:template>
</xsl:stylesheet>

I have a fix which appears to solve the problem: the code for building the prior index is changed so that it stops at the end of the document (tree) containing the node whose preceding-sibling is required. (It also indexes all previous documents in the tree if not already done). I will now regression test this to check for side-effects.

I have run the test repeatedly and unfortunately the original problem reoccurs; it seems my patch reduces the probability of hitting problems but does not eliminate it.

Performance experiment: if I run single-threaded, execution time is

with reuse of SequenceOutputter enabled: 237.34ms

with reuse of SequenceOutputter disabled: 224.82ms

So for this workload at least, reuse of the SequenceOutputter (and hence the TinyTree structure) is delivering no benefit. This suggests the simplest fix might be to disable such reuse. (This fixes the problem because the TinyTree is then immutable after construction, and the prior pointer index can then safely be built for the entire tree.)

If I run with reuse of SequenceOutputter disabled, in 4 threads, execution time is 96.15ms; with 8 threads it is 83.98ms; with 12 threads it is 87.71ms.

I ran the XMark benchmark with and without reuse of TinyTree enabled and there was no appreciable difference. So I think the answer is to eliminate this code.