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:14:25 GMT
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