You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by GitBox <gi...@apache.org> on 2020/09/21 21:46:06 UTC
[GitHub] [cxf] andymc12 opened a new pull request #696: Various fixes from the "Open Liberty Fork"
andymc12 opened a new pull request #696:
URL: https://github.com/apache/cxf/pull/696
Hi All,
While trying to consume the new 3.4.0 release into Open Liberty, we noticed that we had made a number of changes that has effectively forked Open Liberty's implementation of CXF. We'd like to contribute those back to Apache as much as possible.
This PR includes some of the changes that are easy to "disentangle" and include some fixes we needed to make in order to pass some JAX-RS TCK tests, improve performance (the change to MessageImpl in particular made a large difference by avoiding resizing the underlying data store), etc.
Unfortunately, many of our forked classes contains references to Open Liberty-specific code, so we'll need to sort those out before we can contribute them back. One thing we did in this PR to was to add an extension point (the new `RunnableWrapperFactory` class) so that we can avoid that.
Comments and questions welcome - and also, please let me know how to "record" these changes. I could open one JIRA for all of them, or separate them out by different areas (i.e. one for the MessageImpl performance change, one for the URITemplate change, etc.) so that it is clear to users what changed between releases.
Thanks in advance!
Andy
----------------------------------------------------------------
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
[GitHub] [cxf] reta edited a comment on pull request #696: Various fixes from the "Open Liberty Fork"
Posted by GitBox <gi...@apache.org>.
reta edited a comment on pull request #696:
URL: https://github.com/apache/cxf/pull/696#issuecomment-696442516
Thanks a lot for contribution, @andymc12 ! It is indeed very difficult to review since by and large those are unrelated changes, it would be much appreciated to have them splitted, may be by:
- TCK test fixes (we could refer to test suites directly, most recent build https://ci-builds.apache.org/job/CXF/job/CXF-JAXRS-TCK/66/artifact/JTreport/html/failed_gr.html)
- performance improvements
- other issues
WDYT, does it make sense?
----------------------------------------------------------------
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
[GitHub] [cxf] reta edited a comment on pull request #696: Various fixes from the "Open Liberty Fork"
Posted by GitBox <gi...@apache.org>.
reta edited a comment on pull request #696:
URL: https://github.com/apache/cxf/pull/696#issuecomment-696442516
Thanks a lot for contribution, @andymc12 ! It is indeed very difficult to review since by and large those are unrelated changes, it would be much appreciated to have them splitted, may be by:
- TCK test fixes (we could refer to test suites directly, most recent build https://ci-builds.apache.org/job/CXF/job/CXF-JAXRS-TCK/66/artifact/JTreport/html/failed_gr.html)
- performance improvements
- other issues
WDYT, does it make sense?
----------------------------------------------------------------
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
[GitHub] [cxf] andymc12 commented on pull request #696: [CXF-8345] Improve performance by avoiding resize of MessageImpl
Posted by GitBox <gi...@apache.org>.
andymc12 commented on pull request #696:
URL: https://github.com/apache/cxf/pull/696#issuecomment-696994681
Thanks @reta - I opened PRs #697 for the `hasEntity` TCK fixes and #698 for URITemplate and related processing changes. I'll plan to use this PR for the `MessageImpl` performance improvement.
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on a change in pull request #696: [CXF-8345] Improve performance by avoiding resize of MessageImpl
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #696:
URL: https://github.com/apache/cxf/pull/696#discussion_r493102424
##########
File path: core/src/main/java/org/apache/cxf/interceptor/ServiceInvokerInterceptor.java
##########
@@ -62,7 +62,7 @@ public void run() {
Message outMessage = runableEx.getOutMessage();
if (outMessage == null) {
- outMessage = new MessageImpl();
+ outMessage = new MessageImpl(16, 1); // perf: size 16 / factor 1 to avoid resize operation
Review comment:
Sorry @andymc12 , just of of curiosity, the MessageImpl is a HashMap and uses its default constructor, where capacity is set to `16` and load factor to `0.75`.
```
/**
* Constructs an empty <tt>HashMap</tt> with the default initial capacity
* (16) and the default load factor (0.75).
*/
public HashMap() {
this.loadFactor = DEFAULT_LOAD_FACTOR; // all other fields defaulted
}
```
You guys found out that load factor of `1` helps to eliminate unnecessary resizing (probably because the size of the message container is around 16 elements)?
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on a change in pull request #696: [CXF-8345] Improve performance by avoiding resize of MessageImpl
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #696:
URL: https://github.com/apache/cxf/pull/696#discussion_r493102424
##########
File path: core/src/main/java/org/apache/cxf/interceptor/ServiceInvokerInterceptor.java
##########
@@ -62,7 +62,7 @@ public void run() {
Message outMessage = runableEx.getOutMessage();
if (outMessage == null) {
- outMessage = new MessageImpl();
+ outMessage = new MessageImpl(16, 1); // perf: size 16 / factor 1 to avoid resize operation
Review comment:
Sorry @andymc12 , just of of curiosity, the MessageImpl is a HashMap and uses its default constructor, where capacity is set to `16` and load factor to `0.75`.
```
/**
* Constructs an empty <tt>HashMap</tt> with the default initial capacity
* (16) and the default load factor (0.75).
*/
public HashMap() {
this.loadFactor = DEFAULT_LOAD_FACTOR; // all other fields defaulted
}
```
You guys found out that load factor of `1` helps to eliminate unnecessary resizing (probably because the size of the message container is around 16 elements)?
----------------------------------------------------------------
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
[GitHub] [cxf] andymc12 commented on pull request #696: [CXF-8345] Improve performance by avoiding resize of MessageImpl
Posted by GitBox <gi...@apache.org>.
andymc12 commented on pull request #696:
URL: https://github.com/apache/cxf/pull/696#issuecomment-696994681
Thanks @reta - I opened PRs #697 for the `hasEntity` TCK fixes and #698 for URITemplate and related processing changes. I'll plan to use this PR for the `MessageImpl` performance improvement.
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on pull request #696: Various fixes from the "Open Liberty Fork"
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #696:
URL: https://github.com/apache/cxf/pull/696#issuecomment-696442516
Thanks a lot for contribution, @andymc12 ! It is indeed very difficult to review since by and large those are unrelated changes, it would be much appreciated to have them splitted, may be by:
- TCK test fixes (we could refer to test suites directly, most recent build https://ci-builds.apache.org/job/CXF/job/CXF-JAXRS-TCK/66/artifact/JTreport/html/failed_gr.html)
- performance improvements
- other issues
WDYT, does it make sense?
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on pull request #696: Various fixes from the "Open Liberty Fork"
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #696:
URL: https://github.com/apache/cxf/pull/696#issuecomment-696442516
Thanks a lot for contribution, @andymc12 ! It is indeed very difficult to review since by and large those are unrelated changes, it would be much appreciated to have them splitted, may be by:
- TCK test fixes (we could refer to test suites directly, most recent build https://ci-builds.apache.org/job/CXF/job/CXF-JAXRS-TCK/66/artifact/JTreport/html/failed_gr.html)
- performance improvements
- other issues
WDYT, does it make sense?
----------------------------------------------------------------
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
[GitHub] [cxf] andymc12 merged pull request #696: [CXF-8345] Improve performance by avoiding resize of MessageImpl
Posted by GitBox <gi...@apache.org>.
andymc12 merged pull request #696:
URL: https://github.com/apache/cxf/pull/696
----------------------------------------------------------------
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
[GitHub] [cxf] reta commented on pull request #696: Various fixes from the "Open Liberty Fork"
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #696:
URL: https://github.com/apache/cxf/pull/696#issuecomment-696442516
Thanks a lot for contribution, @andymc12 ! It is indeed very difficult to review since by and large those are unrelated changes, it would be much appreciated to have them splitted, may be by:
- TCK test fixes (we could refer to test suites directly, most recent build https://ci-builds.apache.org/job/CXF/job/CXF-JAXRS-TCK/66/artifact/JTreport/html/failed_gr.html)
- performance improvements
- other issues
WDYT, does it make sense?
----------------------------------------------------------------
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
[GitHub] [cxf] reta edited a comment on pull request #696: Various fixes from the "Open Liberty Fork"
Posted by GitBox <gi...@apache.org>.
reta edited a comment on pull request #696:
URL: https://github.com/apache/cxf/pull/696#issuecomment-696442516
Thanks a lot for contribution, @andymc12 ! It is indeed very difficult to review since by and large those are unrelated changes, it would be much appreciated to have them splitted, may be by:
- TCK test fixes (we could refer to test suites directly, most recent build https://ci-builds.apache.org/job/CXF/job/CXF-JAXRS-TCK/66/artifact/JTreport/html/failed_gr.html)
- performance improvements
- other issues
WDYT, does it make sense?
----------------------------------------------------------------
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
[GitHub] [cxf] andymc12 commented on a change in pull request #696: [CXF-8345] Improve performance by avoiding resize of MessageImpl
Posted by GitBox <gi...@apache.org>.
andymc12 commented on a change in pull request #696:
URL: https://github.com/apache/cxf/pull/696#discussion_r493676081
##########
File path: core/src/main/java/org/apache/cxf/interceptor/ServiceInvokerInterceptor.java
##########
@@ -62,7 +62,7 @@ public void run() {
Message outMessage = runableEx.getOutMessage();
if (outMessage == null) {
- outMessage = new MessageImpl();
+ outMessage = new MessageImpl(16, 1); // perf: size 16 / factor 1 to avoid resize operation
Review comment:
Yes. It's been a while since we made this change, but I know the performance team tried a few different options and found 16/1 to be the best combination based on the benchmarks they were running. But I agree with your assessment - this combination is best where there are between 12-16 elements - or rather, it avoids resize between 0-16, but it performs better than the default when where are between 12-16 elements.
Thanks for the review!
----------------------------------------------------------------
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