tinkerpop-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stephen Mallette <spmalle...@gmail.com>
Subject Re: [DISCUSS] groovy-sql
Date Mon, 24 Sep 2018 23:35:12 GMT
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