RFR: Initial code for a JSON-based module descriptor.

[David Bosschaert]

Hi Shi Jun,

Thanks for the review!

On 23 April 2012 10:35, Shi Jun Zhang <zhangshj at linux.vnet.ibm.com> wrote:
> Hi David,
>
> I get the following exception when i run the test you added.
>
> chance at chance:~/workspace/penrose/jigsaw/jdk/test/org/openjdk/jigsaw/z.test$
> ../../../../../build/bin/java -L z.mlib -m com.greetings.json
> Error occurred during initialization of VM
> java.lang.InternalError: Recursive initialization of system class loader
>    at java.lang.ClassLoader.initSystemClassLoader(ClassLoader.java:1515)
>    at java.lang.ClassLoader.getSystemClassLoader(ClassLoader.java:1494)
>    at java.util.ServiceLoader.loadInstalled(ServiceLoader.java:618)
>    at
> sun.util.LocaleServiceProviderPool$1.run(LocaleServiceProviderPool.java:131)
>    at java.security.AccessController.doPrivileged(Native Method)
>    at
> sun.util.LocaleServiceProviderPool.<init>(LocaleServiceProviderPool.java:129)
>    at
> sun.util.LocaleServiceProviderPool.getPool(LocaleServiceProviderPool.java:111)
>    at java.text.NumberFormat.getInstance(NumberFormat.java:747)
>    at java.text.NumberFormat.getNumberInstance(NumberFormat.java:407)
>    at java.util.Scanner.useLocale(Scanner.java:1231)
>    at java.util.Scanner.<init>(Scanner.java:585)
>    at java.util.Scanner.<init>(Scanner.java:608)
>    at java.lang.module.JSONParser.<init>(JSONParser.java:87)
>    at
> java.lang.module.JSONModuleInfoReader.<init>(JSONModuleInfoReader.java:61)
>    at
> java.lang.module.JSONModuleInfoReader.read(JSONModuleInfoReader.java:47)
>    at
> java.lang.module.ModuleSystem.parseModuleInfoJSON(ModuleSystem.java:80)
>    at org.openjdk.jigsaw.SimpleLibrary.findModuleDir(SimpleLibrary.java:763)
>    at
> org.openjdk.jigsaw.SimpleLibrary.readLocalModuleInfoJSON(SimpleLibrary.java:855)
>    at org.openjdk.jigsaw.Library.readLocalModuleInfo(Library.java:142)
>    at org.openjdk.jigsaw.Catalog.readModuleView(Catalog.java:277)
>    at org.openjdk.jigsaw.Launcher.loadModule(Launcher.java:51)
>    at org.openjdk.jigsaw.Launcher.launch(Launcher.java:82)
>    at
> java.lang.ClassLoader.initModularSystemClassLoader(ClassLoader.java:1557)
>    at java.lang.ClassLoader.initSystemClassLoader(ClassLoader.java:1522)
>    at java.lang.ClassLoader.getSystemClassLoader(ClassLoader.java:1494)
>
> Can you verify whether you are using the latest code?

I have never seen that error. Will certainly verify.

> As you just read the
> whole json file into JSONParser, it's not necessary using the Scanner.

I sometimes use the Scanner class as a convenient way to read the
whole file into a string with a onliner:
  new java.util.Scanner(is).useDelimiter("\\A").next()

I guess I should really use
  new java.util.Scanner(is, "UTF-8").useDelimiter("\\A").next()
but other than that is there an issue with this? Do you prefer the
more typical loop reading from the stream until exhausted or is there
another issue with using Scanner?

> And some comments on the changes.
>
> src/share/classes/java/lang/module/ModuleClassLoader.java
>
>  72         ModuleInfo mi;
>  73
>  74         // Weird: getResource/getResources() return matching files from
> all modules, even if not exported.
>  75         // URL miXML = getResource("/META-INF/module-info.xml");
>  76         URL miJSON = null;
>  77         try {
>  78             for (Enumeration<URL>  infos =
> getResources("/META-INF/module-info.json"); infos.hasMoreElements(); ) {
>  79                 URL info = infos.nextElement();
>  80                 String suffix = id.name() + "/" +
> id.version().toString() + "/classes!/META-INF/module-info.json";
>  81                 if (info.toExternalForm().endsWith(suffix)) {
>  82                     miJSON = info;
>  83                     break;
>  84                 }
>  85             }
>  86         } catch (IOException e) {
>  87             throw new Error(e);
>  88         }
>  89
>  90         if (miJSON != null) {
>  91             try {
>  92                 mi =
> moduleSystem.parseModuleInfoJSON(miJSON.openStream());
>  93             } catch (IOException e) {
>  94                 throw new Error(e);
>  95             }
>  96         } else {
>  97             mi = moduleSystem.parseModuleInfo(bs);
>  98         }
>
> ModuleClassLoader doesn't implement its own getResource() method so the
> getResource invocation will call java.lang.ClassLoader.getResource and it
> will delegate to parent's class loader first. I guess using
> ((org.openjdk.jigsaw.Loader)this).getResource("/META-INF/module-info.json")
> will work. Although another method isModulePresent in ModuleClassLoader also
> cast itself to org.openjdk.jigsaw.Loader, i don't think casting to a
> subclass is a good design. A better way to solve this problem is that not
> changing the defineModule method but adding a new method named
> defineModuleByJSON which accepts JSON input stream. Then in
> org.openjdk.jigsaw.Loader.findModule method, call defineModuleByJSON method
> if module-info.json exists, else call defineModule method.

Good idea - I'll try this out soon.

> src/share/classes/java/lang/module/JSONParser.java
> Line 152: jsonKeyValueList.charAt(i) is used for multiple times, assign it
> to a local var.

Sure, that's fine.

> The parser is pretty small but i am worried about the efficiency. The more
> layers the JSON string has, more times the end part of the JSON string will
> be parsed. For example,
> {
>    "key1" :
>    {
>        "key2" :
>        {
>            "key3" : "value"
>        }
>    }
> }
> First time, it go through the whole string and find one key-value string and
> then use regex to match "key1" and its value string. Second time, it go
> through the string '"key2": { "key3" : "value" }' and find one key-value
> string and then use regex to match "key2" and its value string. Third time,
> it go through the string '"key3" : "value"' and find one key-value string
> and then use regex to match "key3" and its value "value". The performance
> degrades fast when the JSON structure becomes complex.

So I'm a little worried about premature optimization here. As you
noted, the current impl uses a recursive approach where the json
layers are peeled off one--by-one. While you are correct that nested
values are scanned more than once (until they are at the top level)
I'm not sure whether optimizing this here right now is completely
necessary. I've measured performance on a similar mechanism in the
past and simply iterating over a number of in-memory buffers was shown
to be extremely fast. Note that the module-info.json files are likely
going to be relatively small so I'm not sure that this is the topmost
priority right now. However I would be open to revisiting the
JSONParser later.

> Other changes look ok to me.

Ok great - I'll work on providing an updated version, maybe you could
clarify whether you think using Scanner to read a file into memory is
problematic.

David

> Regards,
>
> Shi Jun Zhang
>
>