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, 07 Jan 2009 04:08:39 GMT
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