Details

    • Type: Improvement Improvement
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6-rc-1
    • Fix Version/s: 1.6
    • Component/s: XML Processing
    • Labels:
      None
    • Number of attachments :
      3

      Description

      MarkupBuilder#escapeXmlValue uses an algorithm for XML escaping that has unneccessary space and time requirements.

      The attached patch (for trunk), while having less memory consumption, seems to increase the performance to factors between 5 and 25.

      1. patch-MarkupBuilder-trunk.txt
        6 kB
        Alexander Veit
      2. patch-MarkupBuilder-trunk.txt
        8 kB
        Alexander Veit
      3. patch-MarkupBuilder-trunk.txt
        8 kB
        Alexander Veit

        Activity

        Hide
        Alexander Veit added a comment -

        Sorry, now with correct indentation.

        Show
        Alexander Veit added a comment - Sorry, now with correct indentation.
        Hide
        Alexander Veit added a comment -

        Probably MarkupBuilder#transformValue(String) could be removed in trunk.

        Show
        Alexander Veit added a comment - Probably MarkupBuilder#transformValue(String) could be removed in trunk.
        Hide
        blackdrag blackdrag added a comment -

        if somebody says something is faster I always have first to ask what he tested. You did test escapeXmlValue on Strings where there is nothing to escape I guess. I wouldn't expect this version to be 5-20 times faster for strings with values to be escaped.... Also I think the patch can be improved too... for example using StringBuilder instead of StringBuffer.... and it would be nice if you could avoid the whitespace changes in comments. I would like to see only the parts that really changed

        Show
        blackdrag blackdrag added a comment - if somebody says something is faster I always have first to ask what he tested. You did test escapeXmlValue on Strings where there is nothing to escape I guess. I wouldn't expect this version to be 5-20 times faster for strings with values to be escaped.... Also I think the patch can be improved too... for example using StringBuilder instead of StringBuffer.... and it would be nice if you could avoid the whitespace changes in comments. I would like to see only the parts that really changed
        Hide
        Alexander Veit added a comment -

        Some numbers

        • empty string: 25
        • 8k HTML page: 6
        • 64k string containing only <: 200

        If trunk is intended to support only Java 1.5 and above all local occurrences of StringBuffer should certainly be replaced with StringBuilder.

        For the whitespace: my IDE (Eclipse) removes trailing whitespace when saving files since normally this reduces whitespace noise in development teams. Sorry for that.

        Show
        Alexander Veit added a comment - Some numbers empty string: 25 8k HTML page: 6 64k string containing only <: 200 If trunk is intended to support only Java 1.5 and above all local occurrences of StringBuffer should certainly be replaced with StringBuilder. For the whitespace: my IDE (Eclipse) removes trailing whitespace when saving files since normally this reduces whitespace noise in development teams. Sorry for that.
        Hide
        blackdrag blackdrag added a comment -

        what are these numbers? The factor the new version is faster? Well for the empty string that may make sense, but that is not really a relevant case I think. The HTML page is more interesting and more in the area I expected.... but the 64k string... factor 200... that is unexpected.

        trunk is supposed to support 1.5 only yes. The retro build can create a 1.4 compatible version for StringBuilder, so the usage of that class is no problem. We are also step-by-step replacing all usages of StringBuffer with StringBuilder already

        Show
        blackdrag blackdrag added a comment - what are these numbers? The factor the new version is faster? Well for the empty string that may make sense, but that is not really a relevant case I think. The HTML page is more interesting and more in the area I expected.... but the 64k string... factor 200... that is unexpected. trunk is supposed to support 1.5 only yes. The retro build can create a 1.4 compatible version for StringBuilder, so the usage of that class is no problem. We are also step-by-step replacing all usages of StringBuffer with StringBuilder already
        Hide
        Alexander Veit added a comment -

        Reduced whitespace noise.

        Show
        Alexander Veit added a comment - Reduced whitespace noise.
        Hide
        Paul King added a comment -

        Applied a variation of your patch locally but with StringBuilder. Only noticed very minor improvement - but didn't try many cases. I think I will apply anyway shortly - the code is cleaner looking. Not critical for 1.6 RC-2. See what else happens and if I get time before that is released.

        Show
        Paul King added a comment - Applied a variation of your patch locally but with StringBuilder. Only noticed very minor improvement - but didn't try many cases. I think I will apply anyway shortly - the code is cleaner looking. Not critical for 1.6 RC-2. See what else happens and if I get time before that is released.
        Hide
        Pascal Schumacher added a comment -

        Any updates on this?

        Show
        Pascal Schumacher added a comment - Any updates on this?
        Hide
        Paul King added a comment - - edited

        All essential parts of the suggested patch have been previously applied. If you can think of any further improvements relative to master, please open a new issue. Thanks for the suggestions and patch.

        Show
        Paul King added a comment - - edited All essential parts of the suggested patch have been previously applied. If you can think of any further improvements relative to master, please open a new issue. Thanks for the suggestions and patch.

          People

          • Assignee:
            Paul King
            Reporter:
            Alexander Veit
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: