optimize DoubleConverter

[leonchen83]

Benchmark Mode Cnt Score Error Units
DoubleConverterBenchmark.measureFast thrpt 10 1204994.113 ± 11444.945 ops/s
DoubleConverterBenchmark.measureRaw thrpt 10 575634.679 ± 6665.589 ops/s

[chrjohn]

Thanks for the PR. Would it be possible to see the code for the benchmark?
Also, there is #34 that I didn't yet get around to merge. It also contains a DoubleConverter optimization. Maybe you can run the same benchmark on it?
Thanks,
Chris.

[leonchen83]

pls ignore this pr, I will fix a bug in this pr. and will also upload benchmark file

[leonchen83]

Benchmark Mode Cnt Score Error Units
DoubleConverterBenchmark.measureFast thrpt 10 1204420.985 ± 39386.606 ops/s
DoubleConverterBenchmark.measurePR34 thrpt 10 1369704.973 ± 4725.402 ops/s
DoubleConverterBenchmark.measureRaw thrpt 10 602547.907 ± 4138.975 ops/s

import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.runner.Runner;
import org.openjdk.jmh.runner.RunnerException;
import org.openjdk.jmh.runner.options.Options;
import org.openjdk.jmh.runner.options.OptionsBuilder;
import quickfix.FieldConvertError;
import quickfix.field.converter.DoubleConverter;

/**
 * Created by Baoyi Chen on 2017/1/25.
 */
public class DoubleConverterBenchmark {
    private static final String[] DATASET = new String[]{"0",".0",".1","1234","0987654321","1234567890.0987654321","-0","-1234567890.0987654321","-.0"};

    @Benchmark
    public void measureRaw() throws Exception {
        for(String str : DATASET){
            DoubleConverter.convert(str);
        }
    }

    @Benchmark
    public void measureFast() throws Exception {
        for(String str : DATASET){
        	FastDoubleConverter.convert(str);
        }
    }

    @Benchmark
    public void measurePR34() throws Exception {
        for(String str : DATASET){
            PR34DoubleConverter.convert(str);
        }
    }

    public static void main(String[] args) throws RunnerException, FieldConvertError {
        Options opt = new OptionsBuilder()
                .include(DoubleConverterBenchmark.class.getSimpleName())
                .warmupIterations(5)
                .measurementIterations(10)
                .shouldDoGC(false)
                .forks(1)
                .build();

        new Runner(opt).run();
    }
}

[leonchen83]

I already updated this pr , please check

[MartyIX]

It is my understanding that your code is 10 times faster. Is there a spec for FIX double values?

I understand that your implementation is faster because you don't trim the input string, you don't handle hex numbers, etc. Am I correct?

[chrjohn]

Hi @MartyIX ,
here should be the definition of FIX double values (float): http://www.fixtradingcommunity.org/FIXimate/FIXimate3.0/en/FIX.5.0SP2/fix_datatypes.html

[leonchen83]

Hi @MartyIX I just convert -?\d*(\.\d*)? to parseDouble method

[MartyIX]

@leonchen83 I believe this PR should add a unit test too. To verify that it follows the spec:

Sequence of digits with optional decimal point and sign character (ASCII characters "-", "0" - "9" and "."); the absence of the decimal point within the string will be interpreted as the float representation of an integer value. All float fields must accommodate up to fifteen significant digits. The number of decimal places used should be a factor of business/market needs and mutual agreement between counterparties. Note that float values may contain leading zeros (e.g. "00023.23" = "23.23") and may contain or omit trailing zeros after the decimal point (e.g. "23.0" = "23.0000" = "23" = "23.").
Note that fields which are derived from float may contain negative values unless explicitly specified otherwise. The following data types are based on float.

@chrjohn What do you think about the PR? Do you believe that it is backward compatible?

[chrjohn]

Hi @MartyIX ,
from briefly looking at the code it seems fine to me. We already have a unit test which should cover (hopefully ;)) all aspects: https://github.com/quickfix-j/quickfixj/blob/master/quickfixj-core/src/test/java/quickfix/FieldConvertersTest.java method testDoubleConversion()

[leonchen83]

found a bug about IntConverter.convert, more detail pls see test case in this pr

[chrjohn]

@leonchen83 Could you elaborate on the problem? 100 looks like an Integer to me ;)
Thanks

[leonchen83]

according to int spec

Sequence of digits without commas or decimals and optional sign character (ASCII characters "-" and "0" - "9" ). The sign character utilizes one byte (i.e. positive int is "99999" while negative int is "-99999"). Note that int values may contain leading zeros (e.g. "00023" = "23").
Examples:
723 in field 21 would be mapped int as |21=723|.
-723 in field 12 would be mapped int as |12=-723|
The following data types are based on int.

but IntConverter.convert method can accept a full width character e.g. １００

[leonchen83]

the root cause is Character.isDigit can accept a full width character

[chrjohn]

Sounds OK to me. I don't think we should change the implementation and leave it based on what Character.isDigit accepts.

[leonchen83]

so you mean ६७२ is a legal number?
IntConverter.convert("६७२")

[chrjohn]

Let me ask this way: is there a benefit to introduce a restriction to accept only ISO-LATIN digits in that method?

[leonchen83]

got the point ,thx @chrjohn

[chrjohn]

No problem, thanks for the PR. I'd like to accept it with the DoubleConverter stuff that you did.