perf: avoid autoboxing bind indexes

[bokken]

The parser currently builds a List<Integer> (requires boxing) to track bind variables. It then converts this to an int[] (requires unboxing) when creating the NativeQuery.
This change provides a simple collection backed by an int[].

[codecov-io]

Codecov Report

Merging #1244 into master will increase coverage by 0.02%.
The diff coverage is 96%.

@@             Coverage Diff              @@
##             master    #1244      +/-   ##
============================================
+ Coverage     68.78%   68.81%   +0.02%     
+ Complexity     3908     3905       -3     
============================================
  Files           179      179              
  Lines         16420    16436      +16     
  Branches       2674     2677       +3     
============================================
+ Hits          11295    11310      +15     
  Misses         3880     3880              
- Partials       1245     1246       +1

[bokken]

@vlsi, any consideration for including this in 42.2.3 also? It is along the lines of the other performance improvements (reduced memory consumption and execution time) in the Parser class.

[davecramer]

Is there a compelling reason to add it now ? I guess my thought is that we have to cut off changes and release at some point otherwise we will never get it out the door

…

On Tue, Jul 10, 2018, 2:17 PM Brett Okken, ***@***.***> wrote: @vlsi <https://github.com/vlsi>, any consideration for including this in 42.2.3 also? It is along the lines of the other performance improvements (reduced memory consumption and execution time) in the Parser class. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1244 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYz9nt1aX3zFuwF96SZqWVGnzPxkJvSks5uFO_AgaJpZM4VFX-I> .

[vlsi]

1. +1 to @davecramer
2. Even though this looks like an improvement, I would assume it would be hard to notice/measure. Do you have numbers to compare?
3. The PR introduces a new List implementation, and there might be bugs there
4. There are merge conflicts
5. Merging this would require me to update Release notes for 42.2.3 #1238 (and I would like to avoid excessive paperwork)

[bokken]

Is there a compelling reason to add it now?

Simply that it is is line with other optimizations in parsing. Not super compelling.

Even though this looks like an improvement, I would assume it would be hard to notice/measure. Do you have numbers to compare?

It is certainly a small improvement. On the order of 10-100ns for 10 or fewer bind variables. It appears to be (very roughly) 10ns per bind variable. Even at 1000 bind variables it is only 1 microsecond. I am not certain how to get the memory allocation data out of JMH. I believe that in an 64bit jvm an int[] will be less memory than a same-sized Object[]. But again, the difference should be rather small.

Benchmark           (arraySize)  Mode  Cnt      Score     Error  Units
IntBenchmark.array            1  avgt   10     41.770 ±   1.285  ns/op
IntBenchmark.array            2  avgt   10     48.328 ±   1.675  ns/op
IntBenchmark.array            3  avgt   10     50.830 ±   1.005  ns/op
IntBenchmark.array            5  avgt   10     65.940 ±   2.814  ns/op
IntBenchmark.array            7  avgt   10     66.632 ±   1.846  ns/op
IntBenchmark.array          100  avgt   10   1015.309 ±  12.135  ns/op
IntBenchmark.array         1000  avgt   10   9843.775 ± 428.035  ns/op
IntBenchmark.list             1  avgt   10     53.714 ±   1.347  ns/op
IntBenchmark.list             2  avgt   10     69.260 ±   2.323  ns/op
IntBenchmark.list             3  avgt   10     83.542 ±   1.790  ns/op
IntBenchmark.list             5  avgt   10    122.491 ±   3.921  ns/op
IntBenchmark.list             7  avgt   10    156.619 ±   3.835  ns/op
IntBenchmark.list           100  avgt   10   1612.699 ±  49.838  ns/op
IntBenchmark.list          1000  avgt   10  20691.498 ± 320.367  ns/op

The PR introduces a new List implementation, and there might be bugs there

It is not an actual List implementation. It only implements a very few methods and is really quite simple. There is also a unit test which covers the class pretty thoroughly.

There are merge conflicts

Appears to be in the TestCase adding reference to the new junit test. This is easily remedied.

[vlsi]

I am not certain how to get the memory allocation data out of JMH

-prof gc

[vlsi]

@bokken , is this "overflow conscious code"?

[bokken]

it should be overflow safe enough. the largest this safely supports is an array with ~1.4 billion elements. As of java 9 or 10 (when compact string project implemented), a String can only have 1 billion characters in it.

[vlsi]

