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 09:33:19 GMT
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