Wenlong,
Please, see my comments in the JIRA.
WBR,
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
>>>>> >
>>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> С уважением,
>>>> Алексей Федотов,
>>>> ЗАО «Телеком Экспресс»
>>>>
>>>
>>
>
