vert-x / mod-lang-groovy

Vert.x 2.x is deprecated - use instead
https://github.com/vert-x3/vertx-lang-groovy
Other
16 stars 15 forks source link

indy mode patch for 2.1.5 #11

Closed swilliams-pivotal closed 10 years ago

swilliams-pivotal commented 11 years ago

Evaluation of Groovy Invoke Dynamic (InDy) mode for lang-groovy.

Seems to reduce the size of stack traces, but have not yet found a good way to conduct a performance comparison.

jkeys089 commented 11 years ago

Also, if the indy version is specified should we add optimizations to build.gradle?

  tasks.withType(GroovyCompile) {
    groovyOptions.optimizationOptions.int = false
    groovyOptions.optimizationOptions.indy = true
  }
LostInBrittany commented 10 years ago

I'm unable to merge manually, the src/main/groovy/org/vertx/groovy/platform/impl/GroovyVerticleFactory.groovy file has changed quite a lot since your pull request.

Could you please do the modif on the current version? I will accept the pull request as soon as you post it!

danthegoodman commented 10 years ago

With the compiler configuration stuff, this can be done using a file compilerConfiguration.groovy:

customizer = { CompilerConfiguration cfg ->
  cfg.optimizationOptions.int = false
  cfg.optimizationOptions.indy = true
  return cfg
}

I vote this or make indy default and make the user do the inverse of this file if they don't want indy.

LostInBrittany commented 10 years ago

@danthegoodman could you please propose a pull request? If it's done this week, I'll include it in 2.1 release :)

danthegoodman commented 10 years ago

@LostInBrittany I haven't played around with the indy stuff, so I don't know the implications of making indy default. Unless someone else thinks it's a good idea, I'd rather not do a pull request. If someone wants to enable indy, they can add that customization file.

jkeys089 commented 10 years ago

I added InDy support based on this patch and it worked OK with a very specific JVM on linux. I ended up disabling it though because several JVM builds have serious InDy issues on different platforms (e.g. OSX) and there was no noticeable performance boost at all for my app.

This recent tweet from Cédric Champeau (one of the core groovy committers) is very telling: https://twitter.com/CedricChampeau/statuses/425281375122771968

LostInBrittany commented 10 years ago

I guess we aren't pushing it now, unless somebody can give a good reason to do it.

Thanks a lot! On Jan 29, 2014 8:14 PM, "jkeys089" notifications@github.com wrote:

I added InDy support based on this patch and it worked OK with a very specific JVM on linux. I ended up disabling it though because several JVM builds have serious InDy issues on different platforms (e.g. OSX) and there was no noticeable performance boost at all for my app.

This recent tweet from Cédric Champeau (one of the core groovy committers) is very telling: https://twitter.com/CedricChampeau/statuses/425281375122771968

Reply to this email directly or view it on GitHubhttps://github.com/vert-x/mod-lang-groovy/pull/11#issuecomment-33619041 .

purplefox commented 10 years ago

Yeah, no compulsion to merge this. If you don't think it's the right thing to do, reject it

LostInBrittany commented 10 years ago

So for the moment we are putting this on the "Let's see later" stack