Thoughts on proposals for improvements on Gtid handling (parsing, adding to GtidSet) (performance, memory, cpu)

[janickr]

Hi @osheroff

This is a proposal for a change, that I would like to discuss before working on it and submitting a PR.

The current code is not that efficient in the way mysql GTIDs are handled:

• GtidEventDataDeserializer:

  • gets de serverId from the inputstream as a byte array
  • converts sequences of this array to hexadecimal string representations
    • using String.format on each byte
    • adding them to a stringbuilder
    • performing toString on the builders
    • concatenation the result in a full GTID string representation of serverId:sequence

• GtidSet: when the BinaryLogClient commits the Gtid to the gtidSet, it calls add(String)

  • add(String gtid) again parses the gtid string representation into a serverId and a sequence number to be added to the set

When profiling an application that uses the binlog client this shows as a significant part of the allocation profile.

1) I think as a minimum these changes are needed to avoid the string operations:

• introduce a MySqlGtid class similar to the MariaGtid class but containing a UUID serverId (sourceId) and a long transactionId (sequence)
• let GtidEventDataDeserializer get the serverId as 2 longs (mostSignificantBits and LeastSignificantBits) and construct the MySqlGtid
• keep an object gtid instead of a String gtid in the BinaryLogClient
• add a add(Object gtid) method to GtidSet and MariadbGtidSet that casts the gtid to the correct subclass and adds it to the set
• In GtidSet change the <String,UUIDSet>map to <UUID,UUIDSet>map, and change the type of serverId in UUIDSet to UUID

This can probably be done keeping all existing public methods in place, only adding methods and delegating existing ones, so afaik without making breaking changes.

2) Additionally we could also improve the performance of the current GtidSet
I did some experiments and by making the common case fast (usually the next gtid is the increase of the sequence number of the same server)
The troughput of adding a gtid to a gtidset can be increased 4-fold compared with when we only make the changes in (1), and more than 60-fold compared to the original add method with the string.split (if the setup and interpretation of my jmh benchmarks is correct)
This change is a bit harder to perform without making changes to the interface of GtidSet or UUIDSet (I think it's still possible, I'll probably have to actually try to keep the interface the same to see how it works out)

3) We could also make a few changes to the design, but this comes at the cost of making some breaking changes

• at the moment MariaDbGtidSet inherits from GtidSet, but this is mainly because GtidSet defines the interface for adding the gtid (upon commitGtid), it only uses part of the interface an none of the member variables
• there are some if statements in BinaryLogClient that choose different behaviour for mysql and mariadb regarding gtid handling
• there are some members that are inspected or changed together (gtid, gtidset, tx, gtidAccesslock)

=> we could explore introducing some kind of (Abstract)TransactionState/GtidState/ReplicationState with 2 subclasses, one for Mysql and one for mariadb, encapsulation the difference in state and behaviour.
MariadbGtidSet and (Mysql)GtidSet could be two distinct/independent classes, gtid can be a member of those classes with it's respective MariaDbGtid or MysqlGtid type, so there will be no need to use String or Object as the type

For my use case the most important change is 1) then 2) then 3). I wanted to know your thoughts on this before I spend time on one or more PRs for this. Also I'm curious to know how important it is to keep the GtidSet, UUIDSet and GtidEventData interfaces stable given the version number is 0.27.6, because it takes more care to do so.

What are your thoughts on this?

[osheroff]

Can you give some numbers showing how expensive each issue you're calling out is? Whether to proceed on large changes like this really are just a matter of how much speed you'll win by doing so.

As far as breaking the interface goes; generally, the answer is no. Ignore the lower version number, this library is stable and used heavily in production all over the place; I'm not quite sure of course who relies on the specific interfaces of UUIDSet etc, but I'd still prefer to not change interfaces unless there was a very very large win possible.

regarding two different classes for maria/mysql:

The mariaDB GTID support is more recent (and so could in theory break a little), but the interface as it stands can be used reasonably well without having to specify flags for connecting to maria vs mysql, which I like.

we could explore introducing some kind of (Abstract)TransactionState/GtidState/ReplicationState

I like this idea, especially if we can keep the old interfaces the same but add a migration path to the new. I'm also not against marking the old interfaces as deprecated, if you come up with a great approach to organizing this information... but it'd probably be a long long migration to the new interfaces.

[janickr]

I created a POC to compare the allocation and cpu profiles of the points 1) and 2). Point 3) is more of a design change imo.

TL;DR
IMO it is worth it fixing the allocations in GtidEventDataDeserializer, the GtidSet speedup not so much

The setup

The POC app starts a MySql OneTimeServer with binary logs in gtid mode. It creates a table and populates it with 300000 rows. The total size of the binlog is around 130Mib.
It then starts the various binary log client implementations one after another, a first time without profiling, to ensure all classes are loaded at least once, then with cpu profiling and a last time with allocation profiling.
The binlog clients use the EventDeserializer with the default EventDataDeserializers, so in addition to the GTID data, also the WriteRowsEventData is deserialized during the run.

1) The allocations caused by GtidEventDataDeserializer:

First the allocation profile of the current binarylogclient:

More than half of the allocations are caused by the String operations in GtidEventDataDeserializer

If instead we read the gtid in a MySqlGtid object with UUID sourceId and long transactionId:

The GtidEventDataDeserializer accounts for less than 1% of the allocations

With the allocations of the GtidEventDataDeserializer out of the picture, I also noticed that the EventHeaderV4Deserializer kept creating arrays for each event:

This was causing more than half of the allocations

When building and using an index once, these allocations can be avoided:

The string formatting is also very visible in the cpu profile (the byteArrayToHex method):

This is improved when we deserialize to a gtid object:

2) the cpu performance of the GtidSet
... or the perfect example of Amdahl's law

The cpu usage of the original GtidSet.add method:

is 3,3% of the total, mostly dominated by the string parsing

When we use the gtid object instead (no string parsing):

it is reduced to 1,2% of the total cpu usage

With a new GtidSet implementation:

only 0.1% remains

Some concluding thoughts
I think most gains can be made in changing the gtid deserializer (and the EventType.byEventNumber method). This does imply changing the protected gtid field in the BinaryLogClient to a common supertype of MariadbGtid and a new MySqlGtid. The most sensible common supertype is Object, inventing an other one is a bit artificial.
This smells and therefore I proposed (3) introducing some kind of (Abstract)TransactionState/GtidState/ReplicationState. That change is harder (keeping the API stable, maybe that also includes the protected gitd related fields?) and maybe not feasable within the boundaries of the current API.

While the new GtidSet is significantly faster than the current GtidSet, it probably does not matter because the current GtidSet.add method is only a fraction of the full cpu profile. So although I like my implementation, I don't think it would add much value.

Sources
The html flamegraphs (the source for the screenshots) are here:
flamegraphs-2.zip

You can find the source code to review and reproduce the profiling at https://github.com/janickr/mysql-binlog-connector-java/tree/gtd-profiling-poc

Run the poc with
./mvnw test-compile exec:exec -Dexec.executable=java -Dexec.args="-cp %classpath -XX:+UnlockDiagnosticVMOptions -XX:+DebugNonSafepoints com.github.shyiko.mysql.binlog.gtidprofilingpoc.ProfileMain" -Dexec.classpathScope=test

It will generate the flamegraphs in the project root directory.

Edit: new screenshots of a run with better JVM warmup