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.
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 > -- С уважением, Алексей Федотов, ЗАО «Телеком Экспресс»
