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

[Shi Jun Zhang]

On 4/23/2012 9:07 PM, David Bosschaert wrote:
> 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?

Setting "UTF-8" charset explicitly still doesn't work. The problem is 
that Scanner will always get an instance of java.text.NumberFormat, 
which will load services. And at this point, it seems that BootLoader 
has not been initialized yet. BootLoader.getLoader() returns null which 
makes ServiceLoader think it's in legacy mode. I am not sure whether 
it's a bug in ServiceLoader, but i don't think it is a good idea to load 
services before the system class loader being initialized. If there is a 
solution to solve the recursive initialization of system class loader 
problem, i'm glad to see the input stream can be read in one line with 
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.

Agreed. It's low priority until we get an example in real which could 
cause the problem.

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

I'm fine with Scanner as long as it doesn't throw exception.

-- 
Regards,

Shi Jun Zhang