Project

Profile

Help

Bug #5483

closed

Failure to optimize SEQ[position() = 1 to $n]

Added by Michael Kay almost 2 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Performance
Sprint/Milestone:
-
Start date:
2022-05-10
Due date:
% Done:

100%

Estimated time:
Legacy ID:
Applies to branch:
11, trunk
Fix Committed on Branch:
11, trunk
Fixed in Maintenance Release:
Platforms:
.NET, Java

Description

By design, a filter expression of the form SEQ[position() = 1 to $n] should be optimised to subsequence(SEQ, 1, $n).

This isn't happening (revealed by xslt30-extra test case opt-016). The subexpression 1 to $n is extracted into a variable, which means that the pattern posiiton() = 1 to $n doesn't get recognized.

Actions #1

Updated by Michael Kay almost 2 years ago

It seems to be working on the Saxon 10 branch.

On the 10 branch, by the time we get to FilterExpression.tryToRewritePositionalFilter(), the predicate is an IntegerRangeTest. On the 11 branch it is a GeneralComparison.

The reason the IntegerRangeTest doesn't get created is that the code to create it at GeneralComparison.optimize() (line 474) has been commented out - after first being changed to throw an AssertionError with the message "optimization needs rethinking"!

The commenting out appears to have happened in January 2021 very early in the work on C# transpilation, and it's possible that the code was commented out simply because of early difficulties transpiling it, and never reinstated. (Except that wouldn't explain the comment about rethinking the optimization).

Actions #2

Updated by Michael Kay almost 2 years ago

On further examination, I think the optimisation was commented out when RangeExpression was extended to handle experimental 4.0 syntax "$Start to $End by $Step". Simplest fix is to reinstate the optimisation provided $Step is 1.

Actions #3

Updated by Michael Kay almost 2 years ago

I've now reinstated the optimization and opt-016 is looking good -- except there's an odd problem that the SEF file contains 4 global variables, of which 2 are unreferenced. In fact there are two identical expressions so ideally they would be detected as duplicates and we would only have a single global variable.

Actions #4

Updated by Michael Kay almost 2 years ago

It seems the SEF file only contains two global variables, but the explain output contains four.

We're running the test with -export:on. The "explain" output is generated by running PreparedStylesheet.explain() on the reconstituted executable, and this appears to have a list of four global variables (plus the GlobalParam). It we run with -export:off, the same list has only two global variables (plus a GlobalParam). That's clearly wrong...

Actions #5

Updated by Michael Kay almost 2 years ago

In fact there are only two global variables in the reconstituted executable, but the StylesheetPackageEE has a list of global variables, and this list contains duplicate entries; it is this list that is used to generate the explain output, so the two variables each appear twice.

The first addition is from PackageLoaderHE.readGlobalVariable() at line 561. The second is from PackageLoaderHE.registerGlobalVariable() at line 529. It looks to me as if the second call is redundant.

I've removed this call and confirmed that all tests still run OK with -export:on

Actions #6

Updated by Michael Kay almost 2 years ago

  • Status changed from New to In Progress

Now fixed - on the Saxon 11 branch. Needs retrofitting to the main branch.

Actions #7

Updated by Michael Kay almost 2 years ago

  • Subject changed from Failure to optimize SEQ[posiiton() = 1 to $n] to Failure to optimize SEQ[position() = 1 to $n]
  • Description updated (diff)
Actions #8

Updated by Michael Kay almost 2 years ago

  • Status changed from In Progress to Resolved
  • Applies to branch 11, trunk added
  • Fix Committed on Branch 11, trunk added
  • Platforms .NET, Java added
Actions #9

Updated by Debbie Lockett almost 2 years ago

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

Bug fix applied in the Saxon 11.4 maintenance release.

Please register to edit this issue

Also available in: Atom PDF