Here review continues and completes, https://issues.apache.org/jira/browse/HARMONY-6039?focusedCommentId=12664090#action_12664090
Overall, review comments about restructuring the code, avoiding cut&paste and separating new changes are also pleas of making the review of these parts more productive. Wenlong, thanks for the patch! On Thu, Jan 15, 2009 at 11:49 AM, Alexei Fedotov <[email protected]> wrote: > Hello Wenlong, > > The intention of the following review is to improve the readability of > the code. Please find my comments preceded with patch line numbers and > fix anything you find worthy to fix. > > 9 > excessive comment length > > 9- > missed description of parameters (e.g. @param mapFile <description>) > and return value > do we need to pass mapFile through the parameter chain? may it be an element > of > > 22, 24 > we don't need both versions of each function, do we? > using one version (esp. of SetBCPElement) would make the whole code size > smaller > > it would be easier for me to review your deltas of functions if you > don't make the full copies of them > > 37 > seems that environment.h has c/apr style set of includes > can we hide <map> and related typedef in sources to maintain C/apr > style of interfaces > is it possible to use more specific header (e.g. related to jar > parsing) than environment.h for JarFilePackageMapping definition? > > 93 > *the* bootstrap classpath > > 96- > the proper bracket style is specified here > https://issues.apache.org/jira/secure/attachment/12353745/format.sh > [well, the whole file is formatted strangely - Pavel, could you comment?] > > 97 > such -> the > > 150 > *a* temp pool > > 154- > putting map file operations into separate .cpp file with a clear and > readable interface function names in the corresponding .h interface > would not make existing code less readable > > that .h file would be a proper place for new types you introduce, not > environment.h > > you may also use the proper Apache formatting in the new file > > 190- > cannot understand where the signature file comes from - I cannot find > apr_file_write for it > the explanatory comment is welcome > > if both mapping and signature files are things introduced by this > patch why don't we use one file instead of two > > 200 > can this be replaced with assert(luni_path)? > > 213 > +1 to Aleksey's comment on literals > > [have to go, will continue later] > > 434 > commented code > > Thanks. > > > On Wed, Jan 14, 2009 at 4:38 PM, Wenlong Li <[email protected]> wrote: >> >> 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 >> >>>>>>> > >> >>>>>>> >> >>>>>> >> >>>>>> >> >>>>>> >> >>>>>> -- >> >>>>>> С уважением, >> >>>>>> Алексей Федотов, >> >>>>>> ЗАО «Телеком Экспресс» >> >>>>>> >> >>>>> >> >>>> >> >>> >> >> >> > > > > > -- > С уважением, > Алексей Федотов, > ЗАО «Телеком Экспресс» > -- С уважением, Алексей Федотов, ЗАО «Телеком Экспресс»
