Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: RelaxNG.read_memory is now reliable #3259

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

flavorjones
Copy link
Member

What problem is this PR intended to solve?

On both JRuby and CRuby impls, replace the native read_memory method
from XML::Schema and XML::RelaxNG with a ruby method that uses
from_document. read_memory was buggy on both platforms, but
especially on the JRuby impl.

This is comparable in performance on CRuby. From a benchmark taken
before this change:

Warming up --------------------------------------
          Schema.new     1.219k i/100ms
Schema.from_document     1.258k i/100ms
  Schema.read_memory     1.118k i/100ms
Calculating -------------------------------------
          Schema.new     12.160k (± 8.3%) i/s -    121.900k in  10.093638s
Schema.from_document     12.216k (± 8.6%) i/s -    122.026k in  10.059696s
  Schema.read_memory     12.790k (±10.7%) i/s -    127.452k in  10.105931s

Comparison:
  Schema.read_memory:    12789.6 i/s
Schema.from_document:    12215.9 i/s - same-ish: difference falls within error
          Schema.new:    12160.1 i/s - same-ish: difference falls within error

IMHO the resulting code is less buggy and easier to maintain, and the
slight (if any) performance hit is worth it.

Also: improve documentation for Schema and RelaxNG classes.

Closes #2113
Closes #2115

Have you included adequate test coverage?

Does this change affect the behavior of either the C or the Java implementations?

@flavorjones flavorjones added this to the v1.17.0 milestone Jun 24, 2024
On both JRuby and CRuby impls, replace the native `read_memory` method
from XML::Schema and XML::RelaxNG with a ruby method that uses
`from_document`. `read_memory` was buggy on both platforms, but
especially on the JRuby impl.

This is comparable in performance on CRuby. From a benchmark taken
before this change:

    Warming up --------------------------------------
              Schema.new     1.219k i/100ms
    Schema.from_document     1.258k i/100ms
      Schema.read_memory     1.118k i/100ms
    Calculating -------------------------------------
              Schema.new     12.160k (± 8.3%) i/s -    121.900k in  10.093638s
    Schema.from_document     12.216k (± 8.6%) i/s -    122.026k in  10.059696s
      Schema.read_memory     12.790k (±10.7%) i/s -    127.452k in  10.105931s

    Comparison:
      Schema.read_memory:    12789.6 i/s
    Schema.from_document:    12215.9 i/s - same-ish: difference falls within error
              Schema.new:    12160.1 i/s - same-ish: difference falls within error

IMHO the resulting code is less buggy and easier to maintain, and the
slight (if any) performance hit is worth it.

Closes #2113
Closes #2115
@flavorjones flavorjones force-pushed the flavorjones-clean-up-xml-schema-part-3 branch from 4f5fb23 to 49c5b8b Compare June 24, 2024 21:18
@flavorjones flavorjones enabled auto-merge June 24, 2024 21:45
@flavorjones flavorjones merged commit 925f3be into main Jun 24, 2024
131 of 132 checks passed
@flavorjones flavorjones deleted the flavorjones-clean-up-xml-schema-part-3 branch June 24, 2024 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant