All, I submit a new patch for on-demand class loading and parsing. All codes are put in VM side, and the mapping info is automatically produced.
Pls see https://issues.apache.org/jira/browse/HARMONY-6039 Comments are welcome. Thx, Wenlong Managed Runtime Technology Center, Intel On Wed, Jan 7, 2009 at 12:08 PM, Wenlong Li <[email protected]> wrote: > All, > At this moment, I move all updates in classlib to VM side such that > there is no modularity issue. Next step is to produce the mapping > between module and library efficiently and accurately. > > Comments are welcome. > > Thx, Wenlong > Managed Runtime Technology Center, Intel > > On Tue, Jan 6, 2009 at 11:08 PM, Wenlong Li <[email protected]> wrote: >> Thx :) >> >> On Tue, Jan 6, 2009 at 10:35 PM, Alexei Fedotov >> <[email protected]> wrote: >>> Sure. >>> >>> 1. If you dig into SetClasspathFromString, you will see that it starts from >>> splitting the given classpath into pieces. You already know the new piece >>> you add and may skip splitting step. >>> >>> 2. If I understand you code correctly, the case "pdest > >>> (*it).second->bytes" might be a subject of a negative assertion. Adding this >>> assrtion would speed up bug discovery. >>> >>> Thanks. >>> >>> >>> On Tue, Jan 6, 2009 at 5:09 AM, Wenlong Li <[email protected]> wrote: >>> >>>> Yes, Xiao-Feng's understanding is correct. The patch loads and parses >>>> modules on demand. If no class in swing.jar is not requested, then >>>> this module will not be loaded. >>>> >>>> btw, Alexei, you said "SetClasspathFromString" and "pdest > >>>> (*it).second->bytes" are not efficient. Can you share more comments on >>>> them? I just reused some code in Harmony, and didn't optimize them >>>> further. >>>> >>>> Thx, Wenlong >>>> Managed Runtime Technology Center, Intel >>>> >>>> On Fri, Dec 26, 2008 at 5:16 PM, Xiao-Feng Li <[email protected]> >>>> wrote: >>>> > On Fri, Dec 26, 2008 at 5:08 PM, Alexei Fedotov >>>> > <[email protected]> wrote: >>>> >> Xiao Feng, >>>> >> Thank you for explaining. >>>> >> >>>> >> I get more minor comments on more commented code, ineffective >>>> >> SetClasspathFromString usage, non-covered unexpected case when pdest > >>>> >> (*it).second->bytes. One major comment on crossing vm module boundary >>>> >> still remains. But now I'm happy with the design. >>>> > >>>> > Alexei, yes, I agree with your comments. These parts should be >>>> > improved. (Still, this is my personal opinion. :) Let's wait Wenlong >>>> > speaking.) >>>> > >>>> > Thanks, >>>> > xiaofeng >>>> > >>>> >> Sorry for being slow. >>>> >> >>>> >> >>>> >> >>>> >> On Fri, Dec 26, 2008 at 11:40 AM, Xiao-Feng Li <[email protected]> >>>> wrote: >>>> >>> On Fri, Dec 26, 2008 at 4:03 PM, Alexei Fedotov >>>> >>> <[email protected]> wrote: >>>> >>>> Xiao-Feng, >>>> >>>> Continuing with the server example could you please give me a hint >>>> where >>>> >>>> decision to load swing.jar or not is taken in the patch? My initial >>>> >>>> perception was that the list of what to load was hardcoded and was not >>>> >>>> constructed dynamically depending on application. >>>> >>> >>>> >>> Alexei, here is the patch code I found: >>>> >>> >>>> >>> line 245: >>>> >>> + // Find which jar exports this package >>>> >>> + if (pkgName != NULL) { >>>> >>> + char *boot_class_path = >>>> >>> env->JavaProperties()->get(VM_BOOT_CLASS_DIR); >>>> >>> + char *pendingClassPath = NULL; >>>> >>> + apr_pool_t *tmp_pool; >>>> >>> + apr_pool_create(&tmp_pool, NULL); >>>> >>> + while (it != env->pending_jar_set.end()) { >>>> >>> + pdest = strstr( (*it).second->bytes, pkgName ); >>>> >>> + if (pdest != NULL) { >>>> >>> + pendingClassPath = >>>> >>> (char*)STD_MALLOC(strlen(boot_class_path) >>>> >>> + + >>>> strlen((*it).first->bytes) + 1); >>>> >>> + strcpy(pendingClassPath, boot_class_path); >>>> >>> + strcat(pendingClassPath, (*it).first->bytes); >>>> >>> + // Open this found jar, and read all classes >>>> >>> contained in this jar >>>> >>> + SetClasspathFromString(pendingClassPath, >>>> tmp_pool); >>>> >>> + // Erase the found jar from pending jar list >>>> >>> as it has been parsed >>>> >>> + env->pending_jar_set.erase(it++); >>>> >>> + STD_FREE(pendingClassPath); >>>> >>> + } else { >>>> >>> >>>> >>> It checks if a JAR has the requested package, then loads it if yes. I >>>> >>> am not sure if this is what you were asking. (Btw, this is only my >>>> >>> understanding of his patch. I am not speaking for Wenlong.) >>>> >>> >>>> >>> Thanks, >>>> >>> xiaofeng >>>> >>> >>>> >>>> Thanks. >>>> >>>> >>>> >>>> >>>> >>>> On Fri, Dec 26, 2008 at 4:14 AM, Xiao-Feng Li <[email protected]> >>>> wrote: >>>> >>>> >>>> >>>>> On Fri, Dec 26, 2008 at 12:49 AM, Alexei Fedotov >>>> >>>>> <[email protected]> wrote: >>>> >>>>> > Aleksey, >>>> >>>>> > I like your conclusion. >>>> >>>>> > >>>> >>>>> > Wenlong, >>>> >>>>> > I'm trying to understand the real life value of the "abstract" >>>> startup >>>> >>>>> > time metric you've suggested. Does Harmony with your patch load >>>> >>>>> > swing.jar for a server application? Do I understand that loading >>>> >>>>> > happens, though it happens later compared to VM without your patch? >>>> I >>>> >>>>> > believe that the proper design of delayed loading should answer >>>> "no" >>>> >>>>> > to this question. >>>> >>>>> >>>> >>>>> I checked the patch, and I found the answer is indeed "No" as you >>>> expected. >>>> >>>>> >>>> >>>>> Thanks, >>>> >>>>> xiaofeng >>>> >>>>> >>>> >>>>> > In other words, I appreciate if you describe which real use cases >>>> are >>>> >>>>> > improved by this patch. >>>> >>>>> > Thanks! >>>> >>>>> >>>> >>>>> > On Thu, Dec 25, 2008 at 7:29 PM, Aleksey Shipilev >>>> >>>>> > <[email protected]> wrote: >>>> >>>>> >> Ok, here's the catch. >>>> >>>>> >> >>>> >>>>> >> bootclasspath.properties is the SortedSet<JARfile>, which >>>> enumerates >>>> >>>>> >> the JARs available for bootclassloader. The set of such the JARs >>>> is >>>> >>>>> >> really stable because modular decomposition of classlib is stable. >>>> >>>>> >> That's why nobody bothers with automatic generation of it: it only >>>> >>>>> >> should be updated when new JAR file arrives. >>>> >>>>> >> Modulelibrarymapping.properties is different on this point, it's >>>> the >>>> >>>>> >> Map<PackageName,JARfile>, which should be updated each time the >>>> new >>>> >>>>> >> *package* is introduced. I'm not talking about java.* packages >>>> >>>>> >> (they're standardized), rather about org.apache.harmony.*. >>>> >>>>> >> >>>> >>>>> >> Automatic generation of this property file gives two advantages: >>>> >>>>> >> 1. Error-prone. Prevent yourself from hand-messing with mapping >>>> and >>>> >>>>> >> getting spurious ClassNotFoundException. BTW, what's the behaviour >>>> in >>>> >>>>> >> case the mapping is wrong? >>>> >>>>> >> 2. "Researchful". There're lot of guys around who enjoys the >>>> >>>>> >> modularity of Harmony classlib and eventually they might want to >>>> split >>>> >>>>> >> the packages even deeper, into smaller pieces. Then automatic >>>> >>>>> >> generation would enable them to quickly roll-in and experiment >>>> with >>>> >>>>> >> different package layouts and their impact on performance. They >>>> could >>>> >>>>> >> use ordinary bootclasspath.properties, but your feature wouldn't >>>> be >>>> >>>>> >> used by them then ;) >>>> >>>>> >> >>>> >>>>> >> That's merely a housekeeping procedure. I believe that anything >>>> which >>>> >>>>> >> could be done more than once, eventually would be done more than >>>> once. >>>> >>>>> >> Hence it should be automated. You say that the file was generated >>>> from >>>> >>>>> >> manifests of JARs, so is it hard to just tie the same tool into >>>> DRLVM >>>> >>>>> >> build process? >>>> >>>>> >> >>>> >>>>> >> As for DRLVM-specific, my opinion that this is because the patch: >>>> >>>>> >> a. breaks the compatibility of classlib (you change >>>> >>>>> >> bootclasspath.properties, right?) with other VMs. >>>> >>>>> >> b. treated in DRLVM classloader only. >>>> >>>>> >> >>>> >>>>> >> Of course eventually this feature might be used by others, but IMO >>>> we >>>> >>>>> >> should be careful about other guys who use the same classlib. I'd >>>> >>>>> >> rather wait for some incubation on DRLVM side first. >>>> >>>>> >> >>>> >>>>> >> Thanks, >>>> >>>>> >> Aleksey. >>>> >>>>> >> >>>> >>>>> >> On Thu, Dec 25, 2008 at 6:18 PM, Wenlong Li <[email protected]> >>>> wrote: >>>> >>>>> >>> I see. In fact, my file doesn't need track change at the class >>>> >>>>> >>> granularity. Instead, it only needs know package info provided in >>>> the >>>> >>>>> >>> manifest file. When class is added to a library, do we need >>>> change >>>> >>>>> >>> the manifest as well? >>>> >>>>> >>> >>>> >>>>> >>> btw, I guess there is a mis-understanding: my >>>> modulelibrarymapping >>>> >>>>> >>> file only records package info provided by manfiest in each >>>> module. It >>>> >>>>> >>> doesn't relate to each class. >>>> >>>>> >>> >>>> >>>>> >>> thx, >>>> >>>>> >>> Wenlong >>>> >>>>> >>> >>>> >>>>> >>> On Thu, Dec 25, 2008 at 10:55 PM, Pavel Pervov < >>>> [email protected]> >>>> >>>>> wrote: >>>> >>>>> >>>> Classes are added to class library from time to time. I'm not >>>> sure how >>>> >>>>> >>>> it can be possible to track these changes manually. >>>> >>>>> >>>> >>>> >>>>> >>>> WBR, >>>> >>>>> >>>> Pavel. >>>> >>>>> >>>> >>>> >>>>> >>>> >>>> >>>>> >>>> On Thu, Dec 25, 2008 at 5:09 PM, Wenlong Li <[email protected]> >>>> >>>>> wrote: >>>> >>>>> >>>>> Sorry, one more question: bootclasspath.properties is classlib >>>> >>>>> >>>>> specific file, why we could not make a vm specific file >>>> manually? >>>> >>>>> Just >>>> >>>>> >>>>> curious to know the possible reason. :) >>>> >>>>> >>>>> >>>> >>>>> >>>>> thx, >>>> >>>>> >>>>> Wenlong >>>> >>>>> >>>>> >>>> >>>>> >>>>> On Thu, Dec 25, 2008 at 10:00 PM, Pavel Pervov < >>>> [email protected]> >>>> >>>>> wrote: >>>> >>>>> >>>>>> If this would be VM-side automatically produced configuration >>>> >>>>> file... >>>> >>>>> >>>>>> >>>> >>>>> >>>>>> WBR, >>>> >>>>> >>>>>> Pavel. >>>> >>>>> >>>>>> >>>> >>>>> >>>>>> On Thu, Dec 25, 2008 at 4:58 PM, Wenlong Li < >>>> [email protected]> >>>> >>>>> wrote: >>>> >>>>> >>>>>>> btw, because adding new module is rare case, manually >>>> modifying the >>>> >>>>> >>>>>>> bootclasspath.properties is not an issue? >>>> >>>>> >>>>>>> >>>> >>>>> >>>>>>> If so, can I conclude adding another property file with same >>>> update >>>> >>>>> >>>>>>> frequency as bootclasspath would be fine as well? >>>> >>>>> >>>>>>> >>>> >>>>> >>>>>>> Pls kindly correct me if my understanding is wrong. >>>> >>>>> >>>>>>> >>>> >>>>> >>>>>>> On Thu, Dec 25, 2008 at 9:05 PM, Pavel Pervov < >>>> [email protected]> >>>> >>>>> wrote: >>>> >>>>> >>>>>>>> Wenlong, >>>> >>>>> >>>>>>>> >>>> >>>>> >>>>>>>> Note, that bootclasspath.properties is only changed on >>>> adding new >>>> >>>>> >>>>>>>> module. This is pretty rare occasion, I believe. >>>> >>>>> >>>>>>>> >>>> >>>>> >>>>>>>> WBR, >>>> >>>>> >>>>>>>> Pavel. >>>> >>>>> >>>>>>>> >>>> >>>>> >>>>>>>> On Thu, Dec 25, 2008 at 3:48 PM, Wenlong Li < >>>> [email protected]> >>>> >>>>> wrote: >>>> >>>>> >>>>>>>>> Thx for your advice. Alexey. >>>> >>>>> >>>>>>>>> >>>> >>>>> >>>>>>>>> Here I have one question: do you know how the >>>> >>>>> bootclasspath.properties >>>> >>>>> >>>>>>>>> is maintained, manually or automatically? >>>> >>>>> >>>>>>>>> >>>> >>>>> >>>>>>>>> Another comment is I would like to treat the patch as DRLVM >>>> >>>>> specific >>>> >>>>> >>>>>>>>> optimization, e.g., it targets for improving VM creation >>>> time. So >>>> >>>>> that >>>> >>>>> >>>>>>>>> is possible to move all updates to DRLVM part to eliminate >>>> >>>>> potential >>>> >>>>> >>>>>>>>> modularity and compatibility problem. >>>> >>>>> >>>>>>>>> >>>> >>>>> >>>>>>>>> thx, >>>> >>>>> >>>>>>>>> Wenlong >>>> >>>>> >>>>>>>>> >>>> >>>>> >>>>>>>>> On Thu, Dec 25, 2008 at 5:32 PM, Aleksey Shipilev >>>> >>>>> >>>>>>>>> <[email protected]> wrote: >>>> >>>>> >>>>>>>>>> Hi, Wenlong. >>>> >>>>> >>>>>>>>>> >>>> >>>>> >>>>>>>>>> On Thu, Dec 25, 2008 at 11:49 AM, Wenlong Li < >>>> [email protected]> >>>> >>>>> wrote: >>>> >>>>> >>>>>>>>>>> btw, Alexey, Let's go back to discuss whether there is a >>>> need >>>> >>>>> to >>>> >>>>> >>>>>>>>>>> include this feature in Harmony, given 17% performance >>>> gain in >>>> >>>>> Linux >>>> >>>>> >>>>>>>>>>> when using your methodology. For windows test, I will >>>> double >>>> >>>>> check the >>>> >>>>> >>>>>>>>>>> backgroud process as you pointed out. >>>> >>>>> >>>>>>>>>> >>>> >>>>> >>>>>>>>>> My opinion was already expressed after I had finished the >>>> tests >>>> >>>>> from >>>> >>>>> >>>>>>>>>> my side: the boost can be achieved in specific conditions, >>>> so >>>> >>>>> whether >>>> >>>>> >>>>>>>>>> it's worth including into Harmony really depends on how >>>> much >>>> >>>>> mess the >>>> >>>>> >>>>>>>>>> patch would introduce besides the "performance boost". >>>> From what >>>> >>>>> I >>>> >>>>> >>>>>>>>>> see, the patch obliges the maintainer to maintain the >>>> correct >>>> >>>>> mapping >>>> >>>>> >>>>>>>>>> between jars and Java packages. This new feature is also >>>> spread >>>> >>>>> >>>>>>>>>> between Classlib and VM, but it seems like DRLVM specific. >>>> In >>>> >>>>> this >>>> >>>>> >>>>>>>>>> case I would rather stay without the patch. >>>> >>>>> >>>>>>>>>> >>>> >>>>> >>>>>>>>>> Personally (if I'll be committer) I would accept the patch >>>> with >>>> >>>>> two >>>> >>>>> >>>>>>>>>> serious modifications: >>>> >>>>> >>>>>>>>>> 1. Stay within DRLVM, do not introduce this feature into >>>> >>>>> Classlib, >>>> >>>>> >>>>>>>>>> get the thing tested and evolved on DRLVM side. Otherwise >>>> it >>>> >>>>> might >>>> >>>>> >>>>>>>>>> break the compatibility with other VMs. >>>> >>>>> >>>>>>>>>> 2. Make the mapping generated automatically (during build >>>> >>>>> process?) >>>> >>>>> >>>>>>>>>> to free the burden for maintainers. >>>> >>>>> >>>>>>>>>> >>>> >>>>> >>>>>>>>>> Thanks, >>>> >>>>> >>>>>>>>>> Aleksey. >>>> >>>>> >>>>>>>>>> >>>> >>>>> >>>>>>>>> >>>> >>>>> >>>>>>>> >>>> >>>>> >>>>>>> >>>> >>>>> >>>>>> >>>> >>>>> >>>>> >>>> >>>>> >>>> >>>> >>>>> >>> >>>> >>>>> >> >>>> >>>>> > >>>> >>>>> > >>>> >>>>> > >>>> >>>>> > -- >>>> >>>>> > С уважением, >>>> >>>>> > Алексей Федотов, >>>> >>>>> > ЗАО «Телеком Экспресс» >>>> >>>>> > >>>> >>>>> >>>> >>>>> >>>> >>>>> >>>> >>>>> -- >>>> >>>>> http://xiao-feng.blogspot.com >>>> >>>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> -- >>>> >>>> С уважением, >>>> >>>> Алексей Федотов, >>>> >>>> ЗАО «Телеком Экспресс» >>>> >>>> >>>> >>> >>>> >>> >>>> >>> >>>> >>> -- >>>> >>> http://xiao-feng.blogspot.com >>>> >>> >>>> >> >>>> >> >>>> >> >>>> >> -- >>>> >> С уважением, >>>> >> Алексей Федотов, >>>> >> ЗАО «Телеком Экспресс» >>>> >> >>>> > >>>> > >>>> > >>>> > -- >>>> > http://xiao-feng.blogspot.com >>>> > >>>> >>> >>> >>> >>> -- >>> С уважением, >>> Алексей Федотов, >>> ЗАО «Телеком Экспресс» >>> >> >
