harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pavel Pervov" <pmcfi...@gmail.com>
Subject Re: [VM] On-demand class library parsing is ready to commit
Date Wed, 14 Jan 2009 12:43:45 GMT
Wenlong,

Please, see my comments in the JIRA.

WBR,
    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