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

Optimize Translator.php call and EventDispatcher.php #15990

Closed
wants to merge 3 commits into from
Closed

Optimize Translator.php call and EventDispatcher.php #15990

wants to merge 3 commits into from

Conversation

tolbon
Copy link
Contributor

@tolbon tolbon commented May 26, 2020

TAKE WITH CAUTION.

I inspect what function have lot of own time in matomo
I see this :
Screenshot from 2020-05-26 12-03-00
After some modification "own time" decreased (the 3 columns, time in ms)
Screenshot from 2020-05-26 12-02-18

cause some functions like count, is_array, is_string ... if you add \ before PHP optimize this call.

the bench is made in this page. I refresh and aggregates 8 cachegrind files
Screenshot from 2020-05-26 12-07-32

@tolbon
Copy link
Contributor Author

tolbon commented May 26, 2020

Can you validate this optimization ? I am not sure with measure.

@tsteur
Copy link
Member

tsteur commented May 26, 2020

@tolbon could you also let us know in the cachegrind what the column name is for each number? We have been doing quite a few profiles in the past and these changes shouldn't really have an impact on performance.

When doing the profiles make sure Matomo development mode is disabled and caches are warmed up for example. Also be good to make sure to test the profiles several times as they can vary sometimes quite a bit.

Is there any chance you can share the full cachegrind file? I suppose the file be quite large?

@tolbon
Copy link
Contributor Author

tolbon commented May 27, 2020

@tolbon could you also let us know in the cachegrind what the column name is for each number?

Name, Time, Own Time, Memory, OwnMemory, NbCalls

We have been doing quite a few profiles in the past and these changes shouldn't really have an impact on performance.

I am not sure It's a PHP7+ optimization. maybe if you do this before explain what.

I am using matomo docker example

@tsteur
Copy link
Member

tsteur commented May 27, 2020

BTW looking at the 2 screenshots again. Do you notice that the methods are called less often in the second run? Be great to profile the identical request several times and compare the several requests and to make sure there's nothing else running on the server for accurate results.

We've done a lot of profiling on PHP 7+ and from what we know this should make like absolutely no difference. Could be wrong though.

As mentioned with the profiling be great to ensure to compare the same request, run the profile several times cause there will be always some minor differences, check that nothing else is running on that server that might consume CPU for example.

If you can send us a full cachegrind file that be really interesting to look at

@tolbon
Copy link
Contributor Author

tolbon commented May 27, 2020

Hello,
https://send.firefox.com/download/507f7da404b3e24b/#r0qyhY_ylNwaxAUX_MSTog
(72Mo)
V1 is original
V2 is Just this commit
V3 is PHP-CS-Fixer fix --rules=native_constant_invocation (Add leading \ before constant invocation of internal constant to speed up resolving)

I know V2 miss one file Don’t know why.

You work on it always. You have certainly right about no effect of this PR.

@tsteur
Copy link
Member

tsteur commented May 27, 2020

Had a look at some and compared them. Quite interesting the result. To know how accurate the profiles are be great if you could confirm you tested this ideally on a server where nothing else is running, and ideally would need to run the same test quite often to basically end up with some averages. Eg if you run the same test with the same code again, you might notice also quite a difference each time you run it.

@tolbon
Copy link
Contributor Author

tolbon commented May 27, 2020

Ok I see. that’s why I prefer you to test you or your team in your side with this modification.

Add slash not change code result but let php microOptimize.

@tolbon
Copy link
Contributor Author

tolbon commented May 29, 2020

@tsteur
Just a question about code :
In Piwik/CacheId::pluginAware();
sometimes it's call like +1 000 times and call this md5(implode('', $pluginNames)).
I am not sure but I think md5(implode()) return same value +1 000 and recompute this and take 5Mo Ram.
If it's same result I don't know how to save result of this and stock them somewhere cause it's in static call. Maybe create a method in Piwik\Plugin\Manager.php.

Why I search that ? Cause in cachegrind files check memory consumption and I see this function.
I search why Common::getRequestVar take some RAM too

@tsteur
Copy link
Member

tsteur commented Jun 1, 2020

If it's same result I don't know how to save result of this and stock them somewhere cause it's in static call. Maybe create a method in Piwik\Plugin\Manager.php.

We actually don't want to save the result as when this method is used, it's important to detect any change in plugins etc.

5MB memory doesn't sound too bad? Although it should technically free the memory each time. Not sure if you get to see peak memory usage etc.

Generally that method shouldn't cause much of an issue.

Be keen to learn more about your profiling. Are you having memory issues? or performance issues with Matomo? Cause the tracking requests should consume very little memory and the DB takes typically most of the time. In the UI the highest memory consumption usually comes from reading reports (unserialising all the rows, merging rows from reports when needed etc). Performance wise there's a bit that can be done here and there but it might be not that trivial to make it more than 10% faster.

BTW we're using here the tideways profiler, some use https://blackfire.io/

@tolbon
Copy link
Contributor Author

tolbon commented Jun 2, 2020

No no it's not a leak I just ask. it's like compute 2 + 2 = 4 (1000 time during a request) if the result is the same 1000 times I think it's bad to recompute it 999 times.

ok next time I go with blackfire profile :)

@tolbon tolbon closed this Jun 2, 2020
@tsteur
Copy link
Member

tsteur commented Jun 2, 2020

BTW... end of the year PHP 8 should be released which might come with quite some big performance boost without needing to do any change. It's obviously to be seen if there is a performance boost and how much.

@tolbon
Copy link
Contributor Author

tolbon commented Jun 2, 2020

Yeah very excited about PHP 8 + JIT.
I don’t see I close this PR 😅

@mattab mattab added this to the 4.0.0 milestone Sep 28, 2020
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

3 participants