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

improve load times w/ fixed gemset iteration #87

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

AprilArcus
Copy link

This PR merges @nrser's work to improve load times, as discussed at #84 (comment), with some trivial fixes to an iteration error from my own fork.

nrser and others added 10 commits February 5, 2018 04:05
New version adds ~15ms to `ruby -e 'puts "yo"' time :)

Quite possibly broken for more complex cases, but it's a start.

Also, I renamed the hooks to have their event name ('which', 'exec',
'rehash') in their filename so they're easier to pick out in
`RBENV_DEBUG=1` output.
Where we're at:

    $ time ruby -e 'puts "yo"'
    yo

    real	0m0.173s
    user	0m0.097s
    sys	0m0.034s

Versus with all rbenv-gemset hooks removed:

    $ time ruby -e 'puts "yo"'
    yo

    real	0m0.135s
    user	0m0.078s
    sys	0m0.025s

So added ~40ms with the hooks (rehash isn't used in that run). Not bad.
While I'm sure it was there for a reason - I recall
there being weird and troublesome double
executions going on when I dug into the
rbenv-gemset stuff - it messes up subprocesses
that run one lock exe invoked from inside
another.
@jf
Copy link
Owner

jf commented Jan 6, 2019

@AprilArcus @nrser can we leave out 5a1b480 ? Leave the versioning / scheme to me. I'll bump the version when this is done (and I think 3 numbers is enough already).

@jf
Copy link
Owner

jf commented Jan 6, 2019

re 5a382ef:

New version adds ~15ms to `ruby -e 'puts "yo"' time :)

sorry, "adds"?

Thanks for adding stuff like rbenv_gemset_debug, btw. For trying to understand what the code does - and I think even for the purpose of code review - it is useful. I will eventually have to remove this in the end before it becomes production.

re the use of rbenv_gemset_lib_loaded: I agree with this approach. I took the same approach for something I wanted to apply to rbenv (you can see rbenv/rbenv#507)

@jf
Copy link
Owner

jf commented Jan 6, 2019

@nrser can you explain your comment in 43ca1c1 ? What are "lock executables"? and what is "QB"?

@nrser
Copy link

nrser commented Jan 9, 2019

@AprilArcus @nrser can we leave out 5a1b480 ? Leave the versioning / scheme to me. I'll bump the version when this is done (and I think 3 numbers is enough already).

Yeah for sure, I just did that so I had a different version number for my fork and could tell the difference from a quick rbenv gemset version (think I had both the master and my fork installed alongside when I was doing the initial work).

The commits you're looking at are the ones I made as I was writing it. I thought I would come back and rebase it up nice afterwards, but once I realized I wasn't going to have time for that any time soon I just shared my branch, figuring that given the magnitude of performance pain & gain people might want to use it regardless of the mess and lack of test.

@nrser
Copy link

nrser commented Jan 9, 2019

re 5a382ef:

New version adds ~15ms to `ruby -e 'puts "yo"' time :)

sorry, "adds"?

Oh, that meant that after the changes using Ruby through rbenv/gemsets only added an extra ~15ms spin-up time (compared to just running the system Ruby without rbenv).

Before the changes, the rbenv/gemsets combo was adding around ~700ms over the system installation (testing with simple ruby -e ... commands).

Eventually I think I ended up spending around 40ms for spin-up, having addressed more of the paths and features than I had at the point of 5a382ef

@nrser
Copy link

nrser commented Jan 9, 2019

@nrser can you explain your comment in 43ca1c1 ? What are "lock executables"? and what is "QB"?

Like I said previously, I was just committing as I was working along, thinking it would get rebased for publication. That stuff is just notes to self regarding what was broken on my machine.

The "locks" are this:

https://github.com/nrser/rbenv-lock

lil' rbenv plugin I wrote to "lock" certain Gem executables to specific versions of Ruby.

Like, if you use lunchy or travis or whatever CLI apps and you install them as gems, then you either need to install them in every Ruby version you have in rbenv - in which case they might not even support or work across all of them, and even if they do it makes for a horrible setup and maintenance situation - or they will break when you're using a Ruby or gemset that doesn't have them, either through being in a project directory with a .ruby-version, rbenv shell ... set, etc.

I use Ruby for a lot of personal tools and use a few tools distributed through gems and this annoyed the shit out of me, so I wrote a little plugin to try to fix it.

What the comment's talking about is that QB - a tool I wrote to run Ansible roles against localhost from the CLI - is written in Ruby, and uses rbenv-lock to lock on the the Ruby version it's installed for.

The tricky part is that QB may run tasks that invoke executables that are also written in Ruby, and may even use rbenv-lock, so you can get ENV vars set by rbenv/-gemset/-lock trickling down through descendant processes and screwing up Ruby processes that need a different environment to work.

So exporting RBENV_GEMSET_EXEC_ALREADY was causing problems running child Ruby processes, which totally makes sense.

@nrser
Copy link

nrser commented Jan 9, 2019

Thanks for adding stuff like rbenv_gemset_debug, btw. For trying to understand what the code does - and I think even for the purpose of code review - it is useful. I will eventually have to remove this in the end before it becomes production.

rbenv_gemset_debug() only writes output if the $RBENV_GEMSET_DEBUG ENV var is set, so you shouldn't see any debug output unless you turn that guy on, and it's function call, so it's cheap. Do you want to remove it for cosmetic reasons (just don't like seeing/having the calls in the source)?

I don't really do much Bash (and I'm sure it shows), so I'm not sure how projects usually handle logging... but I know at least some tools have CLI options or ENV vars you can use to turn on debug logging in production releases, and I've had it be useful more than once when diagnosing issues.

I see that rbenv just uses their $RBENV_DEBUG flag to set -x, but, man, I've hated wading through the set -x output for anything more than a few-dozen lines of script. Which is why I wrote those logging functions. If that's how it's done though I can get with it.

@AprilArcus
Copy link
Author

any chance of merging this this year?

@jf
Copy link
Owner

jf commented Mar 8, 2020

Thanks for following up, @AprilArcus . This year? I should think so. I need to find time to review this again.

I thought I stated somewhere the version bump implementation should be up to me... but I don't see it here. If I approve this, I will want to do it without the forced version bump.

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