Generally, can you justify this scheme? I read the article, but I still don't understand the advantage of this approach over say, Map<ClassLoader, ?> where the key is weak.
Functionally, the biggest problem is that it looks like we were holding ModuleDefs with soft references, but now they're being held with weak references, which will cause them to disappear too quickly. http://gwt-code-reviews.appspot.com/1274801/diff/1/3 File dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java (right): http://gwt-code-reviews.appspot.com/1274801/diff/1/3#newcode145 dev/core/src/com/google/gwt/dev/cfg/ModuleDefLoader.java:145: ModuleDef moduleDef = (ModuleDef) ClassLoaderLocalMap.get(key, moduleName); This should be fine for dev mode, it will always be the system classloader. http://gwt-code-reviews.appspot.com/1274801/diff/1/2 File dev/core/src/com/google/gwt/dev/util/ClassLoaderLocalMap.java (right): http://gwt-code-reviews.appspot.com/1274801/diff/1/2#newcode17 dev/core/src/com/google/gwt/dev/util/ClassLoaderLocalMap.java:17: * Based on http://java.dzone.com/articles/classloaderlocal-how-avoid I notice your implementation doesn't use a counter like in the article; would NOT having a counter cause problems with parent/child classloader situations? http://gwt-code-reviews.appspot.com/1274801/diff/1/2#newcode20 dev/core/src/com/google/gwt/dev/util/ClassLoaderLocalMap.java:20: public class ClassLoaderLocalMap implements Opcodes { Class should be templatized, right? http://gwt-code-reviews.appspot.com/1274801/diff/1/2#newcode20 dev/core/src/com/google/gwt/dev/util/ClassLoaderLocalMap.java:20: public class ClassLoaderLocalMap implements Opcodes { Having the outer class actually implement Opcodes is kinda bad. Either do static imports, or have some private inner class do the implement. http://gwt-code-reviews.appspot.com/1274801/diff/1/2#newcode44 dev/core/src/com/google/gwt/dev/util/ClassLoaderLocalMap.java:44: { I do this sometimes myself, actually, although usually to separate out local vars. For example, in this code, I'd actually move the declarations of "fv" and "mv" into their respective blocks. "mv" would get declared twice, once for each use. http://gwt-code-reviews.appspot.com/1274801/diff/1/2#newcode52 dev/core/src/com/google/gwt/dev/util/ClassLoaderLocalMap.java:52: mv.visitTypeInsn(NEW, "java/util/WeakHashMap"); Modules need to be held with soft references, per the Apache maps you're replacing. I don't think this map type gives you that. http://gwt-code-reviews.appspot.com/1274801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
