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
