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