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 15:08:35 GMT
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