Ok - fair enough. I will just issue the PR for TINKERPOP-2039 (Groovy 2.5.2 bump) on master. You can then issue PRs as needed on appropriate branches for the indy/no-indy stuff on a separate ticket. Good?
On Mon, Sep 24, 2018 at 6:15 PM Robert Dale <[email protected]> wrote: > On Mon, Sep 24, 2018 at 8:42 AM Stephen Mallette <[email protected]> > wrote: > > > > I think we should go the simpler, non-indy route > > > > meaning just remove the indy classifier from what we have now and leave > the > > dependencies as-is with version 2.5.2 - yes? > > > > > rather than messing with classifiers and exclusions unless someone can > > provide some quantitative performance benchmark. > > > > given that we have used indy for so long, i almost feel the opposite. > like, > > we should benchmark to ensure a switch provides equivalent performance, > no? > > > > Are we using it? > > For core groovy, only 5% of the codebase in written in groovy. Since both > non-indy and -indy are included, the use of indy here is determined by > which appears first in the classpath. In console and server, the classpath > is built by 'echo lib/*.jar'. The sort order is dictated by the shell and > locale. So there are no guarantees which comes first. On my system, it > just so happens that the sort order does put the -indy jar first. I'm not > sure what happens on Windows. Then what of third-party, repackaged > distributions? > > Neither jsr223 nor json have any groovy at all so the inclusion of -indy > for them is inconsequential. > > groovysh-indy is limited to groovy console functions only. > > In my pursuit to find an existing benchmark, I found nothing. I did find > several questions on stackoverflow and issues in jira with micro-benchmarks > complaining about how non-indy outperforms indy. It appears that there is > a narrow range of conditions for invokedynamic to work otherwise it falls > back to call site. So it's not simply 'include this and all will be > faster'. With that being the case, I'm not sure any benchmark we would > come up with would be so comprehensive that it would conclusively point one > way or the other. Also keep in mind that we would only be benchmarking > groovy itself since Gremlin scripts are not compiled with invokedynamic and > Java to Java calls, e.g. Gremlin, are out of scope. > > My primary concern is having a good, correct classpath. My secondary > concern is maintenance. I don't believe having indy adds any significant > or reliable performance. But I also don't believe it's worth trying to > benchmark. So if you want to keep indy, fine. But then we have to add the > rat's nest of exclusions. > > I think this should go back to tp32 because it’s a bug that conflicting > deps are included. > > > > > > > > > > On Wed, Sep 19, 2018 at 2:30 PM Robert Dale <[email protected]> wrote: > > > > > As far as I can tell, 2.5 is in the same boat as 2.4 where we would > have > > to > > > specifically include all 'indy' dependencies while excluding the > non-indy > > > deps of those deps. And because there is no groovy-all jar any more, > > it's > > > no longer as easy as including -all indy. > > > > > > I couldn't find much on using 'indy' with maven. That is, no hints, > > > questions, or complaints about conflicts with including indy and > non-indy > > > jars in the same project. I looked at some popular projects - spring, > > > logback, etc - and none use indy. And maybe that's for java version > > > compatibility more than anything else. I don't know. So those that are > > > using indy, like us, may not be aware that both jars are included and > is > > > unhealthy. > > > > > > Why do we need invokedymanic anyway? > > > > > > Gremlin groovy compiler does not set the invokedymanic flag, so gremlin > > > scripts are not benefiting from it directly there. The Gremlin Console > > also > > > does not enable indy. To be honest, I'm not even aware of the actual > use > > > cases of where invokedymanic kicks in. So it's hard for me to say what, > > if > > > any, benefit it would provide with Gremlin scripts. > > > > > > Within groovy itself, indy only applies to those parts of groovy which > > are > > > actually written in groovy and not java. There is only a small > percentage > > > of groovy written in groovy. Of the dependencies we use, 'src/main/' > of > > > each module: > > > > > > groovy: > > > java: 797 > > > groovy: 43 > > > subprojects/groovy-ant > > > java: 15 > > > groovy: 1 > > > subprojects/groovy-json > > > java: 45 > > > groovy: 0 > > > subprojects/groovy-jsr223 > > > java: 5 > > > groovy: 0 > > > subprojects/groovy-groovysh > > > java: 0 > > > groovy: 61 > > > > > > One module that's written entirely in groovy is groovysh. AFAIK, the > > only > > > place it's used is in Gremlin Console which does not enable indy > (neither > > > does groovy console by default). It would seem there is little > > performance > > > benefit here because how much work does the console actually do? We're > > > talking about the core functions of the console separate from running > > > scripts and compiling scripts (which obviously comes from the compiler > > > which, again, does not enable indy by default). > > > > > > I think we should go the simpler, non-indy route rather than messing > with > > > classifiers and exclusions unless someone can provide some quantitative > > > performance benchmark. If there are complaints after the fact - dude, > > > 3.4.0 sucks cuz my scripts run so slow - then we can always back it > out > > > and go the indy route. > > > > > > Robert Dale > > > > > > > > > On Wed, Sep 19, 2018 at 10:55 AM Stephen Mallette < > [email protected]> > > > wrote: > > > > > > > no idea what "UNCHECKED" in the subject header means........something > > > > apache mailing list put there................... > > > > > > > > On Wed, Sep 19, 2018 at 10:49 AM Stephen Mallette < > > [email protected]> > > > > wrote: > > > > > > > > > Created this: > > > > > > > > > > https://issues.apache.org/jira/browse/TINKERPOP-2039 > > > > > > > > > > On Tue, Sep 18, 2018 at 7:48 PM Robert Dale <[email protected]> > > wrote: > > > > > > > > > >> Sorry, was skimming and only saw the 3.0 mention. The default is > in > > > > 3.0. > > > > >> I > > > > >> thought I read that was new there. I don’t know what’s in 2.5. > > I’ll > > > > try > > > > >> to take a look this week. > > > > >> > > > > >> On Tue, Sep 18, 2018 at 19:20 Stephen Mallette < > > [email protected]> > > > > >> wrote: > > > > >> > > > > >> > ah - i tried to find some specific mention of that and didn't > come > > > > >> across > > > > >> > it. if that's the case, then i think that maybe we should just > > head > > > > >> > straight to 2.5.2 on master. would you still go to groovy-all if > > we > > > > did > > > > >> > that? i presume the indy problem would go away in that case then > > > > right? > > > > >> > > > > > >> > i do think we should just do this all at once as part of one PR > - > > i > > > > have > > > > >> > 2.5.2. building now with some minor required changes to get it > > > > working. > > > > >> i > > > > >> > can push that to a branch and we can go from there.... > > > > >> > > > > > >> > On Tue, Sep 18, 2018 at 7:16 PM Robert Dale <[email protected]> > > > > wrote: > > > > >> > > > > > >> > > Indy is gone because invokedynamic is the default in that > > version. > > > > >> > > > > > > >> > > On Tue, Sep 18, 2018 at 18:30 Stephen Mallette < > > > > [email protected]> > > > > >> > > wrote: > > > > >> > > > > > > >> > > > Looking ahead a bit on this one (I'm interested in what it > > will > > > > >> take to > > > > >> > > get > > > > >> > > > to Groovy 3.0.0) I noticed that 2.5.x requires > > <type>pom</type> > > > > and > > > > >> > > > that groovy-all-x.y.z.jar is no longer available "In order > to > > > > cater > > > > >> to > > > > >> > > the > > > > >> > > > module system of Java 9+" - i also don't see an "indy" > > > classifier > > > > >> for > > > > >> > > that > > > > >> > > > in central: > > > > >> > > > > > > > >> > > > > > > > >> > > https://repo1.maven.org/maven2/org/codehaus/groovy/groovy-all/2.5.2/ > > > > >> > > > > > > > >> > > > atm, i'm not sure if that changes anything in terms of the > > > > >> direction we > > > > >> > > > were headed. > > > > >> > > > > > > > >> > > > On Mon, Sep 17, 2018 at 3:06 PM Stephen Mallette < > > > > >> [email protected] > > > > >> > > > > > > >> > > > wrote: > > > > >> > > > > > > > >> > > > > jeez - hornets nest > > > > >> > > > > > > > > >> > > > > interesting that going to a single module makes our > > > distribution > > > > >> > > smaller. > > > > >> > > > > I assume that we must have though the opposite when we > > started > > > > >> using > > > > >> > > > pieces > > > > >> > > > > of groovy rather than "-all". at this point there > > shouldn't > > > be > > > > >> any > > > > >> > > more > > > > >> > > > > weirdly licensed files in groovy given that they have been > > > > >> releasing > > > > >> > > > under > > > > >> > > > > apache for as long as we have. i can't really come up > with a > > > > >> reason > > > > >> > not > > > > >> > > > to > > > > >> > > > > go to "-all" on 3.4.0, so i guess i'm +1 for that. > > > > >> > > > > > > > > >> > > > > the :install command could support classifiers because > > > grape/ivy > > > > >> > > support > > > > >> > > > > it as an option. we could treat that as a separate issue i > > > > guess - > > > > >> > new > > > > >> > > > JIRA? > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > On Sun, Sep 16, 2018 at 6:00 PM Robert Dale < > > > [email protected]> > > > > >> > wrote: > > > > >> > > > > > > > > >> > > > >> So, I was taking a closer look at this there's > potentially > > a > > > > >> larger > > > > >> > > mess > > > > >> > > > >> we > > > > >> > > > >> want to address. > > > > >> > > > >> > > > > >> > > > >> TLDR: instead of removing groovy-sql, replace all > groovy > > > deps > > > > >> with > > > > >> > > > >> groovy-all-indy. > > > > >> > > > >> > > > > >> > > > >> First, a bit of history. tp32 did actually use > > > groovy.sql.Sql > > > > >> > > > >> in ConsoleImportCustomizerProvider. This class was > > deprecated > > > > in > > > > >> > > 3.2.4. > > > > >> > > > >> Later, it was removed in 3.3.0. > > > > >> > > > >> > > > > >> > > > >> With the plan being to remove groovy-sql completely in > > > 3.4.0, I > > > > >> was > > > > >> > > > >> testing > > > > >> > > > >> my instructions on using the :install feature to > reinstall > > it > > > > >> just > > > > >> > in > > > > >> > > > case > > > > >> > > > >> had someone depended on it. It of course downloaded > > > groovy-sql > > > > >> but > > > > >> > it > > > > >> > > > was > > > > >> > > > >> different than what was in lib/. It was missing '-indy'. > > It > > > > >> also > > > > >> > > > >> downloaded the non-indy version of groovy. I had to go > > learn > > > > >> what > > > > >> > > > '-indy' > > > > >> > > > >> was about. For those who don't know '-indy' is a maven > > > > >> classifier to > > > > >> > > > >> denote > > > > >> > > > >> that the jar was built with Java 1.7 'invokedymanic' > > support > > > as > > > > >> > > opposed > > > > >> > > > >> the > > > > >> > > > >> legacy 'call site'. The short of it is that > invokedymanic > > is > > > > >> more > > > > >> > > > >> performant than call site. > > > > >> > > > >> > > > > >> > > > >> As I was comparing groovy lib/, I noticed that console > and > > > > server > > > > >> > > > already > > > > >> > > > >> include both groovy-2.4.15-indy.jar and > groovy-2.4.15.jar. > > > > >> However, > > > > >> > > the > > > > >> > > > >> docs[1] state that only one or the other should be > > included. > > > > Same > > > > >> > goes > > > > >> > > > for > > > > >> > > > >> any 'indy' and 'non-indy' jar of the same module. > Luckily, > > > we > > > > >> don't > > > > >> > > > have > > > > >> > > > >> any other conflict like that although there is a mix of > > other > > > > >> indy > > > > >> > > > >> non-indy > > > > >> > > > >> modules. > > > > >> > > > >> > > > > >> > > > >> I tried again and was unable to download the indy version > > of > > > > >> > > groovy-sql > > > > >> > > > >> because the :install command does not allow a > 'classifier' > > > > >> > parameter. > > > > >> > > It > > > > >> > > > >> will only download the non-indy verison of modules. Even > > if > > > it > > > > >> > could > > > > >> > > > >> download the 'indy' version, the groovy dependencies are > > > built > > > > >> such > > > > >> > > that > > > > >> > > > >> all 'indy' modules have a dependency on non-indy modules. > > > > >> > > > >> > > > > >> > > > >> So, there are several issues: > > > > >> > > > >> 1) there is included a conflicting mix of indy and > non-indy > > > > >> builds > > > > >> > of > > > > >> > > > the > > > > >> > > > >> same module - groovy. This is bad and should be fixed. > > > > >> > > > >> 2) there is included a mix of indy and non-indy builds of > > > > >> different > > > > >> > > > >> modules. This is not necessarily bad but does mean we're > > > using > > > > >> > > > potentially > > > > >> > > > >> less performant pieces of groovy. > > > > >> > > > >> indy) groovysh, json, jsr223 > > > > >> > > > >> non-indy) console, swing, templates, xml > > > > >> > > > >> 3) Install command doesn't support classifier parameter. > > I'm > > > > not > > > > >> > > aware > > > > >> > > > of > > > > >> > > > >> anyone complaining about this before so it's not > > necessarily > > > an > > > > >> > issue > > > > >> > > in > > > > >> > > > >> itself. However, if we were to desire that all groovy > > modules > > > > be > > > > >> > > > >> indy-only, > > > > >> > > > >> then classifier support would be required. But then I > > don't > > > > know > > > > >> > how > > > > >> > > to > > > > >> > > > >> prevent the non-indy dependencies of that module from > being > > > > >> > > downloaded. > > > > >> > > > It > > > > >> > > > >> seems like a mix would always occur. > > > > >> > > > >> > > > > >> > > > >> There is one simple solution interestingly enough: > depend > > > only > > > > >> on > > > > >> > > > >> groovy-all-indy. Not only does this keep all indy > modules > > > > >> > consistent > > > > >> > > > and > > > > >> > > > >> prevent non-indy conflicts/duplicates, it actually > shrinks > > > the > > > > >> > > > >> distribution > > > > >> > > > >> size by 4M. This is from not distributing the duplicate > > > > non-indy > > > > >> > > > version > > > > >> > > > >> of groovy and because groovy-all isn't much bigger than > the > > > > >> separate > > > > >> > > > >> modules we have now. The other benefit is that we can > > reduce > > > > >> groovy > > > > >> > > > >> dependencies down to a single module across the project. > > The > > > > >> > > > alternative > > > > >> > > > >> is ugly - having a direct dependency on each module in > the > > > > >> > dependency > > > > >> > > > tree > > > > >> > > > >> and having an excludes on each of those dependencies to > > > prevent > > > > >> them > > > > >> > > > from > > > > >> > > > >> loading the non-indy deps. > > > > >> > > > >> > > > > >> > > > >> In conclusion, instead of removing groovy-sql, I am > > proposing > > > > >> that > > > > >> > > > instead > > > > >> > > > >> we replace all groovy deps with groovy-all-indy. > > > > >> > > > >> > > > > >> > > > >> Thoughts? > > > > >> > > > >> > > > > >> > > > >> Stephen, are the groovy scripts submitted to Gremlin > Server > > > > >> compiled > > > > >> > > > with > > > > >> > > > >> invokedynamic? What about Gremlin Console? I know the > > groovy > > > > >> > console > > > > >> > > > does > > > > >> > > > >> not have this enabled by default. > > > > >> > > > >> > > > > >> > > > >> 1. > > > > >> > > > >> > > > > >> > > > >> > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > > > > > > http://docs.groovy-lang.org/docs/groovy-2.4.15/html/documentation/invokedynamic-support.html#_two_jars > > > > >> > > > >> > > > > >> > > > >> Robert Dale > > > > >> > > > >> > > > > >> > > > >> > > > > >> > > > >> On Fri, Sep 14, 2018 at 9:32 AM Robert Dale < > > > [email protected] > > > > > > > > > >> > > wrote: > > > > >> > > > >> > > > > >> > > > >> > No, nothing in the docs. There are some references to > > > > >> > > > java.sql.Timestamp > > > > >> > > > >> > but only within the context of gryo. I'll just make an > > > > upgrade > > > > >> > note > > > > >> > > > >> that > > > > >> > > > >> > it was removed and if required, it can be installed > > through > > > > the > > > > >> > > > :install > > > > >> > > > >> > command. > > > > >> > > > >> > > > > > >> > > > >> > Robert Dale > > > > >> > > > >> > > > > > >> > > > >> > > > > > >> > > > >> > On Fri, Sep 14, 2018 at 6:34 AM Stephen Mallette < > > > > >> > > > [email protected]> > > > > >> > > > >> > wrote: > > > > >> > > > >> > > > > > >> > > > >> >> I think groovy-sql was there because there was a time > > > when I > > > > >> was > > > > >> > > into > > > > >> > > > >> >> polyglot data transforms in Gremlin and groovy-sql > > enabled > > > > >> you to > > > > >> > > > >> connect > > > > >> > > > >> >> to JDBC data sources in a nice way. So, I included > > > > groovy-sql > > > > >> as > > > > >> > a > > > > >> > > > >> >> convenience. I'm not against removing it, though I > think > > > we > > > > >> > should > > > > >> > > > >> remove > > > > >> > > > >> >> it in 3.4.0 only in case someone is currently > depending > > on > > > > it > > > > >> > > > >> indirectly. > > > > >> > > > >> >> Please try to double check that we aren't using > > groovy-sql > > > > in > > > > >> any > > > > >> > > of > > > > >> > > > >> the > > > > >> > > > >> >> documentation for some odd example or something. > > > > >> > > > >> >> > > > > >> > > > >> >> On Thu, Sep 13, 2018 at 5:20 PM Robert Dale < > > > > >> [email protected]> > > > > >> > > > wrote: > > > > >> > > > >> >> > > > > >> > > > >> >> > gremlin-driver has a direct dependency on > > groovy-sql. I > > > > >> > removed > > > > >> > > it > > > > >> > > > >> in > > > > >> > > > >> >> > master and all tests pass (docker/build.sh -i -t -n) > > so > > > > >> there > > > > >> > > does > > > > >> > > > >> not > > > > >> > > > >> >> > appear to be any direct or indirect usage. > > > > >> > > > >> >> > > > > > >> > > > >> >> > Why is it there? Looks like it was pulled in because > > of > > > > >> > > > >> >> > https://issues.apache.org/jira/browse/TINKERPOP-713 > > > > >> > > > >> >> > However, groovy-sql does not appear to be a > dependency > > > of > > > > >> > > > >> >> groovy-console or > > > > >> > > > >> >> > anything groovy (except groovy-all). Wasn't then, > > isn't > > > > now. > > > > >> > > > >> >> > > > > > >> > > > >> >> > Any objections to removing it? > > > > >> > > > >> >> > > > > > >> > > > >> >> > -- > > > > >> > > > >> >> > Robert Dale > > > > >> > > > >> >> > > > > > >> > > > >> >> > > > > >> > > > >> > > > > > >> > > > >> > > > > >> > > > > > > > > >> > > > > > > > >> > > -- > > > > >> > > Robert Dale > > > > >> > > > > > > >> > > > > > >> -- > > > > >> Robert Dale > > > > >> > > > > > > > > > > > > > > > -- > Robert Dale >
