@medic123de
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
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
Travis fails because the mysqli extension didn't support begin_transaction() until php 5.5.
Interesting, 6144d081f15f8febc62391c5f8a682f044f75155 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.
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 https://github.com/piwik/piwik/commit/6144d081f15f8febc62391c5f8a682f044f75155 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
https://github.com/medic123de/piwik/commit/5af8e0b6859ea184f2c1f9f0add90c4320935231 should not have failed.
@mattab
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.
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?
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.
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!
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?
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...
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.
Matt, any reason why you did not merge?
@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
Oh, sorry. I've been worried code quality isn't sufficient, or there were some issues left I missed.
I created a ticket for this change here: http://dev.piwik.org/trac/ticket/5275#ticket
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!
@medic123de Well done, this was an epic change :8ball: