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

Disable tracepoints after continue #160

Merged

Conversation

@k0kubun
Copy link
Contributor

@k0kubun k0kubun commented Aug 8, 2015

Fixes #144.

I couldn't find out how to avoid SEGV in tests and what a correct fix would be. I'm just sharing my idea.
Byebug.start enables tracepoints and they are left enabled after continue. If tracepoints are enabled, it gets slow. In fact, this branch does not change things slow after continue. I want to disable them while I'm not debugging.

I think we can disable tracepoints after continue or ^D and enable them again when byebug or debugger is called. Is there any reason to keep tracepoints enabled all the time?

@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Aug 8, 2015

@k0kubun Thanks! This is the same idea I shared in #144. I agree with you, there's no reason to leave them enabled.

There's two problems with the patch:

  • The obvious one: it breaks the tests.
  • We cannot disable tracepoints if there are any enabled breakpoints or catchpoints, so an extra check is needed somewhere.

Feel free to further research on those issues. Otherwise, I'll pick it up where you left it when I get to this.

Thanks again!

@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Aug 8, 2015

Thank you for your quick response!

there's no reason to leave them enabled.
We cannot disable tracepoints if there are any enabled breakpoints or catchpoints, so an extra check is needed somewhere.

I created this pull request to hear these points. I'll research further. Thanks.

@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Aug 8, 2015

👍 This will be a killer speed improvement if we get it working! 😃

@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Aug 28, 2015

@k0kubun Did you have the chance to investigate further?

@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Aug 28, 2015

I strongly want this change but currently I still coudn't make time to work on this. I have will to investigate this further.

@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Aug 28, 2015

Thanks! We are in the same situation. :)

@k0kubun k0kubun force-pushed the enable-disable-tracepoints branch 11 times, most recently from 968ebff to faa5894 Oct 10, 2015
@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Oct 10, 2015

We cannot disable tracepoints if there are any enabled breakpoints or catchpoints, so an extra check is needed somewhere.

  • I understood it and fixed so in fc1eb51.

The obvious one: it breaks the tests.

  • I recognized sometimes functions registered to tracepoints were called even if
    they are disabled.
    • So I guarded the calling for disabled tracepoints in 72fff22. It protects us from SEGV.
  • It was hard to remove test failure for stopping Byebug in the case that interface input is nil.
    • But I want to continue by C-d and stopping Byebug. I fixed LocalInterface to use continue when C-d (EOF) is input. It is just a workaround but works fine. faa5894
  • If Byebug is stopped, you can't get Byebug.current_context.
    • I stubbed Byebug.stop if there are no tracepoints or catchpoints. fc1eb51
      • I added an extra test to ensure continue works without stub and it stops Byebug.

@deivid-rodriguez I managed to create working patch and make all tests passing. Are you OK to merge and release this?

@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Oct 12, 2015

Thanks! I'll review this very soon!!

@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Oct 13, 2015

I confirmed that the first implementation worked well, but it seems that current implementation locks threads except current threads. With commit 72fff22, I prevented to release thread locks. Adding 6f96e54 didn't work well too.

I tried delayed stopping in 2a77361 (another branch) and released locks in cleanup(). But it looked like some threads were still locked in WEBrick.

@deivid-rodriguez Do you have any idea to release locks of all threads?

@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Oct 13, 2015

I noticed that some threads are waiting before acquire_lock() and lock after scheduled stop in 2a77361.
I'll fix this patch to avoid unexpected thread locking.

@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Oct 13, 2015

Thanks for your hard work!

@k0kubun k0kubun force-pushed the enable-disable-tracepoints branch 6 times, most recently from 0b498a6 to f2db5d5 Oct 14, 2015
@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Oct 14, 2015

I found a safe way to unlock all threads after Byebug.stop. k0kubun@0696f94
I confirmed that this patch works with my rails app and it didn't cause SEGV or deadlock.

@deivid-rodriguez Could you review this again?

