Wenlong, It would be very nice of you to explicitely outline in the last JIRA comment and/or the cover letter which patches are intended to reviewed (and committed). The only patch which is intended for commit is H6039.patch_2, isn't it?
[1] http://issues.apache.org/jira/secure/attachment/12397873/H6039.patch_2 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 >>>>> > >>>>> >>>> >>>> >>>> >>>> -- >>>> С уважением, >>>> Алексей Федотов, >>>> ЗАО «Телеком Экспресс» >>>> >>> >> > -- С уважением, Алексей Федотов, ЗАО «Телеком Экспресс»
