Derby
  1. Derby
  2. DERBY-4512

Avoid unnecessary lookup in transaction table when adding transaction

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 10.6.1.0
    • Fix Version/s: 10.6.1.0
    • Component/s: Store
    • Labels:
      None
    • Bug behavior facts:
      Performance

      Description

      TransactionTable.add() first checks if the Hashtable trans contains a transaction with the same id as the one being added. If it does, add() does nothing. If there is no such transaction, a new TransactionTableEntry is created and put into the Hashtable.

      I believe that TransactionTable.add() is never called on a transaction that has already been added to the table. If this is the case, there's no point in checking the Hashtable first. Instead, we could just create a new TransactionTableEntry and add it unconditionally. This would reduce the number of (synchronized) Hashtable calls and could improve the performance in scenarios like the one described in DERBY-3092.

      1. txtab-noblanks.diff
        2 kB
        Knut Anders Hatlen
      2. txtab.diff
        2 kB
        Knut Anders Hatlen
      3. assert.diff
        0.6 kB
        Knut Anders Hatlen

        Issue Links

          Activity

          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 900714.

          Show
          Knut Anders Hatlen added a comment - Committed revision 900714.
          Hide
          Knut Anders Hatlen added a comment -

          Here's a patch that makes the suggested change (txtab.diff). The patch looks somewhat messy because much code is moved up one indentation level because an if statement is removed. I've therefore also attached a patch generated with svn diff --extensions="-ub" which ignores whitespace changes, to make it easier to see exactly what has changed (this patch is called txtab-noblanks.diff).

          The patch changes TransactionTable.add() in the following ways:

          1) It does not call findTransactionEntry(id) first and checks the return value.

          2) The return value from trans.put(id, ent) is captured, and an assert is added to verify that it is null (that is, the entry was not already there).

          3) An assert that checked that the existing entry had the same exclusion as the one we attempt to add, is removed. This is because we now assert that there is no existing entry, which is a stricter condition.

          TransactionTable.add() uses a mix of tabs and spaces for indentation. I chose to use spaces consistently for my changes.

          All the regression tests ran cleanly with the patch.

          Show
          Knut Anders Hatlen added a comment - Here's a patch that makes the suggested change (txtab.diff). The patch looks somewhat messy because much code is moved up one indentation level because an if statement is removed. I've therefore also attached a patch generated with svn diff --extensions="-ub" which ignores whitespace changes, to make it easier to see exactly what has changed (this patch is called txtab-noblanks.diff). The patch changes TransactionTable.add() in the following ways: 1) It does not call findTransactionEntry(id) first and checks the return value. 2) The return value from trans.put(id, ent) is captured, and an assert is added to verify that it is null (that is, the entry was not already there). 3) An assert that checked that the existing entry had the same exclusion as the one we attempt to add, is removed. This is because we now assert that there is no existing entry, which is a stricter condition. TransactionTable.add() uses a mix of tabs and spaces for indentation. I chose to use spaces consistently for my changes. All the regression tests ran cleanly with the patch.
          Hide
          Knut Anders Hatlen added a comment -

          Thanks Bryan. I was thinking that we could check the return value from Hashtable.put(), which should be null if there was no previous mapping registered for that transaction id.

          I tried such a change with the d2911perf client used in DERBY-3092 on a 32-way system. With 32 threads, trunk performed 158K tx/s and the patched version 182K tx/s, which means about 15% improvement.

          Still, this patch is far less effective than Dyre's patch that replaces the Hashtable with a ConcurrentHashMap. With that patch, I saw 230K tx/s. It is however possible to combine those two patches and get an additional 3-4% increase in the throughput on top of that, it seems, so I think it's going to be beneficial regardless of how/if we end up addressing DERBY-3092.

          Another thing I've noticed with TransactionTable.add() is that the synchronization on TransactionTable.this is probably not needed (will file another JIRA issue for that). My understanding is that synchronization on TransactionTable.this is used to protect transaction state changes from read to update. No such transition happens within that synchronized block, so the (implicit) Hashtable synchronization should be enough.

          Show
          Knut Anders Hatlen added a comment - Thanks Bryan. I was thinking that we could check the return value from Hashtable.put(), which should be null if there was no previous mapping registered for that transaction id. I tried such a change with the d2911perf client used in DERBY-3092 on a 32-way system. With 32 threads, trunk performed 158K tx/s and the patched version 182K tx/s, which means about 15% improvement. Still, this patch is far less effective than Dyre's patch that replaces the Hashtable with a ConcurrentHashMap. With that patch, I saw 230K tx/s. It is however possible to combine those two patches and get an additional 3-4% increase in the throughput on top of that, it seems, so I think it's going to be beneficial regardless of how/if we end up addressing DERBY-3092 . Another thing I've noticed with TransactionTable.add() is that the synchronization on TransactionTable.this is probably not needed (will file another JIRA issue for that). My understanding is that synchronization on TransactionTable.this is used to protect transaction state changes from read to update. No such transition happens within that synchronized block, so the (implicit) Hashtable synchronization should be enough.
          Hide
          Bryan Pendleton added a comment -

          +1 to removing the unnecessary check and leaving the SanityManager assertion as a verification.

          Show
          Bryan Pendleton added a comment - +1 to removing the unnecessary check and leaving the SanityManager assertion as a verification.
          Hide
          Knut Anders Hatlen added a comment -

          None of the tests in suites.All or derbyall call TransactionTable.add() on a transaction that's already in the transaction table. I verified this by applying the attached patch, assert.diff, which raises an assert failure if a transaction with the same id is found, and running all the regression tests. All of the tests passed.

          Show
          Knut Anders Hatlen added a comment - None of the tests in suites.All or derbyall call TransactionTable.add() on a transaction that's already in the transaction table. I verified this by applying the attached patch, assert.diff, which raises an assert failure if a transaction with the same id is found, and running all the regression tests. All of the tests passed.

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Knut Anders Hatlen
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development