FWD » Core Development » Database

The same issue we have in FWD, P2JField.computeHash. Please use hashCodeCaseInsensitive there, instead of .toLowerCase().hashCode().

One last comment here. If you already plan to do some changes to ValueStringIgnoreCase, also consider optimizing ValueStringIgnoreCase.get. This is called thousand of times and pops up in the profiler.

• H2 does some caching on String instances (much like Java does with String Pool). However, StringUtils.cache works as a fixed-size String Pool with eviction. This way, H2 will help the performance by recycling the same String instances as much as possible. If we "play" with the same String instances over and over again, then a == b is more probable in equality checks, so a.equals(b) is faster.
• H2 does the same "caching" technique for ValueStringIgnoreCase using Value.cache. This cache allows us to recycle ValueStringIgnoreCase instances if they store the same String.

This whole caching overhead may take some time to execute. My concern is that ValueStringIgnoreCase will hit the cache case-insensitively (as ValueStringIgnoreCase.equals is case-insensitive); so "aBc" will match the cached "abc". For this matter, H2 will do a second equals check to see if the cached entry has the same case as the look-up key. This basically means two equals.

• Easy, but slower: do the final equality check between cache.value and obj.value (instead of s) as obj.value has a higher chance to be a cached string, so .equals may be faster
• Hash, but faster: change/overload Value.cache to extract from the cache case-sensitively when ValueStringIgnoreCase is used. Basically, check in Value.cache the type of the operands (look-up key / cache entry) and if they are both STRING_IGNORECASE, cast to ValueStringIgnoreCase and do something like equalsCaseSensitive. This will avoid the second equals in ValueStringIgnoreCase.

I changed toLowerCase().hashCode() into StringHelper.hashCodeCaseInsensitive() in P2JField.computeHash().

Commited to 7026d revision 14575.

Ștefan, please provide a (comprehensive) report on your progress here. Include what have been implemented and what is left. Please share here any serious impediments on the way, if any.
I would also like to have a testing and custom profiling round at the end. That is, run testFWD and a large customer application at least.

• every value type from H2
• if it uses the value cache (Value.cache). If yes, if you've already refactored it / going to refactor it / can't refactor it due to a logical constraint.
• number of new calls in <value-type>.get. For each call, you can tell if it can be optimized or not.
• if it overrides lowLevelEquals: already written the code for it / planning to do so / can't use a low level implementation here.
• what hashing are you using for the cache look-up: the same hash technique as for the value / a custom different hash of the low-level value / can't compute the hash over a low level value to use with the cache.

You snapshot your work by now in this table. Edit it as you implement further.

• Less objects allocated on the heap in total (check with VisualVM).
• Faster execution time due to faster cache look-up (check with System.nanoTime).
• Correct results: make sure you end up with a proper result in regard to your expectation.
• A complete profiling report for each value on the table above. Thus, track how much time was spent before / after your changes in each <value-type>.get. This should go in an extra columns of the table: Time before, Time after, Diff.

Make sure each profiling result is computed by averaging several (usually 3) individual runs. Each run should do ~10 iterations (a for loop with 10 iterations basically). The time of a run is the average of the 10 iterations, excluding the cold one (the first). Also, make sure you compute of profiling test that takes ~10 seconds in total, so that any fluctuation is way more visible.

Please make this profiling test easy to use as we may want to share it and re-use it in the future. Therefore, consider implementing a nice small Java "Value profiler" that allows us to check how fast is H2 handling internal values. Note that for now the whole per-value profiling can't be done externally (as it needs changes to the H2 internals to trace <value-type>get). Therefore, profile only an overall time.

After this is completed, we prefer to integrate the profiling tool into H2 so we can run it from the CLI (./build.sh benchmark value-test).

I updated the table according to the new changes.
I changed get method in ValueBytes and ValueString, fromNanos in ValueTime and fromDateValue in ValueDate.

Ștefan, please prepare a commit on 7363b_h2 with your latest changes. Make sure you update #7363-11 accordingly. Also, post a status on your progress so far. I am ready to to review/test/profile a functional version of your changes by tomorrow EOD.

I made a profiler to test the new caching method.
I updated the table with the average time of 5 runs for each value type. The total time of the new caching method (21,255 s) is smaller than the old one (21,637 s).
TimeTimezone test result is strange, is ~20% slower, I will make further investigations for that particular value.
I will commit 7363b_h2 ASAP, i want to test the changes with a large application before committing.

Committed to 7363b_h2 revision 20.

Review of 7363b_h2 revision 20.

• Use space after if keyword to separate from ( (according to H2 standard)
• Use else on the same line as closing brace } (according to H2 standard)
• In fact, there is no need for else branch; please use one single fat if conditional; don't extract cachedType
• Please override the hash of the object if already computed. For example, in ValueBytes, as you already computed hash, you can assign it to the newly created object (obj.hash = hash). This avoids a second hash computation for the object when first used. Unfortunately, this slow-down couldn't have been catch by your profiling.
• For ValueFloat and ValueDouble, you slightly changed the equals approach. H2 uses Double.compare(a, b) 0 and Float.compare(a, b) 0. Please do the same, instead of using ==.
• In ValueString, you are using StringUtils.cache twice. No need for the second cache check.
• In ValueStringFixed and ValueStringIgnoreCase, it is fine to let lowLevelEquals be inherited; no need to redefine.

In general, obj instanceof Long && this.nanos == (long) obj doesn't feel like a safe comparison. Most probably it will work as (long) casting will unbox Long, but as long as you do an instanceof Long, I expect the cast to be done on Long, not long. An ideal solution is obj instanceof Long && this.nanos = ((Long) obj).longValue(). This is the same code JDK uses behind the scenes on equals), so I guess it is better tested. The same goes anywhere you do such primitive comparisons (i.e. Long, Integer, etc.).

I added the changes.

Commited to 7363b_h2 revision 21.

Review of 7363b_h2 revision 21

• ValueByte.lowLevelEquals should use ((Byte) obj).byteValue() to stay consistent with other implementations.
• ValueBytes.lowLevelEquals is not right: you are using this.value.equals(obj) instead of the current ValueBytes.equals implementation Arrays.equals(value, ((ValueBytes) other).value). Please change.
  • Please note that H2 uses different hash-code computation for byte arrays of over/under 50 elements.
  • Please extend you test-cases with byte arrays over/under 50 elements so that you trigger different Utils.getByteArrayHash computation.
• ValueDate / ValueLong / ValueTime / others: I think the cognitive complexity of Long.compare(this.dateValue, ((Long) obj).longValue()) 0 is too high. Please fall-back to this.dateValue ((Long) obj).longValue(). Doing compare makes sense for Double and Float because == is subject to precision issues, while compare does a better job by retrieving more complex representations of the numeric data. Please let only Double and Float use compare.
• ValueDecimal: the implementation here mismatches the original implementation:
  • use a space between if and (
  • the get(BigInteger) is not querying the cache with a BigDecimal, but with a BigInteger. This will always fail and miss the cache! Please create a new BigDecimal(bigInteger) and use it for hash, cache look-up and instantiation. Otherwise, lowLevelEquals will always try to compare a BigDecimal with a BigInteger and it will fail.
  • Please extend your test-cases to cover this case: assign BigInteger to BigDecimal field.
• ValueString doesn't use String.hashCode, but a custom faster implementation of string hashes which can be found in ValueString.hashCode. This should be used on cache look-up.
  • Move the hash computation method in a separate method (protected static int computeHashCode(String value). Use it in ValueString.hashCode and ValueString.get
  • Especially when you do obj.hash = hash you assign a differently computed hash comparing to the one computed by ValueString.hashCode
  • Please extend your test-cases with strings over/under 16 characters so that you trigger different ValueString.hashCode computation.
• The same goes for ValueStringFixed; please use ValueString.computeHashCode for that matter, not String.hashCode
  • You are missing obj.hash = hash for ValueStringFixed; the hash computation is quite heavy for string data
• The same goes for ValueStringIgnoreCase; please use ValueString.computeHashCode for that matter, not String.hashCode
  • You are missing obj.hash = hash for ValueStringFixed; the hash computation is quite heavy for string data
• ValueTimeTimezone performance decrease is really bad. Please take a look into it and try to understand why is this happening. Check that you don't do too much overhead in the caching and that you have the same cache hit as before.

Avoid caching ValueTimestampTimeZone¶

• if there are few (< 2 per ms), we can avoid caching of course
• if there are too many (>100 per ms), we can avoid caching as this will actually pollute our cache. With an ideal hashing technique, the cache will be full of ValueTimestampTimeZone in 10s.

• The number of times CurrentTimestamp.get is called (in total and in avg. per ms)
• The number of ValueTimestampTimeZone in the cache once every 100 ms (a ratio considering the total size of the cache)
• The total time and avg. per call spent in CurrentTimestamp.get (with vs without caching)

I added the changes.

Committed to 7363b_h2 revision 22.

Review of 7363b_h2 revision 22

• ValueBytes is not using instanceof to check obj of type byte[]. This is fatal and should be fixed right away. If some other value with the same hash collides here, H2 will throw ClassCastException.
• I am OK with the other changes

Please fix ValueBytes now. I am planning to do a profiling round asap.

Added the change and committed it to 7363b_h2 revision 23.

FWD-H2 is not at rev. 48 in trunk. This can be closed.