@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Oct 27, 2015

No spring (commented out gem, checked with `ps aux). Updated sample app with latest ruby (2.2.3) and latest rails (4.2.4). No improvement (25ms vs 400ms).

Hmm.. I can't think of any possibility to reproduce the situation (except you enable some breakpoints via break command). But I'm interested in the problem because my colleague also said the same thing.

Again, could you check the return value of Byebug.started?? You can check it by

  def index
    binding.pry
    puts "hello: #{Byebug.started?}"
  end

or something. And there are two ways to exit the REPL of pry-byebug. Which way do you use, Control-D or continue? (Though I supported both situations in deivid-rodriguez/pry-byebug#80.)

@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Oct 27, 2015

I got it. Your log tells that you exit REPL with exit. I'll fix the patch for pry-byebug to support exit... thanks.

@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Oct 27, 2015

@brian-vogogo I optimized pry-byebug for exit or quit too in deivid-rodriguez/pry-byebug@de89e05. Could you try the new revision of byebug-stop branch?

@brian-vogogo
Copy link

@brian-vogogo brian-vogogo commented Oct 27, 2015

Thanks!
exit is fast
quit is fast
Ctrl-D is fast
continue is slow

@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Oct 27, 2015

I'm glad to hear that. :)

continue is slow

Indeed, it was. I overlooked :breakout_nav is thrown before Byebug.stop and it isn't executed...
I fixed it. I think all of them are fast now.

@brian-vogogo
Copy link

@brian-vogogo brian-vogogo commented Oct 27, 2015

All are fast. Thanks so much for working on these PRs!

@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Oct 27, 2015

All are fast. Thanks so much for working on these PRs!

Yes, thanks! 💚

@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Oct 27, 2015

@k0kubun Would you mind rebasing this on top of current master so we can use the latest fixes I've added to master as well?

@k0kubun k0kubun force-pushed the enable-disable-tracepoints branch from fb5ccec to ea8a97f Oct 27, 2015
@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Oct 27, 2015

Done.

@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Oct 28, 2015

Thanks!

@k0kubun k0kubun force-pushed the enable-disable-tracepoints branch from ea8a97f to ae57005 Oct 28, 2015
@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Oct 30, 2015

@k0kubun I've changed the travis build to use your patch to Ruby so this should be ok to merge. I'm not sure whether the segfault could affect normal usage but since you've been using this with success for a while, I'm going to merge it.

Can you rebase one last time and add a CHANGELOG entry?

@k0kubun k0kubun force-pushed the enable-disable-tracepoints branch from ae57005 to 6e56b5f Oct 30, 2015
@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Oct 30, 2015

I've changed the travis build to use your patch to Ruby so this should be ok to merge. I'm not sure whether the segfault could affect normal usage but since you've been using this with success for a while, I'm going to merge it.

Thanks! It's very helpful for us. We didn't see SEGV on our normal usage.

Rebased and added a CHANGELOG entry.

deivid-rodriguez added a commit that referenced this issue Oct 31, 2015
@deivid-rodriguez deivid-rodriguez merged commit 1749e21 into deivid-rodriguez:master Oct 31, 2015
2 checks passed
@k0kubun k0kubun deleted the enable-disable-tracepoints branch Oct 31, 2015
@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Oct 31, 2015

Finally merged this, the appveyor build now segfaults sometimes but I'm happy to have both an stable travis build and a fast debugging feature on master!

I really appreciate the work you did here. Thanks a lot @k0kubun !! ❤️ 💚 💛

@k0kubun
Copy link
Contributor Author

@k0kubun k0kubun commented Oct 31, 2015

It was interesting to investigate this problem for me and I'm very happy to contribute to this awesome project!
Thank you for your helpful comments too. :)

@deivid-rodriguez
Copy link
Owner

@deivid-rodriguez deivid-rodriguez commented Oct 31, 2015

👏 👏 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants