groovy

MarkupBuilder performance

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. Text File
    patch-MarkupBuilder-trunk.txt
    05/Jan/09 10:10 AM
    6 kB
    Alexander Veit
  2. Text File
    patch-MarkupBuilder-trunk.txt
    05/Jan/09 8:31 AM
    8 kB
    Alexander Veit
  3. Text File
    patch-MarkupBuilder-trunk.txt
    05/Jan/09 8:22 AM
    8 kB
    Alexander Veit

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Alexander Veit added a comment -

Sorry, now with correct indentation.

Show
Alexander Veit added a comment - Sorry, now with correct indentation.
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Alexander Veit added a comment -

Reduced whitespace noise.

Show
Alexander Veit added a comment - Reduced whitespace noise.
Hide
Permalink
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
Permalink
Pascal Schumacher added a comment -

Any updates on this?

Show
Pascal Schumacher added a comment - Any updates on this?
Hide
Permalink
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
Vote (1)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: