Created https://issues.apache.org/jira/browse/TINKERPOP-2045
Robert Dale On Mon, Sep 24, 2018 at 7:35 PM Stephen Mallette <[email protected]> wrote: > 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 > > >
