2017-05-16 18:57 GMT+02:00 Sanne Grinovero <sa...@redhat.com>: > Hi Cédric, > > we use Gradle a lot in our team (Hibernate) and all your efforts to > make it faster are highly appreciated. > > I can assure you that during a normal day of work while we might > rebuild a project a few hundred times, I'll rarely have to change > environment variables. > > Obviously I don't represent all users, but as you said yourself the > need to change environment variables is both something you have to > support and something which is extremely rare.
So can't you simply keep the daemon around, as long as environment > variables don't change? > That's one of the suggestions in the original email, but as you can see, it's not uncommon to have environment variables mutated "behind you back", even in a simple command line. Sometimes, it's as easy as having 2 different terminals open (which I think is far from being a rare case): then you cannot share the same daemon instance, which is either very bad for performance or memory usage. Why? Because the open terminal has some specific variables (WINDOWID, ...). > > For the rare case in which environment variables have been mutated, > you could trigger a full restart. > > As a second step, I should mention that on a typical workstation I > might have multiple terminals open, each configured with a different > environment and possibly working on a different project or different > branch - or just testing on a different environment. > It would be even nicer if one could have a different Gradle daemon > instances running for each environment; incidentally this might have > other benefits, like we often need to switch JDK build or even vendor. > I'd be happy to be able to give them some name as identifier. > > That's something we consider, but it's not the same use case at all (my typical setup is pretty much the opposite: a lot of terminals open for the same environment/build), so it has to be supported differently. I don't think we should mix this in the game. > I think this solution would improve Gradle and allow the JDK to move > on with this quite nice cleanup. > > HTH > > Thanks, > Sanne > > > On Tue, May 16, 2017 at 11:05 AM, Cédric Champeau > <cedric.champ...@gmail.com> wrote: > > Thanks Peter for your thoughts, but I don't think it's as simple as that. > > As I explained in my original email, there are multiple ways the > > environment variables can be mutated, and it can be done _externally_. > > Typically, during a task execution, a JNI call performed by a random > native > > tool could mutate the environment. That's something we, as a build tool, > > have to consider as a use case. It's very unlikely but it can happen. > This > > means that for the same classloader, the environment can change. And for > > performance reasons, we cache classloaders between builds, and reuse them > > as much as possible (because classloading is far from being cheap). > > > > 2017-05-14 19:51 GMT+02:00 Peter Levart <peter.lev...@gmail.com>: > > > >> Hi Cedric, > >> > >> Chiming in with my thoughts... > >> > >> It's unfortunate that Gradle plugins or libraries used by plugins use > >> environment variables at all. I wonder who was the first? Did Gradle > >> introduce the feature of passing client environment to the daemon and > >> establishing the view of System.getenv for plugins 1st or did libraries > >> used by plugins happen to use environment variables using System.getenv > and > >> Gradle was just kind enough to provide them a dynamic view controlled by > >> client? > >> > >> In the end it doesn't matter. The fact is that System.getenv is part of > >> standard Java API and anyone using it should be aware that by doing so, > >> they are limiting their software to be (re)configured only by spawning > new > >> process(es). UNIX environment was not designed to be mutated during the > >> course of a long-running process. Maybe just at process startup/setup > when > >> conditions are under control (i.e. a single running thread) but later, > the > >> environment is meant to be read only. > >> > >> Maybe there is a solution for Gradle and othert though. But that > solution, > >> I think, is not in exposing a "live" view of process environment through > >> System.getenv or new methods to "refresh" the view as you are proposing, > >> since that would only encourage people to mutate the process environment > >> which, as was established on this thread, is not safe in a long-running > >> multi-threaded process, which Java processes usually are. Perhaps the > >> solution is in extending the System.getenv API with ways to provide > >> "custom" views of variables for code that runs in a "container". > >> > >> Gradle is, among other things, a container for plugins. Is Gradle > running > >> plugins in a separate ClassLoader? Does it use a separate ClassLoader > for > >> each "build" which might bring with it a new set of environment > variables > >> from the client? In such a setup, one could provide a separate set of > >> environment variables for each ClassLoader, simply by passing them to > the > >> ClassLoader constructor. System.getenv would need to be a > @CallerSensitive > >> method which would return caller's ClassLoader view of variables, if any > >> such view was established, or simply the view inherited from the parent > >> ClassLoader. > >> > >> Such would be a "functional" approach, which would keep environment > >> variables immutable, but allow child "contexts" to have different views > of > >> them. Such approach would also play well with idioms like: > >> > >> class AbcPlugin { > >> static final String SOME_SETTING = System.getenv("SOME_SETTING"); > >> > >> ...and would also help other containers (such as app servers) support > >> existing libraries that use environment variables to be configured > >> differently in different apps deployed in the same VM process. > >> > >> Just a thought. > >> > >> Regards, Peter > >> > >> > >> > >> On 05/11/2017 09:02 AM, Cédric Champeau wrote: > >> > >> Thanks for the answers, folks, but I think they are kind of missing the > >> point. The fact is that environment variables *are* mutable. Java > happens > >> to return an immutable view of the environment variables when the VM was > >> started, which is not the reality. We cannot trust what `System.geteenv` > >> returns. The Javadocs say "current system environment" but it's just not > >> true. The method should be called `getEnvWhenTheVMWasStarted`. > >> > >> We also have the problem that the environment of the client and the > daemon > >> differ over time, as I explained in the previous email. And it is pretty > >> easy to understand that _when the build runs_, we need to get the > >> environment variables of the _client_, not the daemon. Imagine > something as > >> simple as this: > >> > >> String toolPath = System.getenv('SOMETOOL_HOME') > >> > >> and imagine that this code runs in the daemon. When the daemon is > started, > >> there's absolutely no guarantee that this variable is going to exist. > >> However, we know that when we're going to execute the build, this > variable > >> *has* to be defined. Obviously, it's going to be done from the CLI: > >> > >> $ export SOMETOOL_HOME = ... > >> $ ./gradlew run ---> connects to the daemon, passes environment > variables, > >> mutates those of the daemon, which then executes the build > >> > >> Similarly, from a *single* CLI/terminal, it's very common to change the > >> values of environment variables (because, hey, they are mutable!): > >> > >> $ export SOMETOOL_HOME = another path I want to try out > >> $ ./gradlew run --> ... must update env vars again > >> > >> So, to work around the problem that Java doesn't provide a way to mutate > >> the current environment variables, we perform a JNI call to do it. > *Then* > >> we need to mutate the "immutable view" that Java provides, or all Java > code > >> is going to get wrong information, and we would never succeed. Note that > >> having to resort on JNI to mutate the environment is not the problem. We > >> can live with that. Once can think it's a bad idea to allow mutation, in > >> practice, it is necessary, but I reckon it could be a bad idea to have > an > >> API for this. The *real* issue is that `System.getenv` does *not* return > >> the real values, because it's an immutable view of what existed at the > VM > >> startup. > >> > >> Note that it's not recommended to mutate environment variables in a > >> multi-threaded environment. But in practice, you can assimilate what we > do > >> to the VM startup: we get environment variables, mutate them, _then_ the > >> build runs (and only at that moment we would effectively be > >> multi-threaded). We can live with potential issues of mutating the > >> environment: the benefits outperforms the drawbacks by orders of > magnitude. > >> When the daemon is activated, we're not talking about 10% faster builds. > >> They can effectively be 3, 5 or 10x faster! > >> > >> Now, back to the problem with JDK 9: > >> > >> - first, the implementation has changed. But we could adapt our code. > >> - second, calling `setAccessible` is not allowed anymore. And this is a > >> MAJOR pita. The problem is that we're trying to access the java base > >> module, and reflection on that module is not allowed anymore. There are > no > >> good solutions for this: we could write a JVM agent, like you suggested, > >> that rewrites bytecode. But since we're trying to rewrite a core class, > it > >> would have to be found on the invocation of `java` command itself, and > >> cannot be dynamically loaded. In addition, we would have to add a flag > to > >> open core classes to rewrite. There are multiple problems with this > >> approach: first, we don't know on which environment we run before we're > >> started. Gradle can run on JDK 7, 8, 9, ... and we cannot have a startup > >> script which tries to infer the version from whatever it finds, this is > not > >> realistic and would add unacceptable latency on the client side (plus, a > >> lot of headaches to support the various environments we support: Linux, > >> Mac, Windows, ...). Second, we may not have a hand on the CLI at all. > For > >> example, if the build runs in Travis CI, Amazon, or whatever cloudish > thing > >> that spawns its own containers, we cannot tweak the VM arguments. We > just > >> have to use whatever is there. Last, rewriting bytecode has a cost, that > >> you pay at startup. > >> > >> Again, what we need is that Java just returns the live view of the > >> environment variables. Allowing *mutation* is not necessary, technically > >> speaking, because we can rely on JNI. However, I reckon that returning > an > >> immutable view is done for performance reasons. That's why adding a way > to > >> mutate "the view" would be an acceptable thing I think. No reflection, > no > >> bytecode rewriting, just give a way to mutate the map that > >> `ProcessEnvironment` retains. The advantage of this is that you would > not > >> effectively mutate the environment variables, so you, on the JVM side, > >> would be safe. What you would mutate is the view you are retaining. > >> Alternatively, provide us with an API to "refresh" the view. Like, > >> `System.getenv(true)`. The benefit would be that you would only have to > get > >> the new view of environment variables if the user asks for it. And, > >> subsequent callers would get the refreshed view, so people using > `gettenv` > >> in their code would be up-to-date. > >> > >> I'm really concerned that this problem is not taken seriously. It's a > major > >> performance problem for the most widely used build tool out there > >> (considering all users, from native to Java and Android). Just saying > >> "you're doing something nasty" is not a valid answer: we have to do it, > and > >> do it for good reasons. > >> > >> > >> 2017-05-11 4:50 GMT+02:00 Martin Buchholz <marti...@google.com> < > marti...@google.com>: > >> > >> > >> Since you're OK with doing brain surgery on the JDK and you control the > >> entire process, consider controlling your daemon with a bytecode > rewriting > >> agent (e.g. byteman) that changes the definition of System.getenv etc. > >> > >> (Whose side are you on Martin?! ... I confess I also wish for a faster > >> gradle ...) > >> > >> > >> > >> >