FWD » Core Development » Database

I encountered several fake-updates in large customer applications. A fake-update is something like UPDATE tt SET f1 = 2 WHERE f1 = 2. It is not necessarily that the WHERE clause should match the SET clause; only that the old row value should be equal to the new row value.

In the latest FWD-H2, we reconstruct indexes only if indexed fields are updated. However, if we fake-update an indexed field, we may end up reconstructing the index. H2 specifically removes the old row and adds a new row to emulate modification. This doesn't make sense if the old value is the same as the new value. The only side-effect I see is the corruption of delta tables (SELECT * FROM OLD TABLE (UPDATE TEST SET B = 3 WHERE ID = 1);) and triggers, but this won't affect FWD.

As part of the work in #7061, the lazy keyword has some issues with there fake-updates. In fact, lazy will skip all equal records from an index if such fake-update happens (in fact, this happens in way more specific case).

These should not occur since we create the SET part of statement based only on changed properties (BaseRecord.dirtyProps).

However, changing back a value of a field before it had the chance to be flushed could possibly generate the fake-updates (by marking the property as dirty but saving the exact same value). This could be theoretical double checked in Persister.update() but would add a performance penalty (ignoring the dmo.getDirtyProps() and recomparing the properties again). It also looks a bit stupid, unless we rename it to getPossiblyDirtyProps. This should be done only if the performance gain in H2 big enough to overcome this extra work.

I recall a scenario in which some records were updated in a for-loop and their new values were the same as the old values. I didn't dig it up too much; I will try to search for that example.

define temp-table tt field f1 as character.

create tt.
tt.f1 = "abc".

for each tt:
    tt.f1 = "abc". // fake-update
    message tt.f1.
end.

• SQL parsing, cache pollution, index look-up, etc.: update tt3 set f1=? where recid=? and _multiplex=?
• Index reconstruct and logging: remove row, add row, register update to transaction undo log
• 4GL FOR-EACH behavior: the query is not "invalidated", because in 4GL, the update is not done. In FWD, the query is invalidated because of the index reconstruction. We have a weak solution for this, reconstructing the index cursor based on the old value. However, if this is a fake-update, the index cursor reconstruction may skip some records equal to the updated one (because it is unware of which record was stored before invalidation).

If we are to consider a solution at the FWD level, we can cover fake-update in persistent databases as well. If we want to target H2, we can recover some time from index and cursor reconstruction by checking if the updated indexed field is the same.

• if string hashes don't match, let the engine do the fake-update (this is the usual case and is not that time consuming - or is it?)
• if the string hashes match, do a full string equality check
  • if the strings match, avoid doing the update. This recovers a lot of time. Reconstructing an index means several string equality checks, doing only one to prevent it is ideal.
  • if the strings don't match, let the engine do the fake-update (even if this is an overhead, this is very rare)
• maybe we can do some prerequisite checks to avoid hash computation: do reference equality check, check sizes, check first 2 and last 2 characters (this can rule out some cases very fast)

This technique is classic and may fit our intention very well. My only concern is that the hash computation of strings is time consuming, but it is usually cached (internally in String).

H2 database engine is built in Java. My first reaction to your hash suggestion was that it unneeded since Java has the hashing thing already built-in. More than that, since the String is an immutable class, the hash can be computed at first usage, stored and used when a equality test is needed, to quickly determine when two String objects are different.

Then I open the String class (for OpenJDK 1.8) and I found:

    public boolean equals(Object anObject) {
        if (this == anObject) {
            return true;
        }
        if (anObject instanceof String) {
            String anotherString = (String)anObject;
            int n = value.length;
            if (n == anotherString.value.length) {
                char v1[] = value;
                char v2[] = anotherString.value;
                int i = 0;
                while (n-- != 0) {
                    if (v1[i] != v2[i])
                        return false;
                    i++;
                }
                return true;
            }
        }
        return false;
    }

So the hash check is probably going to add a bit of performance improvement, unless the Strings were already intern -ed, in which case the equals will return with positive result from the first line.

OTOH, I think we are interested mostly on the case insensitive case, which is the feature of 4GL we are trying to emulate for basic character data. Unfortunately, the hash will not work here as expected. At least with standard hash of the String. To avoid this, we can use the hash of the lowercased instance of the string instead.

