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 crash in Truffleruby #8602

Merged
merged 2 commits into from Aug 31, 2020
Merged

Fix crash in Truffleruby #8602

merged 2 commits into from Aug 31, 2020

Conversation

jaimerave
Copy link
Contributor

@jaimerave jaimerave commented Aug 27, 2020

This adds a workaround for oracle/truffleruby#1484. With this change Rubocop no longer crashes when parsing files, an all the tests pass under Truffleruby 20.2.0 and head. Unfortunately, the tests are still too slow to add it to CI.

This change also improves performance a little bit:

require "benchmark/ips"

Benchmark.ips do |x|
  x.config(time: 5, warmup: 2)

  source = "this is my source"

  x.report("to_enum") { source.to_enum(:scan, /this is/).map { Regexp.last_match } }
  x.report("workaround") do
    match_datas = []
    source.scan(/this is/) { match_datas << Regexp.last_match }
    match_datas
  end

  x.compare!
end

With ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-darwin19]

Warming up --------------------------------------
             to_enum    53.745k i/100ms
          workaround    73.683k i/100ms
Calculating -------------------------------------
             to_enum    542.663k (± 1.4%) i/s -      2.741M in   5.052060s
          workaround    727.989k (± 1.7%) i/s -      3.684M in   5.062231s

Comparison:
          workaround:   727989.4 i/s
             to_enum:   542663.3 i/s - 1.34x  (± 0.00) slower

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@marcandre
Copy link
Contributor

Cool. You'll need to tweak the changelog (see errors)

Ideally, this would add a github action to test on truffleruby too. I think this might help. Same on rubocop-ast... Are you up to do that?

@jaimerave
Copy link
Contributor Author

@marcandre Github action added, but wow! It is slow. I've have reported the issue to the people in charge of Truffleruby.

@jaimerave
Copy link
Contributor Author

The slow tests issue is now tracked under oracle/truffleruby#2081

@jaimerave jaimerave changed the title Add support for Truffleruby Fix crash in Truffleruby Aug 31, 2020
Copy link

@eregon eregon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I would squash the commits when merging though.

CHANGELOG.md Outdated Show resolved Hide resolved
@eregon
Copy link

eregon commented Aug 31, 2020

@marcandre Who can we ask to review & merge this?

It would be useful to have a release of RuboCop including this fix as it blocks adding TruffleRuby in CI of other gems:
ruby-grape/grape#2099 (comment)

@marcandre marcandre merged commit 6cab599 into rubocop:master Aug 31, 2020
@marcandre
Copy link
Contributor

@jaimerave Thanks for the PR! @eregon Thanks for the ping.

Merged

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.

None yet

3 participants