It is not an actual List implementation. It only implements a very few methods and is really quite simple

Well, the reality with pgjdbc development is there's a lot of options to defeat client applications by innocent changes to the driver code.

Would you share IntBenchmark as well?

[bokken]

@Fork(value = 1, jvmArgsPrepend = "-Xmx128m")
@Measurement(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS)
@Warmup(iterations = 10, time = 1, timeUnit = TimeUnit.SECONDS)
@State(Scope.Thread)
@BenchmarkMode(Mode.AverageTime)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Threads(value = 2)
public class IntBenchmark {
  static final int[] EMPTY_INT_ARRAY = new int[0];

  private static final int[] VALUES = new int[] { 35, 37, 235, 1237, 1890, 134235, 259735 };

  @Param({ "1", "2", "3", "5", "7", "100", "1000" })
  int arraySize;

  @Benchmark
  public int[] list() {
    final List<Integer> list = new ArrayList<Integer>();
    for (int i = 0; i < arraySize; ++i) {
      if (i < VALUES.length) {
        list.add(VALUES[i]);
      } else {
        list.add(i);
      }
    }
    final int[] ints = new int[list.size()];
    for (int i = 0; i < list.size(); ++i) {
      ints[i] = list.get(i);
    }
    return ints;
  }

  @Benchmark
  public int[] array() {
    final IntCollection list = new IntCollection();
    for (int i = 0; i < arraySize; ++i) {
      if (i < VALUES.length) {
        list.add(VALUES[i]);
      } else {
        list.add(i);
      }
    }
    return list.toArray();
  }

  static final class IntCollection {
    private int[] ints = new int[8];
    private int index = 0;

    IntCollection() {
    }

    public void add(int i) {
      if (index >= ints.length) {
        // double in size until 1024 in size, then grow by 1.5x
        final int newLength = ints.length < 1024 ? ints.length << 1 : (ints.length + (ints.length >> 1));
        ints = Arrays.copyOf(ints, newLength);
      }
      ints[index++] = i;
    }

    public int size() {
      return index;
    }

    public int get(int i) {
      if (i < 0 || i >= index) {
        throw new ArrayIndexOutOfBoundsException(i);
      }
      return ints[i];
    }

    public void clear() {
      index = 0;
    }

    public int[] toArray() {
      if (index == 0) {
        return EMPTY_INT_ARRAY;
      }
      return Arrays.copyOf(ints, index);
    }
  }
}

[bokken]

Memory impacts are very small as long as the value (index into sql length of the bind variable) is < 128 (cached Integer values). Then it becomes ~16 bytes per bind variable.

Benchmark                                         (arraySize)  Mode  Cnt      Score     Error   Units
IntBenchmark.array                                          1  avgt   10     46.611 ±   1.907   ns/op
IntBenchmark.array:·gc.alloc.rate.norm                      1  avgt   10     82.079 ±   0.012    B/op
IntBenchmark.array                                          2  avgt   10     55.315 ±   3.662   ns/op
IntBenchmark.array:·gc.alloc.rate.norm                      2  avgt   10     90.288 ±   0.005    B/op
IntBenchmark.array                                          3  avgt   10     61.444 ±   3.072   ns/op
IntBenchmark.array:·gc.alloc.rate.norm                      3  avgt   10     90.290 ±   0.006    B/op
IntBenchmark.array                                          5  avgt   10     65.888 ±   2.854   ns/op
IntBenchmark.array:·gc.alloc.rate.norm                      5  avgt   10     98.496 ±   0.004    B/op
IntBenchmark.array                                          7  avgt   10     75.768 ±   4.652   ns/op
IntBenchmark.array:·gc.alloc.rate.norm                      7  avgt   10    106.703 ±   0.007    B/op
IntBenchmark.array                                        100  avgt   10    873.054 ±  49.160   ns/op
IntBenchmark.array:·gc.alloc.rate.norm                    100  avgt   10   1541.574 ±   1.884    B/op
IntBenchmark.array                                       1000  avgt   10   7337.156 ± 469.733   ns/op
IntBenchmark.array:·gc.alloc.rate.norm                   1000  avgt   10  12553.850 ±   5.596    B/op
IntBenchmark.list                                           1  avgt   10     61.189 ±   1.773   ns/op
IntBenchmark.list:·gc.alloc.rate.norm                       1  avgt   10     98.501 ±   0.004    B/op
IntBenchmark.list                                           2  avgt   10     76.633 ±   4.259   ns/op
IntBenchmark.list:·gc.alloc.rate.norm                       2  avgt   10    106.706 ±   0.005    B/op
IntBenchmark.list                                           3  avgt   10     98.872 ±   3.256   ns/op
IntBenchmark.list:·gc.alloc.rate.norm                       3  avgt   10    123.122 ±   0.005    B/op
IntBenchmark.list                                           5  avgt   10    141.584 ±   2.057   ns/op
IntBenchmark.list:·gc.alloc.rate.norm                       5  avgt   10    164.156 ±   0.017    B/op
IntBenchmark.list                                           7  avgt   10    170.976 ±   3.165   ns/op
IntBenchmark.list:·gc.alloc.rate.norm                       7  avgt   10    205.190 ±   0.013    B/op
IntBenchmark.list                                         100  avgt   10   1800.544 ± 114.141   ns/op
IntBenchmark.list:·gc.alloc.rate.norm                     100  avgt   10   1901.250 ±   3.108    B/op
IntBenchmark.list                                        1000  avgt   10  23176.933 ± 562.064   ns/op
IntBenchmark.list:·gc.alloc.rate.norm                    1000  avgt   10  33644.062 ±  14.218    B/op

[bokken]

It is not an actual List implementation. It only implements a very few methods and is really quite simple

Well, the reality with pgjdbc development is there's a lot of options to defeat client applications by innocent changes to the driver code.

My point was that a List implementation can be quite complicated with iterators, spliterators, sublists, removes, etc. This class is quite simple because it only needs to be able to add, retrieve, clear, and convert to an int[].
I certainly do not want to introduce instability to consumers.

[vlsi]

My point was that a List implementation can be quite complicated with iterators, spliterators, sublists, removes, etc

I perfectly get that.

This class is quite simple because it only needs to be able to add, retrieve, clear, and convert to an int[].

The thing is the class is "new" and one should analyze failure cases for the new class.

[vlsi]

size is missing in the message

[vlsi]

The diff looks odd

[bokken]

I agree. I tried to rebase (as you had suggested earlier) and all of a sudden your commit of the CommandComplete stuff showed up.
I am accustomed to doing merges rather than rebases, so it is quite likely I have simply done something wrong. However, I have no idea how to undo it now.

[vlsi]

This is what helps me in similar cases: https://gist.github.com/iansheridan/870778/28f1a5a2b35a14eea5cac671d126b092e8448d9c#info

git reflog Use this to recover from major fuck ups!

[bokken]

@vlsi, I think this might be a github flub. It looks correct from my fork:
master...bokken:int_collection

[vlsi]

Here's what I have with ParseStatement.bindExecuteFetch benchmark (see https://github.com/pgjdbc/benchmarks/blob/436694736a342d39864704c5e892f9b237e0dd4f/src/main/java/org/postgresql/benchmark/statement/ParseStatement.java#L95 ):

Note that ±Error is quite high for response times, thus only allocation difference is actually significant here (Java 1.8u152, PostgreSQL at the localhost):

Benchmark   (bindCount)  (unique)  Mode  Cnt  master   Error  PR1244   Error   Units
time                  1      true  avgt   10      60 ±     4      61 ±     2   us/op
allocation            1      true  avgt   10    2152 ±     0    2144 ±     0    B/op
time                100      true  avgt   10     278 ±    22     276 ±    15   us/op
allocation          100      true  avgt   10   32760 ±     1   31384 ±     1    B/op
time               1000      true  avgt   10    2509 ±   123    2468 ±   154   us/op
allocation         1000      true  avgt   10  353775 ±  1563  331785 ±  1467    B/op

For the fun sake I've tested unique=false (==reuse SQL statement) in a pgjdbc master branch:

Benchmark   (bindCount)  (unique)  Mode  Cnt  Score   Error   Units
time                  1     false  avgt   10     54 ±     1   us/op
allocation            1     false  avgt   10    984 ±     0    B/op
time                100     false  avgt   10    116 ±     3   us/op
allocation          100     false  avgt   10   7400 ±     0    B/op
time               1000     false  avgt   10    805 ±    19   us/op
allocation         1000     false  avgt   10  79865 ±     4    B/op

Of course we want to eliminate excessive resource usage in the driver, however proper use of SQL (at the application side) results in much better improvement.

[davecramer]

So from what I am reading this is a very minor improvement?

[bokken]

