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

Fieldmap length and checksum optimization #42

Merged
merged 7 commits into from
May 9, 2018

Conversation

charlesbr1
Copy link
Contributor

Avoided some temporary IntField creation to compute length.
In fact, the "isStringEquivalent" branch that I added is useless with previous pull request on Message class, which directly compute length & checksum if possible.
Change some 'for each' list iteration by old loop over index to avoid iterator allocations, as the jvm will not always do it for us.

A cache for strings of frequently used integers
Avoided some temporary IntField creation to compute length.
In fact, the "isStringEquivalent" branch that I added is useless with previous commit on Message class, which directly compute length & checksum if possible.
Change some 'for each' list iteration by old loop over index to avoid iterator allocations, as the jvm will not always do it for us.
@philipwhiuk
Copy link
Contributor

NumbersCache is a bit of a memory/performance trade-off right? This is to do what - convert 9 into "9"?

What's the performance of the cache vs the performance of Integer.toString()?
What's the size of the cache. For the small one it's 2 bytes and 48 bytes for the string. Gauss's trick tells us there's ~500K of those so that's 25MB of data.

1000000 also seems arbitrary and high. The common numbers we see are going to be the fields themselves. Every order of magnitude dropped saves you over 90% of memory size (1 character less in the String and 90% less objects total).

@charlesbr1
Copy link
Contributor Author

charlesbr1 commented Nov 7, 2017 via email

@chrjohn chrjohn added the TODO label Mar 15, 2018
@chrjohn
Copy link
Member

chrjohn commented Mar 15, 2018

Hi @charlesbr1 , @philipwhiuk ,

do you think the big numbers cache makes sense?
Having all numbers from 0 to 999999 may make sense since the cache then contains every tag number, seqnum (unless you send really large numbers of messages), checksum and length that is going to be used. However, the values on the FIX messages (e.g. price, qty) are often floating point values so I am not really convinced to use it. Or maybe I am missing something?

Reducing the cached numbers to 99999 would probably be sufficient, too.

Thanks,
Chris.

@charlesbr1
Copy link
Contributor Author

charlesbr1 commented Mar 15, 2018 via email

@chrjohn
Copy link
Member

chrjohn commented Mar 29, 2018

Hi @charlesbr1 , would it be possible to grant me permission to push to the PR branches of your PRs?

  • E.g. on Allocations optimization for sfl4j logs #35 I needed to make changes to the unit test but had to merge the PR on the command line eventually.
  • From Add a reset() method in Message #40 I created a new PR from your fork so I could build via Travis CI.
    But in both cases the commit looks like I authored the changes. :-/ If you grant me the permission I can make changes to the PRs and you would still be listed as the author of the commit.

Thanks,
Chris.

@charlesbr1
Copy link
Contributor Author

charlesbr1 commented Mar 30, 2018 via email

@chrjohn
Copy link
Member

chrjohn commented Mar 30, 2018

Hi @charlesbr1
looking good now, at least github says "Add more commits by pushing to the fieldmap-length-and-checksum branch on charlesbr1/quickfixj.". That message was not present before.
Thanks!
Chris.

- changed NumbersCache to only hold numbers up to 99999 since it should be sufficient for normal use cases
- removed NumbersCache.get(double) method since its usage is questionable as it will truncate a double to a long value
@chrjohn chrjohn changed the title Fieldmap length and checksum optmization Fieldmap length and checksum optimization Apr 2, 2018
@chrjohn
Copy link
Member

chrjohn commented May 9, 2018

@chrjohn chrjohn added this to the QFJ 2.1.0 milestone May 9, 2018
@chrjohn chrjohn removed the TODO label May 9, 2018
@chrjohn chrjohn merged commit 9921529 into quickfix-j:master May 9, 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