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

Allocations optimization for converters #34

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

charlesbr1
Copy link
Contributor

Made use of cached string values & thread local buffers to avoid frequent allocations.

Parser for CharSequence to double
Made use of cached char values
Made use of cached values for double that are integers, removed costly regexp pattern checker.
Made use of cached values
Made use of reusable buffers
Made use of reusable buffer
Made use of reusable buffer, removed costly String::substring calls
charlesbr1 added a commit to charlesbr1/quickfixj that referenced this pull request Jul 31, 2015
See pull request
Allocations optimization for converters quickfix-j#34
charlesbr1 added a commit to charlesbr1/quickfixj that referenced this pull request Jul 31, 2015
See pull request
Allocations optimization for converters quickfix-j#34
charlesbr1 added a commit to charlesbr1/quickfixj that referenced this pull request Jul 31, 2015
See pull request
Allocations optimization for converters quickfix-j#34
charlesbr1 added a commit to charlesbr1/quickfixj that referenced this pull request Jul 31, 2015
See pull request
Allocations optimization for converters quickfix-j#34
@chrjohn
Copy link
Member

chrjohn commented Dec 12, 2015

Thanks for the PR (or actually the PRs ;))
Just a question regarding the ThreadLocal Context objects. Why are you using StringBuffer instead of StringBuilder? Due to the ThreadLocal implementation only one thread should access it, shouldn't it?
Thanks,
Chris.

@charlesbr1
Copy link
Contributor Author

Hello,

Regarding QuickFix, I may have a fix to commit because of a regression on my changes, regarding double to String conversion, for DoubleField. May be, I have to check what kind of code I gave you for this specific part because the DoubleField optimization is the only piece that I didn’t took from the code I wrote in my work... In prod I made a mistake and we converted double values to string using String.valueOf(), but a NumberFormat must be used instead.
Second point is to put some new parameters in conf’, as you said, for size and number of buffers to use with the mina sending thread, but I didn’t took time to look how to do it, for the moment.

For you question, I obviously know the difference between StringBuffer / Builder, I use the Builder when I can. Where did you see that I use a Buffer ? If I did it there is likely a reason, certainly the code below is not compliant with Builder but only Buffer. This is the case for DateTIme formatting, from what I remember.
We can avoid one allocation by calling this kind of method, which is only Buffer compliant :

public abstract StringBuffer format(Date date, StringBuffer toAppendTo,
FieldPosition fieldPosition);

There is something you have to know with my code there, the FieldPosition argument give my difficulties. I wanted to give the same one by default as the JDK do, but this is not an instance accessible without introspection. And we cannot build the same one as the JDK do because of accessibility restrictions. But the one I one use seems to be working fine :

private static final class DontCareFieldPosition extends FieldPosition {
// The singleton of DontCareFieldPosition.
static final FieldPosition INSTANCE = new DontCareFieldPosition();

private DontCareFieldPosition() {
    super(0);
}

}
If you know this aspect of Java…

Regards,

Charles Briquel

Le 12 déc. 2015 à 18:33, Christoph John [email protected] a écrit :

Thanks for the PR (or actually the PRs ;))
Just a question regarding the ThreadLocal Context objects. Why are you using StringBuffer instead of StringBuilder? Due to the ThreadLocal implementation only one thread should access it, shouldn't it?
Thanks,
Chris.


Reply to this email directly or view it on GitHub #34 (comment).

@charlesbr1
Copy link
Contributor Author

We can use reflexion to resolve Builder issue and avoid a copy of date between StringBuffer - StringBuilder in
DateConverters 'public static void convert(Date d, StringBuilder stringBuilder)' methods. Without having to rewrite the JDK or depends on an external lib.
But it is not justified there.
Furthermore, with Lock Coarsening StringBuffer sync overhead should be removed, but it's still better to drop the potential glitch at the source.
So I think it's ok like that.

@chrjohn chrjohn mentioned this pull request Jan 25, 2017
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