harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Wenlong Li" <wenl...@gmail.com>
Subject Re: [VM] On-demand class library parsing is ready to commit
Date Tue, 06 Jan 2009 02:09:59 GMT
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 <xiaofeng.li@gmail.com> wrote:
> On Fri, Dec 26, 2008 at 5:08 PM, Alexei Fedotov
> <alexei.fedotov@gmail.com> 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 <xiaofeng.li@gmail.com> wrote:
>>> On Fri, Dec 26, 2008 at 4:03 PM, Alexei Fedotov
>>> <alexei.fedotov@gmail.com> 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 <xiaofeng.li@gmail.com>
wrote:
>>>>
>>>>> On Fri, Dec 26, 2008 at 12:49 AM, Alexei Fedotov
>>>>> <alexei.fedotov@gmail.com> 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
>>>>> > <aleksey.shipilev@gmail.com> 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 <wenlong@gmail.com>
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 <pmcfirst@gmail.com>
>>>>> 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 <wenlong@gmail.com>
>>>>> 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 <pmcfirst@gmail.com>
>>>>> wrote:
>>>>> >>>>>> If this would be VM-side automatically produced
configuration
>>>>> file...
>>>>> >>>>>>
>>>>> >>>>>> WBR,
>>>>> >>>>>>    Pavel.
>>>>> >>>>>>
>>>>> >>>>>> On Thu, Dec 25, 2008 at 4:58 PM, Wenlong Li
<wenlong@gmail.com>
>>>>> 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
<pmcfirst@gmail.com>
>>>>> 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 <wenlong@gmail.com>
>>>>> 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
>>>>> >>>>>>>>> <aleksey.shipilev@gmail.com>
wrote:
>>>>> >>>>>>>>>> Hi, Wenlong.
>>>>> >>>>>>>>>>
>>>>> >>>>>>>>>> On Thu, Dec 25, 2008 at 11:49
AM, Wenlong Li <wenlong@gmail.com>
>>>>> 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
>
Mime
View raw message