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

Refactor env path handling and fix some bugs related to it #1907

Merged
merged 5 commits into from
Oct 15, 2018

Conversation

ferd
Copy link
Collaborator

@ferd ferd commented Oct 7, 2018

This updates and replaces how most of the path handling in rebar3 is being done. Instead of adding paths here and there mostly at random, this attempts to take a more systematic approach of tracking currently-used modules and replacing them explicitly, while caring for plugins or deps to be in proper order.

Implements the fixes proposed at #1899

Also fixes a path bug in the compiler.

@ferd ferd force-pushed the refactor-env-paths branch 2 times, most recently from 62eb63d to 9e06e18 Compare October 8, 2018 23:53
@ferd ferd changed the title WIP: Refactor env paths Refactor env path handling and fix some bugs related to it Oct 8, 2018
@ferd
Copy link
Collaborator Author

ferd commented Oct 8, 2018

Cleaned up commits in a more logical unit. Probably ready to merge after reviews and confirmation it does not break things (and possibly fixes them!) for @tothlac and @eproxus since you both are probably our biggest plugin users.

I tried it on my own rebar3 proper and found no problems.

Probably worth taking some time to do profiling as well.

@ferd
Copy link
Collaborator Author

ferd commented Oct 9, 2018

A quick check on run times for the test suite reveals that this patch set is almost 40% slower than what we had before.

There's obviously a need to rework the stuff taking place in this PR so that it is faster, or gets similar paths set at a lower runtime cost.

ferd added 4 commits October 11, 2018 08:43
Move path management out of rebar_utils manual code path function
handling (which we leave there for backwards compat), and centralize
them to allow easier coordination of paths between plugins and deps.

On top of path handling, do a check of loaded modules to only purge and
reload those that actually need it done in order to prevent all kinds of
weird interaction and accidental purge kills. It also allows the
possible cohabitation of both at once, with a "in case of conflict pick
X" as a policy

Changing path handling in providers also highlighted a bunch of bugs in
some tests and appears to fix some in other providers, specifically
around plugins.
Also handle some formatting
Some finishing touch to that code
- Only set paths that need to be put as a priority
- Clean up paths before leaving API mode

The first point accounted for some performance cost, but the latter one
explains the 40% overhead in test runs: since rebar3 calls rebar3 a lot
with a bunch of fake apps, and that the new mechanism for path handling
by default does not _remove_ paths, it just _orders_ them, we would end
up in a situation where as the tests ran, more and more fake paths would
get added to the VM.

By the time the run was over, all path handling would take longer since
more paths needed filtering every time. By resetting paths at the end of
an API run, we prevent a given 'project' from polluting another one's
runtime and performance once the API successfully returns.
@ferd ferd force-pushed the refactor-env-paths branch from b2e4bcb to dada4e3 Compare October 11, 2018 12:45
@ferd
Copy link
Collaborator Author

ferd commented Oct 11, 2018

I found the source of the performance regression and fixed it:

  • Only set paths that need to be put as a priority
  • Clean up paths before leaving API mode

The first point accounted for some performance cost, but nothing major.
The second point explains the 40% overhead in test runs: since tests call
a lot with a bunch of fake apps, and that the new mechanism for path handling
by default does not remove paths, it just orders them, we would end
up in a situation where as the tests ran, more and more fake paths would
get added to the VM.

By the time the run was over, all path handling would take longer since
more paths needed filtering every time. By resetting paths at the end of
an API run, we prevent a given 'project' from polluting another one's
runtime and performance once the API successfully returns.

This likely makes rebar3 a bit more usable as an API, and such path leak
wouldn't impact normal projects in any significant way since they tend to
remain with one set of plugins and deps for their lifetime that won't change
too much.

@ferd ferd requested a review from tsloughter October 14, 2018 15:16
@eproxus
Copy link
Contributor

eproxus commented Oct 15, 2018

Works fine with the GRiSP plug-in:

$ rebar3 --version
rebar 3.7.0-rc1+build.4159.reffb6de6e0 on Erlang/OTP 21 Erts 10.0.8
$ rebar3 grisp deploy -n robot -v 0.1.0
===> Compiling mapz
===> Compiling bbmustache
===> Compiling grisp_tools
===> Compiling rebar3_grisp
===> Verifying dependencies...
===> Compiling robot
* Using custom OTP (cf4ef706)
===> Starting relx build process ...
===> Resolving OTP Applications from directories:
          /Users/alind/Stritzinger/Code/GRiSP/robot/_build/grisp/lib
          /Users/alind/Stritzinger/Code/GRiSP/robot/_checkouts
          /Users/alind/Stritzinger/Code/GRiSP/robot/_grisp/otp/21.0/install
          /Users/alind/Stritzinger/Code/GRiSP/robot/_build/grisp/rel
===> Resolved robot-0.1.0
===> Including Erts from /Users/alind/Stritzinger/Code/GRiSP/robot/_grisp/otp/21.0/install
===> release successfully created!
* Running pre_script
* Copying files...
* Copying release...
* Running post_script
foobar
===> Deployment of robot-0.1.0 complete

Also runs fine with _build and ~/.cache/rebar3/plugins removed to start from a clean slate.

@ferd
Copy link
Collaborator Author

ferd commented Oct 15, 2018

Got the approval from @tothlac over Slack as well. Merging!

@ferd ferd merged commit 7bfc811 into erlang:master Oct 15, 2018
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