harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Xiao-Feng Li" <xiaofeng...@gmail.com>
Subject Re: [VM] On-demand class library parsing is ready to commit
Date Fri, 26 Dec 2008 09:16:09 GMT
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