harmony-dev mailing list archives

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

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
>



-- 
С уважением,
Алексей Федотов,
ЗАО «Телеком Экспресс»
Mime
View raw message