Project

Profile

Help

Bug #3389

closed

Performance regression: sorting nodes into document order

Added by Fady Moussallam over 6 years ago. Updated over 6 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Performance
Sprint/Milestone:
-
Start date:
2017-08-14
Due date:
% Done:

100%

Estimated time:
Legacy ID:
Applies to branch:
9.7, 9.8
Fix Committed on Branch:
9.8
Fixed in Maintenance Release:
Platforms:

Description

Hello,

Some of our queries are taking longer on Saxon 9.7.0-20 than they used to take on Saxon-B 9.0.

In the attached file, you will find a query that seems to be representative of the type of queries that are experiencing the issue.

The attached query is not particularly optimized. It is a stripped down version of the queries we have. Its sole purpose is to show a longer execution time when you run runHE.bat as opposed to runB.bat.

In this case, the increase is in the order of 15/20% but some of our most complex queries are showing as much as a 40% increase.

We would like to avoid having to change the queries themselves as we have a large number of these already running in production so any ideas would be greatly appreciated.

Also worth mentioning, we are actually on Saxon-PE.

Thank you.


Files

case.zip (5.8 MB) case.zip Fady Moussallam, 2017-08-14 10:13
Actions #1

Updated by Michael Kay over 6 years ago

Running with -repeat:50 and ignoring query analysis time, I'm seeing the following timings for the larger input file. Where there are two figures, I measured it twice to check consistency:

9.0 421ms

9.2 406ms

9.3 395ms 410ms

9.4 524ms

9.5 555ms

9.6 547ms

9.7 770ms 757ms

9.8 663ms 672ms

So yes, for this particular query there's been a fair bit of variation between releases, with 9.7 definitely showing regression, and some of this being pulled back in 9.8.

I'm going to focus on 9.8 for investigation. While we are still supporting 9.7, we are managing that release for maximum stability and a fairly minor performance issue like this one would not justify intervention unless we discover from the 9.8 study that there's something that's very simple to correct.

Actions #2

Updated by Michael Kay over 6 years ago

The Java profile for 9.8 shows that the performance is heavily dominated by sorting - probably sorting nodes into document order. There is also a fair bit of sorting happening in the 9.3 trace, but it is far less dominant. (General observation about this query: none of the functions declare the types of their arguments, so there's no type information for the optimizer to work with. So the generated code is always going to assume the worst case.)

Internal tracing in Saxon shows that we are doing exactly 4000 sort operations, each of exactly 1000 or 2000 items.

Actions #3

Updated by Michael Kay over 6 years ago

The sorting seems to be happening at line 4

for $out_root_outloop_outloop_1_c at $out_root_outloop_outloop_1_c_i in 
  let $inVar := $top/root/inloop/inloop_1 return  $inVar

which Saxon has simplified to

for $out_root_outloop_outloop_1_c at $out_root_outloop_outloop_1_c_i in 
  $top/root/inloop/inloop_1

Internally this appears as

for $out_root_outloop_outloop_1_c at $out_root_outloop_outloop_1_c_i in 
   docOrder((docOrder((ConditionalSorter(exists(tail(($top) treat as node())), docOrder((($top) treat as node())/child::element(Q{}root))))/child::element(Q{}inloop)))/child::element(Q{}inloop_1))

The ConditionalSorter is an internal expression which effectively says "if $top is a singleton then return $top!root!inLoop!inLoop1 else return sort($top!root!inLoop!inLoop1) - it's designed to avoid the sort in the case where $top is a singleton node -- which is expressly designed to deal with queries like this one in which users haven't taken the trouble to declare the static type of the variable. However, this is then thwarted by the result being wrapped in a call of docOrder(), which does the sort anyway. This outer call of docOrder should be eliminated as unnecessary, so we need to see why this isn't happening.

Actions #4

Updated by Michael Kay over 6 years ago

At an intermediate stage of optimization, we have an expression such as

Sort (
    ConditionalSort( $var/child::x ) / child::y
)

and the optimizer is not recognizing that the outer Sort is unnecessary. In fact, if $var returns multiple nodes and they are not a peer node-set (one node is an ancestor of another), then the outer sort might indeed be necessary.

We should be rewriting this expression as

ConditionalSort($var / (child::x/child::y) )

Either that, or we should avoid creating it in the first place, but that's probably more difficult. The performance regression is probably because at at earlier release the path expression x/y/z was held in left-associative rather than right-associative form, both of which have their merits for different purposes).

I've implemented this rewrite and it brings the execution time down to 12ms

The same improvement can be achieved in Saxon 9.7 by modifying the query to declare the first function with "$top as node()" (thus saving the optimizer a lot of effort).

Actions #5

Updated by Michael Kay over 6 years ago

  • Subject changed from Increased execution times for some queries on HE 9.7.0-20 as compared to B 9.0 to Performance regression: sorting nodes into document order
  • Category set to Performance
  • Status changed from New to Resolved
  • Assignee set to Michael Kay
  • Applies to branch 9.8 added
  • Fix Committed on Branch 9.8 added

The patch appears to have no adverse consequences so I am committing it on the 9.8 branch.

I consider it too risky to apply to the 9.7 branch, where stability is a higher objective than performance improvement.

Thanks for reporting it.

Actions #6

Updated by Fady Moussallam over 6 years ago

This is outstanding. Thank you! As a side note, your web site mentions "Saxon 9.7 is currently considered the most stable and reliable release", is this still accurate?

Actions #7

Updated by Michael Kay over 6 years ago

9.8 is getting there, but the bug rate is still a bit higher than I would like before recommending people to move forward unless they actually need something in the new version (the best reason for moving forward is because you are using XSLT 3.0 or XQuery 3.1). Furthermore, as this issue demonstrates, we are still prepared to take a few risks in making inessential changes on this release branch, which we will stop doing once it becomes the recommended most stable release.

Actions #8

Updated by O'Neil Delpratt over 6 years ago

  • Status changed from Resolved to Closed
  • % Done changed from 0 to 100
  • Fixed in Maintenance Release 9.8.0.4 added

Bug fix applied in the Saxon 9.8.0.4 maintenance release.

Please register to edit this issue

Also available in: Atom PDF