Jochen, of course, I don't want this to be accepted as soon as
possible. Quite the contrary, as this is a big change, it'd better be
reviewed thoroughfully and tested. Get well!
When is 2.4 planned? What would the target release be otherwise, if
the change was less intrusive? And why is the lock field being final
such a big deal? Because it's a part of an API that somebody might
reassign?
As for volatile, I've almost never seen its reads being a major
performance issue. If you knew how many of them IDEA performs every
microsecond during very multithreaded highlighting... But the
performance issues are typically caused by inefficient algorithms, or
too large memory usage, or too long locking, not by volatile reads per
se.
The fact that ClassNode is lazily initialized under a lock suggests
that it can be used in a multithreaded environment (although other
places in ClassNode seem to ignore that). If the variable in
double-checked locking is not volatile, then it opens a door to
heisenbugs and overall language instability. So I'd say one has to
decide here: either Groovy is supposed to be compiled and run in
multithreaded environment, or not. In the first case volatile is
needed, in the second case, synchronized is not needed.
The original idea was to replace the default resolver with a more specific one, in case the environment needs that. Since you yourself already said, that this should maybe be limited to commandline compilation, it would probably make sense for those to set a ClassNodeResolver subclass instead. Do you agree here?
Writing a separate non-invasive resolver was my original approach,
too. Then I realized that I needed to change ClassNode and Java5 as
well and decided that it's better for the overall resulting code to
stay compact. A subclass can certainly be created. I have some
aesthetical issues with it. First, I don't like subclassing with
calling supers, as they tend to make the code flow less obvious.
Second, tryAsScript would still needed to be called from the subclass,
so it'd be promoted to protected and become a part of API, and I like
less API more than more API :) And finally, all the clients that need
ASM-resolving probably would now have to write some code instead of
just specifying an optimization options.
Then the part with the asmResolving and classLoaderResolving flags, look a bit strange.. first of all, doing it again and again is not good for performance.
Checking optimization options is only performed in an attempt to find
a class that's not already loaded, which should be dominated by the
cost of this class loading by a very large extent. Of course, these
flags could be cached somehow, I just don't see an easy way to do so,
given that this cache might also need clearing on option change.
But also why does asmResolving==true not imply classLoaderResolving==false?
A tough question where I don't have a strong opinion. ASM resolving
seems faster (at least in the profiler) than classloading-based one,
so it might be benefitial even in a dynamic world, where most classes
are anyway taken from the class files, and only some of them are
created dynamically.
Besides, when it's turned on by default, then all groovy-core tests
have it. And when these thousands of tests pass with ASM resolving,
I'm more convinced that it actually works, than if it were only its 23
unit tests. Maybe there should be a separate build configuration where
it's on, I don't know. On the other hand, turning such a new and
untested feature by default might be dangerous. On the third hand, if
it isn't turned on, then nobody will test it :) So I'm not sure.
What's your opinion?
Peter, thanks for the pull request!
I hope you can wait till Monday, the pull request is quite big, so a review
will take a bit time. Also I am unable to concentrate due to a cold these
days.
Of course I already took a look at it and maybe we can already discuss a few
points here right now...
I know that making the lock final is better and absolutely more correct...
but such a change will limit the pull request most probably to 2.4 only. If
that is ok with you, then no problem. Another thing is the volatile on
boolean. I know double checked locking and such... but the JVM reacts really
really bad to memory barriers of any kind, so I try to avoid them, if they
are not absolutely needed. And in this it is not. If it where not a
primitive, then things would be different.
The original idea was to replace the default resolver with a more specific
one, in case the environment needs that. Since you yourself already said,
that this should maybe be limited to commandline compilation, it would
probably make sense for those to set a ClassNodeResolver subclass instead.
Do you agree here?
Then the part with the asmResolving and classLoaderResolving flags, look a
bit strange.. first of all, doing it again and again is not good for
performance. But also why does asmResolving==true not imply
classLoaderResolving==false?
as for the failures on JDK6... not sure... looks like related to inner
classes. An endless loop, when searching for the outer class.
more later
bye blackdrag
Post by Peter Gromovhttps://github.com/groovy/groovy-core/pull/552
Post by Peter GromovToo bad :(
Anyway, I've started working on asm resolve. I have something very
initial that doesn't account for generics and inner classes and isn't
really incorporated into resolve yet, but if I'm going in a completely
wrong directrion (or if my coding style isn't acceptable in
groovy-core), I'd be glad to hear that :)
https://github.com/donnerpeter/groovy-core/commit/accb2944a808cb5f043057daec54baa5c00b6626
BTW do you have smoke tests that run a full-blown Groovy compiler
(like it happens from command line) and check it works?
Peter
Post by Guillaume LaforgeSpeaking of joint compilation, Jochen tried an approach recently with the
http://blackdragsview.blogspot.fr/2014/11/a-joint-compiler-for-groovy-and-java.html
Javac is indeed the other plausible approach. Which also made me wonder how
stable this API is across versions of JDK? (ie. doesn't mean we'd need
different joint compilers for different JDK versions?)
Avoiding class loading (using ASM-class reading) and avoiding stub
generation are really two things we're looking forward for Groovy.
Thanks for giving a hand!
Guillaume
Post by Peter GromovThanks!
Jochen, indeed, I can try to use ClassNodeResolver to speed up
resolve. Which doesn't mean that I won't create a pull request anymore
since I'll anyway have to write some asm-reading code, so why not
share it.
I'll have to think more about avoiding stub compilation. It's
complicated, because we have to tell Javac API how to resolve Groovy
code that are used from Java.
Peter
Post by Jochen TheodorouHi Peter,
to add on that... I added something called ClassNodeResolver to
CompilationUnit, it can provide a ClassNode for a class lookup as well as do
caching and especially this can be used to interface with IDE internals
if
wanted and if direct use of CompilationUnit is possible. With that, you
could for example let the Groovy compiler compile against structures of
the
IDE and thus avoid even stub compilation
bye blackdrag
Post by Cédric ChampeauHi Peter!
There's a lot of interest in that. It's been in the todo list for long
but nobody in the core team had time to work on it,
so a PR would be very appreciated!
Hi,
Tired of slow Groovy compilation times in IntelliJ, I've decidede
there's time to do something about it :) Much slowness (and some
additional issues as well) is caused by groovyc using class loaders to
resolve classes during compilation.
I wonder if there's any work being done on removing that restriction,
so that the classes are loaded using just ASM. If not, and if there's
interest, I could try to devise a patch/pull request myself.
Peter Gromov
JetBrains
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email
--
Jochen "blackdrag" Theodorou - Groovy Project Tech Lead
blog: http://blackdragsview.blogspot.com/
german groovy discussion newsgroup: de.comp.lang.misc
For Groovy programming sources visit http://groovy-lang.org
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email
--
Guillaume Laforge
Groovy Project Manager
Pivotal, Inc.
Blog: http://glaforge.appspot.com/
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email
--
Jochen "blackdrag" Theodorou - Groovy Project Tech Lead
blog: http://blackdragsview.blogspot.com/
german groovy discussion newsgroup: de.comp.lang.misc
For Groovy programming sources visit http://groovy-lang.org
---------------------------------------------------------------------
http://xircles.codehaus.org/manage_email
---------------------------------------------------------------------
To unsubscribe from this list, please visit:
http://xircles.codehaus.org/manage_email