Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CXF-8345] Improve performance by avoiding resize of MessageImpl #696

Merged
merged 1 commit into from
Sep 23, 2020

Conversation

andymc12
Copy link
Contributor

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

@andymc12 andymc12 self-assigned this Sep 21, 2020
@reta
Copy link
Member

reta commented Sep 22, 2020

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:

WDYT, does it make sense?

@andymc12 andymc12 changed the title Various fixes from the "Open Liberty Fork" [CXF-8345] Improve performance by avoiding resize of MessageImpl Sep 22, 2020
@andymc12
Copy link
Contributor Author

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@andymc12 andymc12 merged commit cb60a4d into apache:master Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants