You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by GitBox <gi...@apache.org> on 2020/02/14 23:13:14 UTC
[GitHub] [tomcat] oopexpert opened a new pull request #242: get a hands on
...
oopexpert opened a new pull request #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242
First commits. Did some refactoring. I'll be happy to get some feedback if the process is correct. So don't be too mean if smth went wrong.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] oopexpert commented on issue #242: get a hands on ...
Posted by GitBox <gi...@apache.org>.
oopexpert commented on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586633902
I did the performance measurement. My setup:
i5 4440 CPU 3,1 GHz
10.000.000 calls on doOptions
Current implementation: 1,381 seconds
My implementation: 3,852 seconds
trace allowed evaluation mocked (omit reflection)
As expected: My implementation is 3 times slower under these conditions.
Further execution analysis shows that the reflection stuff the "trace allowed" evaluation does (even without executing "getAllowTrace") will take 11,814 seconds alone!!!
Let's do the math:
My implementation: 1,381 + 11,814 = 13,195
My implementation: 3,852 + 11,814 = 15,666
So the performance decreased by only 18% considering the reflection stuff at least partially.
The main point here is: we have 10 million calls...
So to say that this is a problem is the same as to say: "We have a problem JAVA. Let's write all our programs in Assembler so everything is faster!"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] oopexpert commented on issue #242: get a hands on ...
Posted by GitBox <gi...@apache.org>.
oopexpert commented on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586561346
I do understand that there are some prequistes that I did not consider according to the Java 8 deps. It would be helpful to get a hint what kind of line of code does violate it.
I also agree that performace might decrease a little bit. But you have to admit: Reflection in general is not fast at all. And as you said: Here performance does not really matter.
But what I do not understand is: what do you mean with excessive? According to my motivation: I like CleanCode an I wanted to help to improve the code base. Never thought to justify for that anywhere...
Nevertheless. Thank you for the the fast feedback. Could we at least have one more iteration where you see the problem with java deps.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] oopexpert commented on issue #242: get a hands on ...
Posted by GitBox <gi...@apache.org>.
oopexpert commented on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586642730
> Refactorings are nice, but you start to have to argue about them, then it's not the best thing.
Serious?
> As a general rule of thumb ... Ideally for a first contribution, you should try to look at fixing specific issues.
The point is: I like refactorings. This would have been my offer to support this project: Refactoring.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] oopexpert commented on issue #242: get a hands on ...
Posted by GitBox <gi...@apache.org>.
oopexpert commented on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586642121
> If Im not mistaken and if it helps, you can cache the method after the first call then cost will be negligible.
I could but it would change the assertion of the "allow trace" evaluation. I tried to keep the assertions as they were during my refactoring. Who knows which methods rely on a non-cached "allow trace evaluation"?
> Very worse case you make a catalina class implementing Function<HttpServletRequest, xxx> or equivalent in catalina and instantiate it at first call in the servlet (these classes are available) by reflection then it is native code.
Please rephrase it. I do not understand what you mean.
> So we are not yet at java limitations IMHO.
I never said that. I only see no problem with the performance 18% performance decrease. This is because...
... reflection calls are the crucial parts which were already part of the previous implementations
... the doOption() method will surely never be called 10 million times in 4 seconds
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] oopexpert edited a comment on issue #242: get a hands on
...
Posted by GitBox <gi...@apache.org>.
oopexpert edited a comment on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586642730
> Refactorings are nice, but you start to have to argue about them, then it's not the best thing.
Serious?
> As a general rule of thumb ... Ideally for a first contribution, you should try to look at fixing specific issues.
The point is: I like refactorings. This would have been my offer to support this project: Refactoring.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] oopexpert edited a comment on issue #242: get a hands on
...
Posted by GitBox <gi...@apache.org>.
oopexpert edited a comment on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586561346
I do understand that there are some prequistes that I did not consider according to the Java 8 deps. It would be helpful to get a hint what kind of line of code does violate it.
I also agree that performance might decrease a little bit. But you have to admit: Reflection in general is not fast at all. And as you also said: Here performance does not really matter.
But what I do not understand is: what do you mean with excessive? According to my motivation: I like CleanCode an I wanted to help to improve the code base. Never thought to justify for that anywhere...
Nevertheless. Thank you for the the fast feedback. Could we at least have one more iteration where you see the problem with java deps as I did compile it with Java 8. Maybe I have to adjust my local setup.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] oopexpert edited a comment on issue #242: get a hands on
...
Posted by GitBox <gi...@apache.org>.
oopexpert edited a comment on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586561346
I do understand that there are some prequistes that I did not consider according to the Java 8 deps. It would be helpful to get a hint what kind of line of code does violate it.
I also agree that performance might decrease a little bit. But you have to admit: Reflection in general is not fast at all. And as you also said: Here performance does not really matter.
But what I do not understand is: what do you mean with excessive? According to my motivation: I like CleanCode an I wanted to help to improve the code base. Never thought to justify for that anywhere...
Nevertheless. Thank you for the the fast feedback. Could we at least have one more iteration where you see the problem with java deps?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on issue #242: get a hands on ...
Posted by GitBox <gi...@apache.org>.
markt-asf commented on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586525169
Sorry, this PR cannot be applied. The Servlet API classes cannot have any direct dependencies other than the Java 8 API (which is why the Tomcat specific hack is implemented the way it is).
It would help to understand the motivation for the refactoring.
There does seem to be an excessive, and unnecessarily verbose, use of one line methods. e.g. `optionsAllowed()`
I'd be interested to see some performance numbers comparing the old and new code. My instinct is the new code will be slower. It probably doesn't matter for doOptions() but I'd still be interested.
Looking at the original code, there is certainly scope for improvement although, again, performance isn't critical here. A simpler refactoring that dropped the booleans, used a StringBuilder, started with an allow header value of `"OPTIONS"` and added to the allow header while looping though the methods is probably as far as I would have gone.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] markt-asf commented on issue #242: get a hands on ...
Posted by GitBox <gi...@apache.org>.
markt-asf commented on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586686517
The issue with refactoring for clean code is that the benefits are subjective and people are going to have different views.
Performance numbers are objective and while a refactoring that makes a small piece of code ~3x slower is going to have a negligible impact on performance, the idea of repeating that sort of refactoring - with that sort of impact - across the code base is not something that is likely to be considered acceptable.
The caching idea is one that occurred to me - although the TRACE component can't be cached because it is dynamic. There looks to be merit in that idea. This combined with the simpler refactoring I outlined earlier is more likely to get accepted.
The issue isn't that you have depended on Java 9+ functionality (as far as I could see from a quick look), but that you have added a compile time dependency on `org.apache.catalina.*`. That is not permitted for the Servlet API classes which cannot have any compile time dependencies other than the JRE.
It is also worth noting that, for ease of maintenance and back-ports, we try and keep the code aligned across all the currently supported Tomcat versions. That means a minimum Java version of Java 6. This isn't always practical and there are plenty of places where we don't do this but impact on maintenance is another factor to consider when introducing Java 8 specific code.
If refactoring is your area of interest then I'd recommend taking a look at the unit tests. There is a lot of scope there for reducing code duplication and improving code coverage by making much more use of parameterized tests. [Current code coverage stats](https://ci.apache.org/projects/tomcat/tomcat10/coverage/) are available from one of the CI systems we use.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] oopexpert edited a comment on issue #242: get a hands on
...
Posted by GitBox <gi...@apache.org>.
oopexpert edited a comment on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586642121
> If Im not mistaken and if it helps, you can cache the method after the first call then cost will be negligible.
I could but it would change the assertion of the "allow trace" evaluation. I tried to keep the assertions as they were during my refactoring. Who knows which methods rely on a non-cached "allow trace evaluation"?
> Very worse case you make a catalina class implementing Function<HttpServletRequest, xxx> or equivalent in catalina and instantiate it at first call in the servlet (these classes are available) by reflection then it is native code.
Please rephrase it. I do not understand what you mean.
> So we are not yet at java limitations IMHO.
I never said that. I only see no problem with the 18% performance decrease. This is because...
... reflection calls are the crucial parts which were already part of the previous implementations
... the doOption() method will surely never be called 10 million times in 4 seconds
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] oopexpert commented on issue #242: get a hands on ...
Posted by GitBox <gi...@apache.org>.
oopexpert commented on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586698111
> Performance numbers are objective and while a refactoring that makes a small piece of code ~3x slower is going to have a negligible impact on performance, the idea of repeating that sort of refactoring - with that sort of impact - across the code base is not something that is likely to be considered acceptable.
I see that you are not able to get my point. Maybe my english isn't that precise. Sorry about that.
> The issue isn't that you have depended on Java 9+ functionality (as far as I could see from a quick look), but that you have added a compile time dependency on org.apache.catalina.*. That is not permitted for the Servlet API classes which cannot have any compile time dependencies other than the JRE.
Here you got a point according to the "hack".
> The issue with refactoring for clean code is that the benefits are subjective and people are going to have different views.
Clean Code isn't a matter of "views". It's a matter of understanding.
Nevertheless: I heard enough. Thank you for your time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmannibucau commented on issue #242: get a hands on ...
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586638368
If Im not mistaken and if it helps, you can cache the method after the first call then cost will be negligible.
Very worse case you make a catalina class implementing Function<HttpServletRequest, xxx> or equivalent in catalina and instantiate it at first call in the servlet (these classes are available) by reflection then it is native code.
So we are not yet at java limitations IMHO.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] rmaucher commented on issue #242: get a hands on ...
Posted by GitBox <gi...@apache.org>.
rmaucher commented on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586641782
As a general rule of thumb ... Ideally for a first contribution, you should try to look at fixing specific issues. Refactorings are nice, but you start to have to argue about them, then it's not the best thing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] ChristopherSchultz commented on issue #242: get a hands on
...
Posted by GitBox <gi...@apache.org>.
ChristopherSchultz commented on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-587542753
> The issue with refactoring for clean code is that the benefits are subjective and people are going to have different views.
There is a term for this: [bikeshedding](https://en.wikipedia.org/wiki/Law_of_triviality)
Specific refactorings are often a good idea. Re-factoring code to make it "feel nicer" for a single programmer often ends up in a commit war.
Let's see where we can build consensus, and move-forward with those refactorings. Others will simply have to wait.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] oopexpert closed pull request #242: get a hands on ...
Posted by GitBox <gi...@apache.org>.
oopexpert closed pull request #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[GitHub] [tomcat] oopexpert edited a comment on issue #242: get a hands on
...
Posted by GitBox <gi...@apache.org>.
oopexpert edited a comment on issue #242: get a hands on ...
URL: https://github.com/apache/tomcat/pull/242#issuecomment-586642121
> If Im not mistaken and if it helps, you can cache the method after the first call then cost will be negligible.
I could but it would change the assertion of the "allow trace" evaluation. I tried to keep the assertions as they were during my refactoring. Who knows which methods rely on a non-cached "allow trace evaluation"?
> Very worse case you make a catalina class implementing Function<HttpServletRequest, xxx> or equivalent in catalina and instantiate it at first call in the servlet (these classes are available) by reflection then it is native code.
Please rephrase it. I do not understand what you mean.
> So we are not yet at java limitations IMHO.
I never said that. I only see no problem with the performance 18% performance decrease. This is because...
... reflection calls are the crucial parts which were already part of the previous implementations
... the doOption() method will surely never be called 10 million times in 4 seconds
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org