Alexei, Sorry for confusing. The patch for review is H6039.patch_2. Please kindly provide your comment.
Aleksey, I have not measured the performance before completing the code review. I will do that later. thx, Wenlong On Wed, Jan 14, 2009 at 9:14 PM, Wenlong Li <[email protected]> wrote: > 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 >>>>>>> > >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> С уважением, >>>>>> Алексей Федотов, >>>>>> ЗАО «Телеком Экспресс» >>>>>> >>>>> >>>> >>> >> >
