@medic123de opened this Pull Request on May 20th 2014 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 commented on May 20th 2014 Contributor

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 commented on May 20th 2014 Contributor

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

@robocoder commented on May 21st 2014 Contributor

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.

@medic123de commented on May 21st 2014 Contributor

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 commented on May 21st 2014 Member

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 commented on May 21st 2014 Contributor

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?

@mattab commented on May 22nd 2014 Member

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 commented on May 22nd 2014 Member

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 commented on May 22nd 2014 Contributor

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 commented on May 22nd 2014 Member

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 commented on May 22nd 2014 Contributor

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 commented on May 26th 2014 Contributor

Matt, any reason why you did not merge?

@mattab commented on May 27th 2014 Member

@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 commented on May 27th 2014 Contributor

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

@mattab commented on June 2nd 2014 Member

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

@mattab commented on June 2nd 2014 Member

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!

@mattab commented on June 2nd 2014 Member

@medic123de Well done, this was an epic change :8ball:

This Pull Request was closed on June 2nd 2014
Powered by GitHub Issue Mirror