It is a minor improvement. Per sql statement parsed, it is ~10ns per bind variable and ~16 bytes per bind variable at position > 128 in the "native" sql satement.
Yes, having constant sql that remains in statement cache is even better.

What is the threshold for performance improvements to be interesting? While this benefit is small, the change is also quite small.

[davecramer]

There is no "threshold" really. I had no issue with the PR, I took issue with the timing and the tangent that it led us down while we were trying to get 42.2.3 out the door.

I think this is going to prompt the project to become more organized about what goes in and when and if something gets pushed to the next release. Any thoughts around how to accomplish that ? The core postgreSQL project has a notion of commit fests. They even have an app for what gets in and what stays in. https://commitfest.postgresql.org/19/

[bokken]

I would be a big fan of having regularly scheduled releases.

[davecramer]

I'm in the process of setting up projects hopefully this will help.

some thoughts:

We have to specify what is going into a release and not allow creep .

We need to get ruthless about getting feedback. If we request it and don't receive it then the PR gets moved to the next release.

Aging a PR. After a while it will get moved to the ice box. I'll setup a project for this. This is usually where PR's go to die.

Ideally someone would pick it up and work on it.

Thoughts ?

[sehrope]

We have to specify what is going into a release and not allow creep .

+1 to regularly scheduled releases. It's helpful both from the perspective of contributors and users. Contributors get to know that once a PR is merged then they'll have have their PR in the core released driver at a fixed time. Users get the benefit of having a schedule of when they should be following up on driver updates that hopefully they can tie into their software lifecycle.

We need to get ruthless about getting feedback. If we request it and don't receive it then the PR gets moved to the next release.

I cycle in and out but would happen to be called out if reviewers are needed for something. I've noticed that most of the things I'd chime in on have already been closed out by the time I follow up so quieter than usual lately (and I'm generally on the side of simpler code vs. squeezing perf gains :)).

Aging a PR. After a while it will get moved to the ice box. I'll setup a project for this. This is usually where PR's go to die.

+1 to this as well. Nobody wants PRs hanging around forever. Ditto for issues. Having the open set reflect what's active would help existing and new contributors focus there efforts on what people actually want. And if something does get booted the original author can always re-submit, hopefully after lining up a reviewer.

[AppVeyorBot]

✅ Build pgjdbc 1.0.226 completed (commit 917e7110d1 by @bokken)

[codecov-commenter]

Codecov Report

Merging #1244 into master will decrease coverage by 0.67%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1244      +/-   ##
============================================
- Coverage     69.21%   68.53%   -0.68%     
+ Complexity     4193     4163      -30     
============================================
  Files           197      197              
  Lines         17996    18010      +14     
  Branches       2916     2919       +3     
============================================
- Hits          12456    12344     -112     
- Misses         4188     4329     +141     
+ Partials       1352     1337      -15     

[pwagland]

I know that the performance improvements in this PR are on the edge, but are there any plans to reinvigorate them?

[davecramer]

@pwagland curious why this interests you given the small performance benefits. Are there other benefits ?

[pwagland]

Thanks for the quick answer @davecramer!

There is nothing specific that we want this for, I actually found it while looking for another issue (#1311) where we add potentially tens of thousands of parameters. This PR is not the thing that makes this slow! :-)

One of the things that we are looking at is how to minimise the number of allocations that we make, this would also not really make much difference there, it will just do a smaller generation in 99.9% of our queries, since Integer.valueOf() returns the cached values for "small" offsets, and we rarely have more than ten parameters, let alone, more than 128.

I was just curious, since in #1244 (comment) there was a discussion about what to do with old PR's which this should be considered now I guess? And #1244 (comment) kind of implied that the PR was mostly OK, so I'm not quite sure what else would be needed for this PR, from there the question.

[vlsi]

As we are on Java 8+, should we consider using java.util.stream.IntStream#builder() instead of coding our own int-optimized int array builder?

[vlsi]

Ah, it seems that IntStream#builder().add().stream().toIntArray() would incur significant overhead :-(

[vlsi]

I guess it should be IntList to enforce it keeps the order of elements, and it could probably move to a top-level.
IntList would be helpful for #1946

[vlsi]

@bokken , I've resurrected the PR. I think it should be good to merge now.

Any thoughts, comments?

[bokken]

This is a blast from the past. Changes look good to me.

[panchenko]

It's actually size.
It would be good to rename ensureSize(int size) too.