Project

Profile

Help

Support #3233

closed

Unused local variable gets evaluated, causing an error

Added by Radu Coravu almost 7 years ago. Updated over 6 years ago.

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

100%

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

Description

This is not really a bug, maybe it's a request for comment.

For this XSLT stylesheet:

<xsl:stylesheet xmlns:fo="http://www.w3.org/1999/XSL/Format"
    xmlns:xsl="http://www.w3.org/1999/XSL/Transform" xmlns:xs="http://www.w3.org/2001/XMLSchema"
    version="2.0">
    <xsl:output indent="yes"/>
    <xsl:template match="/">
        <xsl:variable name="totalCost">
            <xsl:call-template name="sum">
            </xsl:call-template>
        </xsl:variable>
        <xsl:variable name="totalVAT" select="($totalCost*0.24)">
        </xsl:variable>
        <root></root>
    </xsl:template>
    <xsl:template name="sum"/>
</xsl:stylesheet>

Saxon 9.7 reports this problem on the variable called totalVAT

FORG0001: String to double conversion: no digits found

which is OK because the "totalCost" is an empty string and cannot be multiplied as an integer.

Somehow Saxon 9.6 never reported this problem, probably because it never evaluated the $totalCost as it is not used anywhere.

So Saxon 9.7 seems more eager to evaluate variables, even if they are not used.


Files

invoiceVendor.xml (781 Bytes) invoiceVendor.xml Radu Coravu, 2017-07-20 12:38
invoiceVendor.xsl (12.1 KB) invoiceVendor.xsl Radu Coravu, 2017-07-20 12:38
Actions #1

Updated by Michael Kay almost 7 years ago

  • Category set to Performance
  • Assignee set to Michael Kay
  • Priority changed from Low to Normal

Thanks for pointing this out. I think this is probably an unintended side-effect of some recent bug fix. The intent is that LetExpression.optimize() should eliminate a variable declaration if there are no references to the variable; but we need to be very careful before doing this that the reference list is accurate (it can easily become wrong as a result of other changes to the expression tree). What is happening is that XSLTemplate.optimize calls resetLocalProperties(), and the intent is that this should rebuild the reference list. However, if there are no references to the variable, then it is leaving the reference list null, and LetExpression.optimize() does not eliminate the variable if the reference list is null.

Actions #2

Updated by Michael Kay almost 7 years ago

Changing LetExpression.resetLocalStaticProperties() to set references to an empty list rather than to null seems to solve the problem, but it will need careful regression testing.

Actions #3

Updated by Michael Kay almost 7 years ago

  • Status changed from New to Resolved
  • Applies to branch 9.7, trunk added
  • Fix Committed on Branch 9.7, trunk added

This change was applied and I am now satisfied that it fixes the problem.

Actions #4

Updated by O'Neil Delpratt almost 7 years ago

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

Bug fix applied in the 9.7.0.19 maintenance release.

Actions #5

Updated by Radu Coravu almost 7 years ago

Unfortunately t

Actions #6

Updated by Radu Coravu almost 7 years ago

Unfortunately the fix does not work on the larger XSLT from which I extracted the initial sample XSLT.

I'm attaching a sample XML + XSL. Even with the fix, the XSLT complains that the value for "totalVAT" cannot be properly computed, although the variable is still not used anywhere.

Actions #7

Updated by Michael Kay almost 7 years ago

  • Status changed from Closed to In Progress
Actions #8

Updated by Michael Kay almost 7 years ago

This is a consequence of the large size of the match="/" template rule.

We've been making a serious effort in 9.8 to reduce compile time, and to achieve this we looked at where large costs were being incurred during optimization. One of these was that some operations to simplify the expression tree were quadratic in the size of the expression tree, and so there are some optimizations we no longer attempt if the tree is above a certain size. This includes the elimination of a variable that is never used: for this particular case, we abandon the search after 500 nodes in the expression tree.

It's very hard to say whether we're doing the right thing here. It's all very pragmatic. However, we were seeing a significant number of use cases where we were clearly doing more work at compile time than was justified by the run-time improvements achieved, so optimizing a little less aggressively seemed a sensible thing to do.

For this particular use case it occurs to me that you have a large block of code that doesn't depend on any variables at all. We have an expression property "depends on local variables", and if that property value is trustworthy then we should be able to prune the search when looking for references to one particular variable. I'll experiment with this. The danger is that all derived information on the expression tree can get out of date as a consequence of tree rewrites.

Actions #9

Updated by Michael Kay almost 7 years ago

Making this change led to four xsl:evaluate tests failing (-009, -012, -013, -014) complaining that a variable binding has been deleted when references to the variable exist. In all these cases the variable is referenced in xsl:evaluate/xsl:with-param/@select. So presumably the DEPENDS_ON_LOCAL_VARIABLES property is not being correctly set in this case.

Actions #10

Updated by Michael Kay almost 7 years ago

Indeed, EvaluateInstr.computeDependencies() fails to consider the xsl:with-param children. In fact the implementation of this method is very poor: it should rely on the generic implementation which gets all the dependencies of the expression's operands, customized by supplying a getIntrinsticDependencies method that returns the intrinsic dependencies of the expression irrespective of its operands. Changing this fixes the regression.

Actions #11

Updated by Michael Kay almost 7 years ago

I was doing that on the 9.8 branch, I now realise this was originally a 9.7 bug, so I need to look at the 9.7 behaviour.

Actions #12

Updated by Michael Kay almost 7 years ago

  • Status changed from In Progress to Resolved
  • Applies to branch 9.8 added
  • Applies to branch deleted (trunk)
  • Fix Committed on Branch 9.8 added
  • Fix Committed on Branch deleted (trunk)

The relevant code is the same on the 9.7 and 9.8 branches so I have committed both patches to both these branches.

Actions #13

Updated by Radu Coravu over 6 years ago

I kept trying to come up with a small XSLT reproducing the problem and I kept failing because once the XSLT got smaller the problem could no longer be reproduced :)

Thanks for the fix, the problem is of course not critical for us. We integrated Saxon 9.7.0.19 with Oxygen and all the other fixes you made in the meantime seem fine, thanks for being so responsive.

Actions #14

Updated by O'Neil Delpratt over 6 years ago

  • Fixed in Maintenance Release 9.7.0.20 added
  • Fixed in Maintenance Release deleted (9.7.0.19)

Bug fix applied in the Saxon 9.7.0.20 maintenance release.

Actions #15

Updated by Michael Kay over 6 years ago

  • Subject changed from Difference in behavior between Saxon 9.6 and 9.7 to Unused local variable gets evaluated, causing an error
Actions #16

Updated by O'Neil Delpratt over 6 years ago

  • Status changed from Resolved to Closed
  • Fixed in Maintenance Release 9.8.0.4 added

Bug fix applied in the 9.8.0.4 maintenance release.

Please register to edit this issue

Also available in: Atom PDF