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
