Github user wangyf2010 commented on the pull request:
https://github.com/apache/tomcat/pull/22#issuecomment-131473216
Thanks for your comments, market!
Understood your concern, let me explain it one by one:
>The patch appears to be quite large for a marginal benfit. Separating the
refactoring and the changes into separate commits would enable the true scale
of the changes to be judged.
>I remain on the fence about this enhancement, pending a patch that clearly
shows the scale of the changes.
Benefit of this feature:
For enterprise applications, there are tons of jars in class path.
For our case, one application have over 500 jars in class path, it took
about 23s on annotation scanning. Scan them in parallel could reduce it to be
14s. That's big gains. And also scan it in parallel isn't a new idea, jetty
already supported, anyway, it's a reasonable feature for tomcat also.
As for refactoring code on ContextConfig:
ContextConfig class has too many code inside it, and annotation scanning is
an independent functionality indeed. And parallel scanning's idea is: split
each jar's scanning as separate task to run.
That's why I'd prefer to have separate class and let it focus on one jar's
annotation scanning.
BTW, it's only move that jar scanning logic into new class, no change on
all methods' logics. So from this perspective, change is few.
>I am -1 on the patch as current written since:
>it is not thread safe (look at the Java class cache)
>it does not follow the coding style of the existing code:
>no tabs (use 4 * spaces for indent)
>no new line before {
>no space after (
>no space before )
>it uses system properties for configuration when they are not necessary.
Configuration for this feature >should be at the Context level
>no documentation provided
>no license header in new files
>max coding line length is 100
>max comment line length is 80
>it needs to be dsiabled by default
1. code style
Could you let me know where is the check style xml for tomcat coding? I
tried to find it, but haven't get it.
2. document & license are problem, you're right. I will add them.
3. system property to disable this feature should be in context level.
Agree, will cover that.
>I also have some more general concerns about this feature:
>Is parallel threads scanning the same class an issue and if so, how do we
prevent that?
It isn't possible to let parallel threads to scan same class, because, this
feature is split each jar's scanning as separate task and dispatch separate
thread to scanning. Won't have the case that multiple threads scan same jar or
same class.
>The deployment process is already (or can be) multi-threaded. How much
benefit does this feature >provide when multiple apps are being deployed in
parallel?
This feature is for one app's performance improvement. Even multiple apps
are deployed in parallel, this feature could benefit each app's startup. As for
how much, it depends on each app's dependency jars. If there are tons of
dependency jars, this feature could benefit a lot.
>What will be the impact on performance for already deployed applications
if this feature is enabled?
First, you mean "deployed", means already started it or just deployed onto
tomcat?
If you're meaning latter one. This feature is to improve server startup
performance, not care whether this app is deployed or not.
>Is a 20s default tieout to scan a JAR appropriate?
yes, that's for one jar, that should be good enough, per benchmark metrics,
20s could scan one jar with size 200MB.
Thanks for your detail comments, again.
From my view, scanning annotations in parallel is better way, and as web
server, tomcat also need it.
It could benefit those monster apps, especially for enterprise level apps.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
