Skip to content

use optimized CGIEscape#5949

Closed
ahorek wants to merge 1 commit intojruby:masterfrom
ahorek:cgifix
Closed

use optimized CGIEscape#5949
ahorek wants to merge 1 commit intojruby:masterfrom
ahorek:cgifix

Conversation

@ahorek
Copy link
Contributor

@ahorek ahorek commented Oct 28, 2019

as commented in #5937

CGI.escapeHTML does always use a pure ruby version which is slower

https://github.com/ruby/ruby/blob/master/ext/cgi/escape/escape.c#L408

ruby
CGI.escapeHTML    242.472  (? 7.0%) i/s -      1.220k in   5.061783s
optimized
CGI.escapeHTML    804.555  (? 5.6%) i/s -      4.032k in   5.027866s

@ahorek ahorek changed the title WIP: use optimized CGIEscape use optimized CGIEscape Oct 28, 2019
@ahorek
Copy link
Contributor Author

ahorek commented Oct 28, 2019

If I understand this correctly, the original issue was fixed in #5627 and the CGI tweak was partly reverted in 3d8847e . Is it safe to use the CGIEscape.java library now? I didn't find any issues and the CI is green. ping @headius ?

@headius
Copy link
Member

headius commented Oct 29, 2019

Looking into it. I want to understand what it was disabled.

@headius
Copy link
Member

headius commented Oct 29, 2019

I attempted to uncomment both of those lines and that results in errors like this during test/mri/cgi tests:

CGIUtilTest#test_cgi_unescapeHTML:UTF-32LE:
NoMethodError: super: no superclass method `unescapeHTML' for CGI:Class
Did you mean?  unescape_html
               escape_html
               unescapeHTML
               escapeHTML
    org/jruby/RubyBasicObject.java:1709:in `method_missing'
    org/jruby/ext/cgi/escape/CGIEscape.java:377:in `unescapeHTML'
    org/jruby/ext/cgi/escape/CGIEscape.java:377:in `unescapeHTML'
    /Users/headius/projects/jruby/test/mri/cgi/test_cgi_util.rb:135:in `block in CGIUtilTest'

And the issue in #4678 still causes a similar error attempting to call a super foo. Will poke at this a bit but it may be a deeper problem than we can fix for 9.2.9 on short notice.

@ahorek
Copy link
Contributor Author

ahorek commented Oct 29, 2019

ok, if it's too risky, feel free to postpone it

@headius
Copy link
Member

headius commented Oct 29, 2019

Yeah I'll postpone it. There's something not right about how we're putting this particular class hierarchy together, or else something wrong with the way super searches that hierarchy. I'll prioritize this for 9.2.10 so we can get rid of the remaining hacks and workarounds.

@headius
Copy link
Member

headius commented Jul 1, 2021

I realized a simple way we can integrate this: implement it as a separate ext that does not have to be prepended and supered in the ways MRI does. Looking into this quickly as an option.

@headius headius modified the milestones: JRuby 9.3.0.0, JRuby 9.4.0.0 Jul 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants