Move rdoc require out of repeated execution path #416

st0012 · 2022-10-18T09:14:24Z

SHOW_DOC_DIALOG will be called repeatedly whenever the corresponding key is pressed, but we only need to require rdoc once. So ideally the require can be put outside of the proc.

And because when rdoc is not available the entire proc will be nonfunctional, we can stop registering the SHOW_DOC_DIALOG if we failed to require rdoc.

st0012 · 2022-10-19T10:09:01Z

@junaruga I saw #395 and found this potential improvement. Let me know what you think about it :-)

junaruga · 2022-10-19T17:31:23Z

Sure. I will check it tomorrow and will let you know it.

junaruga · 2022-10-20T10:14:14Z

Now I checked the PR's code.

This PR's intent is to improve the performance, avoid calling the require 'rdoc' repeatedly, because accessing the rdoc files to check if it exists repeatedly is costy. Right?

I suppose if we keep the compatibility from the previous code, when the require 'rdoc' raises the LoadError, we still need to set the nil to the Reline.add_dialog_proc's 2nd argument. So, the code can be below.

      if IRB.conf[:USE_AUTOCOMPLETE]
        dialog = nil 
        begin
          require 'rdoc'
          dialog = SHOW_DOC_DIALOG
        rescue LoadError
        end
        Reline.add_dialog_proc(:show_doc, dialog, Reline::DEFAULT_DIALOG_CONTEXT)
      end

But if we don't need to call the Reline.add_dialog_proc in the case of the require 'rdoc' raising the LoadError, the current PR looks ok.

SHOW_DOC_DIALOG will be called repeatedly whenever the corresponding key is pressed, but we only need to require rdoc once. So ideally the require can be put outside of the proc. And because when rdoc is not available the entire proc will be nonfunctional, we can stop registering the SHOW_DOC_DIALOG if we failed to require rdoc.

st0012 · 2022-10-20T10:47:24Z

This PR's intent is to improve the performance, avoid calling the require 'rdoc' repeatedly, because accessing the rdoc files to check if it exists repeatedly is costy. Right?

Yes. Especially in the case when rdoc is not installed, it'll repeatedly raise and rescue LoadError.

I suppose if we keep the compatibility from the previous code, when the require 'rdoc' raises the LoadError, we still need to set the nil to the Reline.add_dialog_proc's 2nd argument.

Actually, the dialog can't be nil because when being rendered it'll cause error at this line:

/Users/st0012/.rbenv/versions/3.1.0/lib/ruby/gems/3.1.0/gems/reline-0.3.1/lib/reline/line_editor.rb:588:in `instance_exec': no block given (LocalJumpError)

So my test cases were actually misleading and I've updated them.

If we still want to have the :show_proc dialog registered for compatibility reason, we can use Proc.new {} instead.

But if we don't need to call the Reline.add_dialog_proc in the case of the require 'rdoc' raising the LoadError

I think we don't need to call Reline.add_dialog_proc in that case tbh. Because reline iterates through registered dialogs in every rendering and performs certain computation even when if the dialog proc is empty, it should be avoided when possible.

junaruga · 2022-10-20T12:12:54Z

Yes. Especially in the case when rdoc is not installed, it'll repeatedly raise and rescue LoadError.

Ah, ok. That process looks costy.

Actually, the dialog can't be nil because when being rendered it'll cause error at this line:

Oh I see. Thanks for finding the issue.

If we still want to have the :show_proc dialog registered for compatibility reason, we can use Proc.new {} instead.

The :show_proc dialog? You meant :show_doc dialog?

Right now it's not easy to test the non-rdoc case. When we create the Ruby RPM package in the Fedora project, we make the separated rdoc RPM package as a soft (weak or optional) dependency. In the situation, I can test it the case. So, right now I am ok to merge this PR.

I think we don't need to call Reline.add_dialog_proc in that case tbh. Because reline iterates through registered dialogs in every rendering and performs certain computation even when if the dialog proc is empty, it should be avoided when possible.

OK. Thanks for checking it.

st0012 · 2022-10-20T12:35:34Z

The :show_proc dialog? You meant :show_doc dialog?

Yes 🤦‍♂️

Right now it's not easy to test the non-rdoc case.

Yeah I'm aware of that. I'll see if having unit-tests like TestRelineInputMethod makes implementing the conditional mocking you mentioned in #396 easier.

junaruga · 2022-10-21T12:18:42Z

Yeah I'm aware of that. I'll see if having unit-tests like TestRelineInputMethod makes implementing the conditional mocking you mentioned in #396 easier.

OK. That's great for you to work on covering the no-rdoc case.
Maintainers for the IRB, could you review this PR?

st0012 · 2022-10-21T20:42:26Z

@junaruga I've added a case for the rdoc missing scenario based on your snippet in #395 (comment). Let me know if that's an acceptable approach to you.

junaruga · 2022-10-21T21:42:08Z

test/irb/test_input_method.rb

+      IRB.conf[:USE_AUTOCOMPLETE] = true
+
+      without_rdoc do
+        IRB::RelineInputMethod.new


The assert commands should be in the wethout_rdoc block shouldn't be?

without_rdoc do IRB::RelineInputMethod.new assert Reline.autocompletion # doesn't register show_doc dialog assert_equal empty_proc, Reline.dialog_proc(:show_doc).dialog_proc end

I think it doesn't need to be? Because Reline's state isn't affected by the without_rdoc block. We just need to make sure the logic that changes Reline's state is inside the block.

Ah, you are right. Yes, you don't need to move the assertions.

junaruga · 2022-10-21T21:45:37Z

test/irb/test_input_method.rb

+      Reline.add_dialog_proc(:show_doc, original_show_doc_proc, Reline::DEFAULT_DIALOG_CONTEXT)
+    end
+
+    def without_rdoc(&block)


How about moving the without_rdoc method to the common file test/lib/helper.rb, as the logic: require 'rdoc' and resure LoadError is in other parts too?

$ grep -r rdoc lib/ lib/irb/cmd/help.rb: require 'rdoc/ri/driver' lib/irb/completion.rb: require 'rdoc' lib/irb/easter-egg.rb: require "rdoc" lib/irb/input-method.rb: require 'rdoc'

I want to leave it to the next PR (which I'll open after this is merged) because the goal of this one is to move out of the require "rdoc". Increasing the coverage is just a good addition. WDYT?

Yes, I agree with you. It can be the next PR. Without the test_initialization_with_use_autocomplete_but_without_rdoc test in this PR, it's still better than the current situation.

junaruga · 2022-10-21T21:50:58Z

I've added a case for the rdoc missing scenario based on your snippet in #395 (comment). Let me know if that's an acceptable approach to you.

Thanks for that! That looks great. I like the approach of the without_rdoc as a direction. I commented. I am not sure if the test with the without_rdoc works on Bundler on RubyGems to pass the CI after moving the assertions to the inside of the without_rdoc.

st0012 · 2022-10-21T22:31:13Z

cc @peterzhu2118

peterzhu2118

Looks good, thank you!

st0012 force-pushed the follow-#395 branch from 0d9e78b to d9c0404 Compare October 18, 2022 10:25

st0012 added 2 commits October 20, 2022 11:28

Add tests for RelineInputMethod

a1406b1

st0012 force-pushed the follow-#395 branch from d9c0404 to b1278b7 Compare October 20, 2022 10:32

Add a test case to cover rdoc unintalled scenario

aa78dd6

junaruga reviewed Oct 21, 2022

View reviewed changes

peterzhu2118 approved these changes Oct 24, 2022

View reviewed changes

peterzhu2118 merged commit daa1ea1 into ruby:master Oct 24, 2022

st0012 deleted the follow-#395 branch October 24, 2022 13:37

st0012 mentioned this pull request Oct 28, 2022

Adding test cases on no rdoc gem case. #396

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move rdoc require out of repeated execution path #416

Move rdoc require out of repeated execution path #416

st0012 commented Oct 18, 2022

st0012 commented Oct 19, 2022

junaruga commented Oct 19, 2022

junaruga commented Oct 20, 2022

st0012 commented Oct 20, 2022

junaruga commented Oct 20, 2022

st0012 commented Oct 20, 2022

junaruga commented Oct 21, 2022 •

edited

Loading

st0012 commented Oct 21, 2022

junaruga Oct 21, 2022

st0012 Oct 21, 2022

junaruga Oct 21, 2022

junaruga Oct 21, 2022

st0012 Oct 21, 2022

junaruga Oct 21, 2022

junaruga commented Oct 21, 2022

st0012 commented Oct 21, 2022

peterzhu2118 left a comment

Move rdoc require out of repeated execution path #416

Move rdoc require out of repeated execution path #416

Conversation

st0012 commented Oct 18, 2022

st0012 commented Oct 19, 2022

junaruga commented Oct 19, 2022

junaruga commented Oct 20, 2022

st0012 commented Oct 20, 2022

junaruga commented Oct 20, 2022

st0012 commented Oct 20, 2022

junaruga commented Oct 21, 2022 • edited Loading

st0012 commented Oct 21, 2022

junaruga Oct 21, 2022

Choose a reason for hiding this comment

st0012 Oct 21, 2022

Choose a reason for hiding this comment

junaruga Oct 21, 2022

Choose a reason for hiding this comment

junaruga Oct 21, 2022

Choose a reason for hiding this comment

st0012 Oct 21, 2022

Choose a reason for hiding this comment

junaruga Oct 21, 2022

Choose a reason for hiding this comment

junaruga commented Oct 21, 2022

st0012 commented Oct 21, 2022

peterzhu2118 left a comment

Choose a reason for hiding this comment

junaruga commented Oct 21, 2022 •

edited

Loading