Skip to content

MYFACES-4239: Multiple performance improvements#8

Merged
pnicolucci merged 1 commit intoapache:masterfrom
pnicolucci:MYFACES-4239
Jun 26, 2018
Merged

MYFACES-4239: Multiple performance improvements#8
pnicolucci merged 1 commit intoapache:masterfrom
pnicolucci:MYFACES-4239

Conversation

@pnicolucci
Copy link
Contributor

No description provided.

@pnicolucci pnicolucci self-assigned this Jun 22, 2018
@pnicolucci pnicolucci requested a review from ebreijo June 22, 2018 13:57
@tandraschko
Copy link
Member

tandraschko commented Jun 22, 2018

@pnicolucci i'm not sure about this changes out.write vs sb.append...

Why do you switchted from out.write to a new StringBuilder?
You know, this are always new object instances... And we have something like a SharedStringBuilder

@pnicolucci
Copy link
Contributor Author

@tandraschko , we've been running some performance tests and can see a .5 - 1% increase in our testing in performance with an application that is only about 15% JSF , with a larger application and more JSF within it , the gain could be even greater. Basically we want to avoid the writer.write path , it is quite a long path to call over and over again if it isn't necessary.

@tandraschko
Copy link
Member

ok, i see
i'm ok for it if you use the SharedStringBuilder

@tandraschko
Copy link
Member

tandraschko commented Jun 22, 2018

JFYI: we avoided new objects so much in the past that we even switched from for-each to for loops, so the SharedStringBuilder is IMO a must-have for this change, also those methods are called really often

@tandraschko
Copy link
Member

All other changes looks fine of course.

@pnicolucci
Copy link
Contributor Author

I'll refactor a bit to use the SharedStringBuilder. Thanks for the information / review!

@tandraschko
Copy link
Member

I also thought a bit about it, i think it would be better if we discuss it first on the mailing list.
And details about it would be great. You know, we already have a buffer.

So could you maybe split the sb/out commits from the other improvements?

@pnicolucci
Copy link
Contributor Author

I'll work to remove the sb changes from this PR and will open a new issue / PR for the changes and get a discussion started on the mailing list.

Copy link
Member

@ebreijo ebreijo left a comment

Choose a reason for hiding this comment

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

Changes look good to me.

@pnicolucci
Copy link
Contributor Author

@tandraschko ok with the current changes in this PR?

@tandraschko
Copy link
Member

Yep! :)

@pnicolucci pnicolucci merged commit 4619df6 into apache:master Jun 26, 2018
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.

3 participants