Pavel, Pls see my comments in the JIRA.
thx, Wenlong On Wed, Jan 14, 2009 at 8:44 PM, Pavel Pervov <[email protected]> wrote: > Please, also, check that jar entry caches still work correctly after your > patch. > > Pavel. > > On Wed, Jan 14, 2009 at 12:33 PM, Wenlong Li <[email protected]> wrote: >> 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 >>>>>> > >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> С уважением, >>>>> Алексей Федотов, >>>>> ЗАО «Телеком Экспресс» >>>>> >>>> >>> >> >
