Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transactional Database Mode #281

Merged
merged 18 commits into from Jun 2, 2014

Conversation

medic123de
Copy link
Contributor

Standard MySQL ( InnoDB ):

without-optimization:
Total time: 350 seconds
Requests imported per second: 13.41 requests per second

without-optimization + transaction:
Total time: 78 seconds
Requests imported per second: 543.68 requests per second

@medic123de
Copy link
Contributor Author

good idea!
i added DbException throwing, so an rollback will occur if comitting fails
( which in fact should not happen )
But in this case every information about the request is lost, so this is a bit more 'consistent'

@robocoder
Copy link
Contributor

Travis fails because the mysqli extension didn't support begin_transaction() until php 5.5.

@robocoder
Copy link
Contributor

Interesting, 6144d08 passed on Travis. Were the later commits necessary?

Did you benchmark running the tests? Travis gives the impression that tests are taking longer to run with this PR.

@medic123de
Copy link
Contributor Author

Sadly yes - i haven't found a way yet to setup a local test-suite, so several errors slipped through.
I found them when doing local tests to ensure a real performance gain.

Intrestingly 6144d08 passed, but i don't know why .. it should have failed, as there was still the PHP 5.5+ Implementation of MySQLi begin_transaction()

Benchmark ?
I have rolled out the changes several times across 6 different servers, and they all show a great performance gain.
I plan to add some unit-tests ( as soon as i found out how to - i never did code in PHP before )...

I considered it an coincidence that tests ran longer - but i wasn't able to find how to have them retestet by Mr. Travis, as
medic123de@5af8e0b should not have failed.

@mattab
Copy link
Member

mattab commented May 21, 2014

I restarted the test and it's passed now!

Very good idea to use transactions!!

This results in speed improvements, because the Innodb INDEXes are written only once when the transaction is committed, rather than being written after each UPDATE/INSERT queries.

@medic123de
Copy link
Contributor Author

I am happy you like it.
As you probably know, transactions are a problem when seperate parts ( objects) of a program try to use them nested ..

This is not a factor yet - but in case it ever will, i'd like to suggest to return a "transaction-ID" on beginTransaction, which has to be submittet in the commit-call ( and therefore to be saved ) ..
calling commit / rollback with a wrong transaction-ID could be a No-OP, making Transactions "safer to use", as the outmost Transaction will be covering all the others, and not be finished too early by a stray "commit/rollback".

what's your oppinion on that?

@@ -25,6 +25,8 @@ class Mysqli extends Db
protected $username;
protected $password;
protected $charset;
private $_activeTransaction = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do not prefix variables with _ so please name member: $activeTransaction

@mattab
Copy link
Member

mattab commented May 22, 2014

transactions are a problem when seperate parts ( objects) of a program try to use them nested

Good point, I don't see that we will use transactions within Tracking API appart from this wrapping around Bulk requests.

@mattab
Copy link
Member

mattab commented May 22, 2014

Before merging here is one change that is important:

  • only start transaction, when there is at least 2 requests, ie. only start transaction when we are doing a bulk tracking request. If we start transaction on all piwik.php requests, i'm afraid this could regress performance.

PS: I can make the change if you're not comfortable with it, just let me know!

@medic123de
Copy link
Contributor Author

Will do as suggested by you.
Will also test "single call"-mode. I'd guess it benefits, as single calls do 2 inserts and 1 update.
OK to you?

@mattab
Copy link
Member

mattab commented May 22, 2014

No we should not use transactions for "single call", let's only use transaction in Bulk mode (see previous comment). I could make this change if you wish, no problem...

@medic123de
Copy link
Contributor Author

please review again.
I included the nesting-protection - as I consider this a real danger to hit our ( better said your ) necks one day. introduced overhead should be negligible.

@medic123de
Copy link
Contributor Author

Matt, any reason why you did not merge?

@mattab
Copy link
Member

mattab commented May 27, 2014

@medic123de the release was coming and didn't want to make such big change prior to our major release. I'll investigate soon and will change it to use transactions only when count($requests) >= 2

@medic123de
Copy link
Contributor Author

Oh, sorry. I've been worried code quality isn't sufficient, or there were some issues left I missed.

@mattab
Copy link
Member

mattab commented Jun 2, 2014

I created a ticket for this change here: http://dev.piwik.org/trac/ticket/5275#ticket

Matthieu Aubry added 2 commits June 2, 2014 16:40
; Whether Bulk tracking requests will be wrapped within a DB Transaction.
; This greatly increases performance of Log Analytics and in general any Bulk Tracking API requests.
bulk_requests_use_transaction = 1
@mattab
Copy link
Member

mattab commented Jun 2, 2014

I made the couple small changes in a pull request on your fork https://github.com/medic123de/piwik/pull/2/files - once you merge it I think we can merge this PR!

Only use transactions for bulk requests (more than one request)
mattab pushed a commit that referenced this pull request Jun 2, 2014
Use transactions for Tracking API requests in bulk. fixes #5275

Thanks for the pull request, it's an Awesome Performance Improvement!
@mattab mattab merged commit 657a35e into matomo-org:master Jun 2, 2014
@mattab
Copy link
Member

mattab commented Jun 2, 2014

@medic123de Well done, this was an epic change 🎱

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants