Am 10.12.2011 14:04, schrieb Frans van Buul:

Hi all,
I've been working on ScriptingEngine issue
http://jira.codehaus.org/browse/GROOVY-4975 over the last week, since
this issue is bothering me in a Groovy application we've built (a
process engine designed to replace Oracle BPEL, with processes defined
in Groovy and read by the scripting engine).
I think I finally got it, after a couple of iterations and feedback from
Jochen. A proposed patch (patch4975_v2.txt) is attachted to the jira
issue; it contains quite a few changes to the scripting engine. I
thought it might be useful to bring this to attention on this list; any
feedback on the proposed patch would be useful.

(1) you subclass DependencyTracker and you change DependencyTracker in that patch.. why not move the logic for subclassing into the DependencyTracker class? The class is only for GSE atm

(2) I am a bit worried by that loop

<at> <at> -557,6 +584,15 <at> <at> public class GroovyScriptEngine implements ResourceConnector {
            if (depEntry.lastModified < lastMod) {
                ScriptCacheEntry newEntry = new ScriptCacheEntry(depEntry.scriptClass, lastMod, depEntry.dependencies);
                scriptCache.put(scriptName, newEntry);
+                /* GROOVY-4975 Removing entries from scriptcache and classcache that depended on the entry we just
+                 * updated; these should be recompiled. */
+                for(Map.Entry<String, ScriptCacheEntry> currentEntry : scriptCache.entrySet()) {
+                       if(!currentEntry.getKey().equals(scriptName) &&
+                                       currentEntry.getValue().dependencies.contains(scriptName)) {
+                       scriptCache.remove(currentEntry.getKey());
+                       groovyLoader.removeClassCacheEntry(currentEntry.getValue().scriptClass.getName());
+                       }
+                }

I wonder if we should work with a reverse transitive hull or something like that instead, then we could avoid going through the potentially large number of entries.

The other thing is the new usage of scriptCache. isSourceNewer is not synchronized and called by loadScriptByName.

parseClass has for example this one:

           ScriptCacheEntry origEntry = scriptCache.get(codeSource.getName());
           Set<String> origDep = null;
           if (origEntry != null) origDep = origEntry.dependencies;

this was ok, because before it was ensured, that if there was an entry once, there always will be. But since in that patch you now remove things outside of the synchronization scope of this method, it can fail.

A similar thing is the removal of class entries in the groovy class loader... assuming you remove them in isSourceNewer, and right after that it is added again, because a parseClass call, that adds the entry just completed. What is the point of removing the entry then?

parseClass itself is not called in a synchronized context either. super.parseClass will call such a context. And before it was not enter that context in GSE itself, because we used ThreadLocals and such anyway. So I guess in this new logic we would have to start using super.sourceCache to synchronize, making the engine a lot slower if we do that in isSourceNewer... or we move the removal code somewhere else... for example into our newly synchronized parseClass...

The other way would be to rethink the solution with removals. Can't it work with just updates?

bye blackdrag

--
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


---------------------------------------------------------------------
To unsubscribe from this list, please visit:

  http://xircles.codehaus.org/manage_email

Thanks for contributing it!

Now we want even more from you

Guillaume 

Le 2 févr. 2012 à 16:17, "Passell, Matthew" <matthew.passell-B24Wn5KSNrjby3iVrkZq2A@public.gmane.org> a écrit :

Merci beaucoup! :)

It's a pretty small contribution, but I'm glad it got in anyway.

--Matt

On Thu, Feb 2, 2012 at 2:18 AM, Cédric Champeau <cedric.champeau <at> gmail.com> wrote:

Done :)

Le 01/02/2012 16:20, Passell, Matthew a écrit :

Hi All,

Would anyone with commit rights be able to look at the patch I attached to http://jira.codehaus.org/browse/GROOVY-5241 ?  I know it's not a major enhancement, but it would be nice to get it into 1.8.6. :)

Thanks,

Matt

On Wed, Feb 1, 2012 at 6:38 AM, Guillaume Laforge <glaforge-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

Hi developers,

We're targeting the release of Groovy 1.8.6 for Thursday 9th.

We'll create a release branch on Tuesday, to freeze the code, and release a snapshot in our snapshot repository, for those who want to test this snapshot with their respective projects, tools and frameworks.

If you've got a few things in the pipeline, this is the time to see if you want to integrate that in 1.8.6 before the release.

Thanks for your attention and happy coding

--
Guillaume Laforge
Groovy Project Manager
SpringSource, a division of VMware

Blog: http://glaforge.appspot.com/

Twitter: <at> glaforge

Google+: http://gplus.to/glaforge

-- Cédric Champeau SpringSource - A Division Of VMware http://www.springsource.com/ http://twitter.com/CedricChampeau