| Message ID | 20220804163756.652474-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | e5f79eaea5fc668bffc4f2bbc060428fbce3cb92 |
| Headers | show |
| Series | [ovs-dev] ovsdb: Don't store rows that didn't change in transaction history. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 8/4/22 18:37, Ilya Maximets wrote: > Transaction history is used to construct database updates for clients. > But if the row didn't change it will never be used for monitor updates, > because ovsdb_monitor_changes_classify() will always return > OVSDB_CHANGES_NO_EFFECT. So, ovsdb_monitor_history_change_cb() > will never add it to the update. > > This condition is very common for rows with references. While > processing strong references in ovsdb_txn_adjust_atom_refs() the > whole destination row will be cloned into transaction just to update > the reference counter. If this row will not be changed later in > the transaction, it will just stay in that state and will be added > to the transaction history. Since the data didn't change, both 'old' > and 'new' datums will be the same and equal to one in the database. > So, we're keeping 2 copies of the same row in memory and we are > never using them. In this case, we should just not add them to the > transaction history in the first place. > > This change should save some space in the transaction history in case > of transactions with rows with big number of strong references. > This should also speed up the processing since we will not clone > these rows for transaction history and will not count their atoms. > > Testing shows about 5-10% performance improvement in ovn-heater > test scenarios. > > 'n_atoms' counter for transaction adjusted to count only changed > rows, so we will have accurate value for a number of atoms in the > history. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- Looks good to me, thanks! Acked-by: Dumitru Ceara <dceara@redhat.com>
On 8/26/22 15:46, Dumitru Ceara wrote: > On 8/4/22 18:37, Ilya Maximets wrote: >> Transaction history is used to construct database updates for clients. >> But if the row didn't change it will never be used for monitor updates, >> because ovsdb_monitor_changes_classify() will always return >> OVSDB_CHANGES_NO_EFFECT. So, ovsdb_monitor_history_change_cb() >> will never add it to the update. >> >> This condition is very common for rows with references. While >> processing strong references in ovsdb_txn_adjust_atom_refs() the >> whole destination row will be cloned into transaction just to update >> the reference counter. If this row will not be changed later in >> the transaction, it will just stay in that state and will be added >> to the transaction history. Since the data didn't change, both 'old' >> and 'new' datums will be the same and equal to one in the database. >> So, we're keeping 2 copies of the same row in memory and we are >> never using them. In this case, we should just not add them to the >> transaction history in the first place. >> >> This change should save some space in the transaction history in case >> of transactions with rows with big number of strong references. >> This should also speed up the processing since we will not clone >> these rows for transaction history and will not count their atoms. >> >> Testing shows about 5-10% performance improvement in ovn-heater >> test scenarios. >> >> 'n_atoms' counter for transaction adjusted to count only changed >> rows, so we will have accurate value for a number of atoms in the >> history. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- > > Looks good to me, thanks! > > Acked-by: Dumitru Ceara <dceara@redhat.com> Applied. Thanks! Best regards, Ilya Maximets.
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c index 6864fe099..450ac28cf 100644 --- a/ovsdb/transaction.c +++ b/ovsdb/transaction.c @@ -42,7 +42,7 @@ struct ovsdb_txn { struct ovs_list txn_tables; /* Contains "struct ovsdb_txn_table"s. */ struct ds comment; struct uuid txnid; /* For clustered and relay modes. It is the eid. */ - size_t n_atoms; /* Number of atoms in all transaction rows. */ + size_t n_atoms; /* Number of atoms in all changed transaction rows. */ ssize_t n_atoms_diff; /* Difference between number of added and * removed atoms. */ }; @@ -987,9 +987,17 @@ static struct ovsdb_error * OVS_WARN_UNUSED_RESULT count_atoms(struct ovsdb_txn *txn, struct ovsdb_txn_row *txn_row) { struct ovsdb_table *table = txn_row->table; + size_t n_columns = shash_count(&table->schema->columns); ssize_t n_atoms_old = 0, n_atoms_new = 0; struct shash_node *node; + if (txn_row->old && txn_row->new + && bitmap_is_all_zeros(txn_row->changed, n_columns)) { + /* This row didn't change, it has nothing to contribute + * to atom counters. */ + return NULL; + } + SHASH_FOR_EACH (node, &table->schema->columns) { const struct ovsdb_column *column = node->data; const struct ovsdb_type *type = &column->type; @@ -1115,6 +1123,14 @@ ovsdb_txn_clone(const struct ovsdb_txn *txn) struct ovsdb_txn_row *r; HMAP_FOR_EACH (r, hmap_node, &t->txn_rows) { size_t n_columns = shash_count(&t->table->schema->columns); + + if (r->old && r->new + && bitmap_is_all_zeros(r->changed, n_columns)) { + /* The row is neither old or new and no columns changed. + * No need to store in the history. */ + continue; + } + struct ovsdb_txn_row *r_cloned = xzalloc(offsetof(struct ovsdb_txn_row, changed) + bitmap_n_bytes(n_columns));
Transaction history is used to construct database updates for clients. But if the row didn't change it will never be used for monitor updates, because ovsdb_monitor_changes_classify() will always return OVSDB_CHANGES_NO_EFFECT. So, ovsdb_monitor_history_change_cb() will never add it to the update. This condition is very common for rows with references. While processing strong references in ovsdb_txn_adjust_atom_refs() the whole destination row will be cloned into transaction just to update the reference counter. If this row will not be changed later in the transaction, it will just stay in that state and will be added to the transaction history. Since the data didn't change, both 'old' and 'new' datums will be the same and equal to one in the database. So, we're keeping 2 copies of the same row in memory and we are never using them. In this case, we should just not add them to the transaction history in the first place. This change should save some space in the transaction history in case of transactions with rows with big number of strong references. This should also speed up the processing since we will not clone these rows for transaction history and will not count their atoms. Testing shows about 5-10% performance improvement in ovn-heater test scenarios. 'n_atoms' counter for transaction adjusted to count only changed rows, so we will have accurate value for a number of atoms in the history. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- The function 'ovsdb_txn_clone' is going to be renamed as part of the other patch, so it will make more sense: https://patchwork.ozlabs.org/project/openvswitch/patch/20220802132333.2258491-1-i.maximets@ovn.org/ ovsdb/transaction.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)