tinkerpop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Robert Dale <robd...@gmail.com>
Subject Re: [DISCUSS] groovy-sql
Date Tue, 25 Sep 2018 11:05:11 GMT
Created https://issues.apache.org/jira/browse/TINKERPOP-2045

Robert Dale


On Mon, Sep 24, 2018 at 7:35 PM Stephen Mallette <spmallette@gmail.com>
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 <robdale@gmail.com> wrote:
>
> > On Mon, Sep 24, 2018 at 8:42 AM Stephen Mallette <spmallette@gmail.com>
> > 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 <robdale@gmail.com> 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 <
> > spmallette@gmail.com>
> > > > 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 <
> > > spmallette@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Created this:
> > > > > >
> > > > > > https://issues.apache.org/jira/browse/TINKERPOP-2039
> > > > > >
> > > > > > On Tue, Sep 18, 2018 at 7:48 PM Robert Dale <robdale@gmail.com>
> > > 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 <
> > > spmallette@gmail.com>
> > > > > >> 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 <
> robdale@gmail.com>
> > > > > wrote:
> > > > > >> >
> > > > > >> > > Indy is gone because invokedynamic is the default
in that
> > > version.
> > > > > >> > >
> > > > > >> > > On Tue, Sep 18, 2018 at 18:30 Stephen Mallette
<
> > > > > spmallette@gmail.com>
> > > > > >> > > 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
<
> > > > > >> spmallette@gmail.com
> > > > > >> > >
> > > > > >> > > > 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 <
> > > > robdale@gmail.com>
> > > > > >> > 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 <
> > > > robdale@gmail.com
> > > > > >
> > > > > >> > > 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 <
> > > > > >> > > > spmallette@gmail.com>
> > > > > >> > > > >> > 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 <
> > > > > >> robdale@gmail.com>
> > > > > >> > > > 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
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message