Ovidiu Maxiniuc wrote:

Apparently, the hash short-circuit is not used. The length of the strings are compared, but not the hashes. I really did not expect this.

I had the same expectation. String.compareTo is even more shocking, as there is no equality rule-out.

OTOH, I think we are interested mostly on the case insensitive case, which is the feature of 4GL we are trying to emulate for basic character data. Unfortunately, the hash will not work here as expected. At least with standard hash of the String. To avoid this, we can use the hash of the lowercased instance of the string instead.

The case-sensitivity is important. Doing something like update tt set f1 = 'abC' where f1 = 'aBc' is not a "fake-update"; I expect to retrieve abC afterwards. I don't think we should suppress such update, even if f1 is case-insensitive and matches the old value. I agree however on the fact that its position inside the index doesn't change, so there is no need to reconstruct. But this is a fine tune IMO, as I expect most of the "fake-updates" to have the same case as before.

Right now, I am still dragged back by String.hashCode:

   public int hashCode() {
        int h = hash;
        if (h == 0 && value.length > 0) {
            char val[] = value;

            for (int i = 0; i < value.length; i++) {
                h = 31 * h + val[i];
            }
            hash = h;
        }
        return h;
    }

Alexandru Lungu wrote:

Right now, I am still dragged back by String.hashCode:
[...]

Why is that? It seems like the usual lazy computation of hash private field. Maybe the function is not dispersing the string values the best across the full 32 bit key space but this is a quite standard, fast approach.

Worst case scenario, a string is not interned, so we have to compute its hash code. If we do such iteration anyways, we could have done the equality check instead at the same cost. My solution heavily relied on the fact that String hash code computation is faster and better cached. If we know that we already have computed the hash-code in a previous stage, we are good to go. But I don't think that this is the case. For an update, I don't think we ever compute the hash code in a previous step.

At the moment the String value reaches the database level it is probably have been involved in some computations and the chances are that the has already been calculated. Usually, the String values are rarely equals (think of unique string indices) so the 'fake' updates are not common. Detecting the difference with a simple integer compare operation would be faster in those cases. But if the ABL code does this repeatedly or the values are long enough, the improvement might be visible.

Ovidiu Maxiniuc wrote:

Alexandru Lungu wrote:

Right now, I am still dragged back by String.hashCode:
[...]

Why is that? It seems like the usual lazy computation of hash private field. Maybe the function is not dispersing the string values the best across the full 32 bit key space but this is a quite standard, fast approach.

Mostly because it does a full string traversal. I agree that there is no faster way to do it, but we are to compute a hash-code for each update we are going to make in the database. Also, a simple integer compare is fast, but to achieve that integer, you have to do a full string traversal (maybe tens or even hundreds of characters?). I also expect most of the strings to be encountered for the first time as they are not interned, so they don't have a precomputed hash-code. This whole effort just to check if we hit that slim number of cases of an fake-update.

At the moment the String value reaches the database level it is probably have been involved in some computations and the chances are that the has already been calculated. Usually, the String values are rarely equals (think of unique string indices) so the 'fake' updates are not common.

Right. I will do some tests to check how many such fake-updates are actually in some large customer applications. I will also check how much the String.hashCode call takes on average in a real scenario.

Committed 7138a_h2/rev.16. Made changes to should-circuit fake-updates. TestCompatibility change is related to #7108-41. Made build.sh executable.

A fake-update is detected using the Value.compareTypeSafe method, which compares values of the same type. I also keep track of the number of updated non-indexed/indexed columns, this helped to remove the inMemUpdate usage and replace it with another condition related to the number of columns updated. We want to make an update in memory when we handle non-indexed columns

• the old value is null and it is updated with null
• the old value is Value.Enum
• the value types of the old and new value are different

For the last mentioned case, Table.validateConvertUpdateSequence is called after checking the columns that need to be updated and eventually leads to the value conversion:

    CREATE TABLE test (f1 VARCHAR(8)); // field values will be ValueString
    INSERT INTO test (f1) VALUES('21');
    UPDATE test SET f1 = 21; // new value is ValueInt

• TestLinkedTable: A table link can be created for a table and any operation done on the table link is reflected in the original table. The test runs 3 iterations using different table links in different modes and each iteration executes a delete, insert and update using the same values. While updating the value for a read only table link, the fake-update is discovered and the method returns early without throwing an error for a read only table. The test was changed to use different values at each iteration.
• TestStatement: Related to the MERGE statement. It would try to merge the same value twice and would fail since a fake-update is discovered.

Please address this review on 7138a_h2/rev.16 asap. Some of the points here are crucial for the performance tests to make sense.

• You have oldValue.getValueType() != Value.NULL twice in the fake-update conditional.
• You can extract getValueType answer in separate variable to avoid multiple calls to the same function.
• You use compareTypeSafe to check if the values are not in fact equal. Maybe it is faster to simply use equals instead, we already know that they have the same type?
• table.hasIndexedColumn is way faster than table.getIndexForColumn. Refactor to use hasIndexedColumn.
• I think it better to move the inMemRows and notInMemRows resets in the finally block. If an error occurs, these won't be reset properly.

We will take in consideration the hashing tactic discussed in #7138-6 afterwards, if still needed.

Alexandru Lungu wrote:

Please address this review on 7138a_h2/rev.16 asap. Some of the points here are crucial for the performance tests to make sense.

• You have oldValue.getValueType() != Value.NULL twice in the fake-update conditional.

• You can extract getValueType answer in separate variable to avoid multiple calls to the same function.
• You use compareTypeSafe to check if the values are not in fact equal. Maybe it is faster to simply use equals instead, we already know that they have the same type?

• table.hasIndexedColumn is way faster than table.getIndexForColumn. Refactor to use hasIndexedColumn.
• I think it better to move the inMemRows and notInMemRows resets in the finally block. If an error occurs, these won't be reset properly.

Committed 7138a_h2/rev.17. I applied the changes mentioned in #7138-12. Please go ahead with the performance tests.

Review to 7138a_h2/rev.17 : I am OK with the changes.

Danut, please re-test 7138a_h2 (own test-cases, large customer application, FWD-H2 regression testing) and let me know if everything is OK.
Please fix/remove in 7138a_h2 the regression tests which fail due to your latest changes; AFAIK they address very specific cases not beneficial to FWD.

Once finished, let me know. I will start the testing/profiling routine.

I tested Hotel_GUI and found no problems, but I found a regression in a customer application which I didn't manage to point out yet. I am still looking into it.

I obtained good and bad results. I will continue to expand the tests and share my findings. Please let me know of any specific tests that should be done.

I've tested fake-update performance for larger tables and extent fields, results of each test are measured in milliseconds and consist of the average obtained after running it 5 times.

In all cases, fake-update of indexed fields or of all fields shows an improvement, while updating all fields or fake-update of non-indexed fields show little to no change (<= 5%). Should I do similar tests for persistent tables?
UPDATE: Corrected results that were accidentally swapped. Tests need to be redone due to update statements being executed for each field.

Dănuț Filimon wrote:

Should I do similar tests for persistent tables?

As a lower priority, yes, just to make sure there isn't a regression when used in that mode.

We don't use H2 for persistent tables in production, so performance of persistent tables in H2 is not as important for us as embedded, in-memory use. However, we do use it for incremental conversion support, and occasionally for development/testing purposes. So, we will want to know if there is a regression. However, this applies to all H2 changes, not just this specific one.

• you can do a quick reconversion of Hotel GUI and use H2 persistent database just to check for regressions. AFAIK FWD-H2 regression testing is OK.
• please compute a ratio of how many fake-updates are happening in a large customer application: this is for reference only.

I am doing a final profiling round over a POC.

Committed 7138a_h2/rev.21. Fixed a concurrency problem. inMemUpdate and notInMemUpdate need to be set to null when database is in persistent mode.

Alexandru Lungu wrote:

• you can do a quick reconversion of Hotel GUI and use H2 persistent database just to check for regressions. AFAIK FWD-H2 regression testing is OK.

Hotel GUI has no regressions with H2 persistent database.

• please compute a ratio of how many fake-updates are happening in a large customer application: this is for reference only.

I updated and tested a customer application (~35 minutes) and from a total of 1352516 update statements I calculated the following results:

Even thought updates made on both types of columns represent ~0.24%, more than half of the field updates are fake-updates, while updates made only on non-indexed columns have ~10% fake-update cases.

I ran separate tests and used VisualVM to track the total time of certain operations. The tests consist of a temporary table with 10 indexed fields and 10 non-indexed fields (all 20 are integers), it starts with an initial create of 1000 items (for 10000 it takes around ~9 minutes to create a snapshot).

The test originally used integer fields. If necessary, I will also do the same tests using string fields and 2500-5000 initial creates.

Tested with a large customer application and I get similar results: between -0.1% and +0.1%. I will retest on a newer binaries set.

• update a non-null value with null
• update a null value with non-null
• fake-update with null

Note that by null, I mean NullValue.

After looking at the code, I remember that I excluded Value.NULL because I was using compareTypeSafe which throws an exception when comparing null values, but this does not happen now because the comparison is done using equals. It is safe to remove the type check for Value.NULL and let it be handled as a fake-update when necessary.

Dănuț Filimon wrote:

After looking at the code, I remember that I excluded Value.NULL because I was using compareTypeSafe which throws an exception when comparing null values, but this does not happen now because the comparison is done using equals. It is safe to remove the type check for Value.NULL and let it be handled as a fake-update when necessary.

Please go ahead with this change. Let the null checks: a != null & b != null, but remove the NullValue checks.

Committed 7138a_h2/rev.22. Allowed ValueNull values to be compared.

Alexandru Lungu wrote:

Danut, I rebased 7138a_h2 from FWD-H2 trunk (rev. 23). Please commit a patch to avoid fake-updates only on indexed fields. I will retest with your changes and #7454 (which will make the comparison faster eliminating the value casts).

Committed 7138a_h2/rev.29. Only indexed columns will be checked for fake-updates.

Yes, please go ahead.

After running a large POC with 7156b/rev.14846 (which uses FWD-H2/rev.33) and the changes from 7138a_h2/rev.24 - 29. From 5 runs each (baseline and 7156b with 7138a_h2) I did not obtain an improvement for the average of the last 20 runs (the result was +0.065%) and ~1% improvement for the total average of the 100 runs.

What is the status of 7138a_h2? Does it need more review? More testing? It seems to provide a decent improvement on the total runs.

I updated the sources for the customer applications but there are some regressions/files missing which I'll need to look into. The POC needs to be reconverted with the latest version of 7156b. I am currently running ChUI regression tests since those are currently the only possible ones that can be done immediately.

Please let the application convert over night. After, we can go ahead with proper testing.

Alexandru Lungu wrote:

Please let the application convert over night. After, we can go ahead with proper testing.

That's my intention, confirming that the POC results are close to the ones from #7138-31 is a priority.

• Rebased 7138a_h2 with fwd-h2/rev.38. 7138a_h2 is now at revision 45;
• ChUI regression tests passed;
• Reconverted POC project with 7156b/rev.14932 and will start redoing the baseline and testing 7138a_h2;
• A few issues in a large customer application were solved in the latest version of trunk, so I'll go ahead and do a quick smoke test before continuing with POC.

There are currently problems with the POC project and the performance tests will be put on hold until further notice. I also successfully smoke tested two customer applications and found no problems so the POC is the only one remaining on the list.

I tested POC with 7156b/rev.14933 (which uses FWD-H2/rev.38) and 7138a_h2/rev.45. From 5 runs each (baseline and 7156b with 7138a_h2) I obtained a decrease in performance in both cases, 1.087% for the average of the last 20 runs and 0.786% for the total average.

I don't think the results are spoiled, the only difference is that I had to redo the baseline since the last test from #7138-31 and that I am testing on another device. I want to make sure that we can move on with the changes, so I'll also run the tests on the original device.

Danut, this is a common pattern: in different days and on different platforms to see different results. The only way to ensure you have a proper conclusion is to continue testing until the times actually converge to a fixed point. Ensure you are always on the same device (first and foremost), ensure there are no background tasks and *re-import" database when changing branches (it works faster on fresh database).

I retested POC on the original device and obtained a performance improvement for both the average of the last 20 runs (4.77%) and the total average (2.28%). Regression tests were done (#7138-37) and there were no issues, can we go ahead and merge 7138a_h2?

Alexandru: If you are OK with this, it can merge now.

Danut, please go ahead and merge to FWD-H2 trunk.
Greg, this will reach FWD on the next H2 upgrade.