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 Wed, 14 Jan 2009 13:38:26 GMT
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 <wenlong@gmail.com> wrote:
> Pavel,
>
> Pls see my comments in the JIRA.
>
> thx, Wenlong
>
> On Wed, Jan 14, 2009 at 8:44 PM, Pavel Pervov <pmcfirst@gmail.com> 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 <wenlong@gmail.com> 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 <wenlong@gmail.com> 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 <wenlong@gmail.com> wrote:
>>>>> Thx :)
>>>>>
>>>>> On Tue, Jan 6, 2009 at 10:35 PM, Alexei Fedotov
>>>>> <alexei.fedotov@gmail.com> 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 <wenlong@gmail.com>
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 <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