From 0496940d5998ccbc50d16fb734993ab50fc60c2d Mon Sep 17 00:00:00 2001 From: NAITOH Jun Date: Mon, 18 Mar 2024 23:30:47 +0900 Subject: [PATCH] Optimize the parse_attributes method to use `Source#match` to parse XML. (#119) ## Why? Improve maintainability by consolidating processing into `Source#match`. ## Benchmark ``` RUBYLIB= BUNDLER_ORIG_RUBYLIB= /Users/naitoh/.rbenv/versions/3.3.0/bin/ruby -v -S benchmark-driver /Users/naitoh/ghq/github.com/naitoh/rexml/benchmark/parse.yaml ruby 3.3.0 (2023-12-25 revision 5124f9ac75) [arm64-darwin22] Calculating ------------------------------------- before after before(YJIT) after(YJIT) dom 10.891 10.622 16.356 17.403 i/s - 100.000 times in 9.182130s 9.414177s 6.113806s 5.746133s sax 30.335 29.845 49.749 54.877 i/s - 100.000 times in 3.296483s 3.350595s 2.010071s 1.822259s pull 35.514 34.801 61.123 66.908 i/s - 100.000 times in 2.815793s 2.873484s 1.636041s 1.494591s stream 35.141 34.475 52.110 56.836 i/s - 100.000 times in 2.845646s 2.900638s 1.919017s 1.759456s Comparison: dom after(YJIT): 17.4 i/s before(YJIT): 16.4 i/s - 1.06x slower before: 10.9 i/s - 1.60x slower after: 10.6 i/s - 1.64x slower sax after(YJIT): 54.9 i/s before(YJIT): 49.7 i/s - 1.10x slower before: 30.3 i/s - 1.81x slower after: 29.8 i/s - 1.84x slower pull after(YJIT): 66.9 i/s before(YJIT): 61.1 i/s - 1.09x slower before: 35.5 i/s - 1.88x slower after: 34.8 i/s - 1.92x slower stream after(YJIT): 56.8 i/s before(YJIT): 52.1 i/s - 1.09x slower before: 35.1 i/s - 1.62x slower after: 34.5 i/s - 1.65x slower ``` - YJIT=ON : 1.06x - 1.10x faster - YJIT=OFF : 0.97x - 0.98x faster --- lib/rexml/parsers/baseparser.rb | 116 ++++++++++++-------------------- test/parse/test_element.rb | 4 +- test/test_core.rb | 20 +++++- 3 files changed, 64 insertions(+), 76 deletions(-) diff --git a/lib/rexml/parsers/baseparser.rb b/lib/rexml/parsers/baseparser.rb index c01b087b..f66b968f 100644 --- a/lib/rexml/parsers/baseparser.rb +++ b/lib/rexml/parsers/baseparser.rb @@ -114,7 +114,7 @@ class BaseParser module Private INSTRUCTION_END = /#{NAME}(\s+.*?)?\?>/um - TAG_PATTERN = /((?>#{QNAME_STR}))/um + TAG_PATTERN = /((?>#{QNAME_STR}))\s*/um CLOSE_PATTERN = /(#{QNAME_STR})\s*>/um ATTLISTDECL_END = /\s+#{NAME}(?:#{ATTDEF})*\s*>/um NAME_PATTERN = /\s*#{NAME}/um @@ -128,7 +128,6 @@ module Private def initialize( source ) self.stream = source @listeners = [] - @attributes_scanner = StringScanner.new('') end def add_listener( listener ) @@ -614,87 +613,60 @@ def process_instruction(start_position) def parse_attributes(prefixes, curr_ns) attributes = {} closed = false - match_data = @source.match(/^(.*?)(\/)?>/um, true) - if match_data.nil? - message = "Start tag isn't ended" - raise REXML::ParseException.new(message, @source) - end - - raw_attributes = match_data[1] - closed = !match_data[2].nil? - return attributes, closed if raw_attributes.nil? - return attributes, closed if raw_attributes.empty? - - @attributes_scanner.string = raw_attributes - scanner = @attributes_scanner - until scanner.eos? - if scanner.scan(/\s+/) - break if scanner.eos? - end - - start_position = scanner.pos - while true - break if scanner.scan(ATTRIBUTE_PATTERN) - unless scanner.scan(QNAME) - message = "Invalid attribute name: <#{scanner.rest}>" - raise REXML::ParseException.new(message, @source) - end - name = scanner[0] - unless scanner.scan(/\s*=\s*/um) + while true + if @source.match(">", true) + return attributes, closed + elsif @source.match("/>", true) + closed = true + return attributes, closed + elsif match = @source.match(QNAME, true) + name = match[1] + prefix = match[2] + local_part = match[3] + + unless @source.match(/\s*=\s*/um, true) message = "Missing attribute equal: <#{name}>" raise REXML::ParseException.new(message, @source) end - quote = scanner.scan(/['"]/) - unless quote - message = "Missing attribute value start quote: <#{name}>" - raise REXML::ParseException.new(message, @source) - end - unless scanner.scan(/.*#{Regexp.escape(quote)}/um) - @source.ensure_buffer - match_data = @source.match(/^(.*?)(\/)?>/um, true) - if match_data - scanner << "/" if closed - scanner << ">" - scanner << match_data[1] - scanner.pos = start_position - closed = !match_data[2].nil? - next + unless match = @source.match(/(['"])(.*?)\1\s*/um, true) + if match = @source.match(/(['"])/, true) + message = + "Missing attribute value end quote: <#{name}>: <#{match[1]}>" + raise REXML::ParseException.new(message, @source) + else + message = "Missing attribute value start quote: <#{name}>" + raise REXML::ParseException.new(message, @source) end - message = - "Missing attribute value end quote: <#{name}>: <#{quote}>" - raise REXML::ParseException.new(message, @source) end - end - name = scanner[1] - prefix = scanner[2] - local_part = scanner[3] - # quote = scanner[4] - value = scanner[5] - if prefix == "xmlns" - if local_part == "xml" - if value != "http://www.w3.org/XML/1998/namespace" - msg = "The 'xml' prefix must not be bound to any other namespace "+ + value = match[2] + if prefix == "xmlns" + if local_part == "xml" + if value != "http://www.w3.org/XML/1998/namespace" + msg = "The 'xml' prefix must not be bound to any other namespace "+ + "(http://www.w3.org/TR/REC-xml-names/#ns-decl)" + raise REXML::ParseException.new( msg, @source, self ) + end + elsif local_part == "xmlns" + msg = "The 'xmlns' prefix must not be declared "+ "(http://www.w3.org/TR/REC-xml-names/#ns-decl)" - raise REXML::ParseException.new( msg, @source, self ) + raise REXML::ParseException.new( msg, @source, self) end - elsif local_part == "xmlns" - msg = "The 'xmlns' prefix must not be declared "+ - "(http://www.w3.org/TR/REC-xml-names/#ns-decl)" - raise REXML::ParseException.new( msg, @source, self) + curr_ns << local_part + elsif prefix + prefixes << prefix unless prefix == "xml" end - curr_ns << local_part - elsif prefix - prefixes << prefix unless prefix == "xml" - end - if attributes.has_key?(name) - msg = "Duplicate attribute #{name.inspect}" - raise REXML::ParseException.new(msg, @source, self) - end + if attributes.has_key?(name) + msg = "Duplicate attribute #{name.inspect}" + raise REXML::ParseException.new(msg, @source, self) + end - attributes[name] = value + attributes[name] = value + else + message = "Invalid attribute name: <#{@source.buffer.split(%r{[/>\s]}).first}>" + raise REXML::ParseException.new(message, @source) + end end - return attributes, closed end end end diff --git a/test/parse/test_element.rb b/test/parse/test_element.rb index 9f172a28..987214f3 100644 --- a/test/parse/test_element.rb +++ b/test/parse/test_element.rb @@ -41,9 +41,9 @@ def test_empty_namespace_attribute_name assert_equal(<<-DETAIL.chomp, exception.to_s) Invalid attribute name: <:a=""> Line: 1 -Position: 9 +Position: 13 Last 80 unconsumed characters: - +:a=""> DETAIL end diff --git a/test/test_core.rb b/test/test_core.rb index 5668b934..44e2e7ea 100644 --- a/test/test_core.rb +++ b/test/test_core.rb @@ -116,11 +116,12 @@ def test_attribute def test_attribute_namespace_conflict # https://www.w3.org/TR/xml-names/#uniqAttrs - message = <<-MESSAGE + message = <<-MESSAGE.chomp Duplicate attribute "a" Line: 4 Position: 140 Last 80 unconsumed characters: +/> MESSAGE assert_raise(REXML::ParseException.new(message)) do Document.new(<<-XML) @@ -1323,11 +1324,26 @@ def test_ticket_21 exception = assert_raise(ParseException) do Document.new(src) end - assert_equal(<<-DETAIL, exception.to_s) + assert_equal(<<-DETAIL.chomp, exception.to_s) Missing attribute value start quote: Line: 1 Position: 16 Last 80 unconsumed characters: +value/> + DETAIL + end + + def test_parse_exception_on_missing_attribute_end_quote + src = ' +Line: 1 +Position: 17 +Last 80 unconsumed characters: +value/> DETAIL end