Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Added cache data collector and profiler page#21065

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
Nyholm wants to merge9 commits intosymfony:masterfromNyholm:cache-data-collector

Conversation

@Nyholm
Copy link
Member

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#19297
LicenseMIT
Doc PRn/a

Adding a first version of cache profiler page. Most things are taken from PHP-cache.
FYI:@aequasi

What is included?

A collector, recording adapter and a profiler page.

screen shot 2016-12-27 at 16 07 35
screen shot 2016-12-27 at 16 07 45

What is not included?

  • A good logo
  • Nice design on the profiler page

This PR aims to pass as the minimum requirement for a cache page. Im happy to do fancy extra features but those should be a different PR.

maidmaid, SuperHamouch11, damienalexandre, enleur, Renrhaf, Simperfit, jesuspuras, kbond, ro0NL, tjaari, and 2 more reacted with thumbs up emojiRenrhaf reacted with hooray emoji
$loaders =array_map(function ($loader) {returnnewReference($loader); },$config['loaders']);
$loaders =array_map(function ($loader) {
returnnewReference($loader);
},$config['loaders']);
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This change was from fabbot.io

Choose a reason for hiding this comment

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

you should revert them, fabbot is broken these days

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Okey, thank you


$container->setParameter('templating.engines',$config['engines']);
$engines =array_map(function ($engine) {returnnewReference('templating.engine.'.$engine); },$config['engines']);
$engines =array_map(function ($engine) {
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This change was from fabbot.io

@javiereguiluz
Copy link
Member

@Nyholm thanks for this PR. Don't worry about the logo because the SensioLabs designer will take care of that. And same thing about the design, don't worry because we can iterate it later. Let's focus on the contents and the information provided. Thanks!

@Nyholm
Copy link
MemberAuthor

Nyholm commentedDec 27, 2016•
edited
Loading

That is great. Then Im done with this PR =)

("Done" = "Waiting for community feedback")

@Koc
Copy link
Contributor

Koc commentedDec 27, 2016

It would be very useful collect this information also from production env and analyze it later. Can anybody idea how to make it possible without enabling whole profiler?

@Nyholm
Copy link
MemberAuthor

It would be very useful collect this information also from production env and analyze it later. Can anybody idea how to make it possible without enabling whole profiler?

With the current implementation you should only loadcache_debug.xml and then make sure to store the data from the collector somewhere onkernel.terminate

*
* @return string
*/
privatefunctiongetValueRepresentation($value)
Copy link

@RenrhafRenrhafDec 27, 2016•
edited
Loading

Choose a reason for hiding this comment

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

@Nyholm I'm not sure, but can't we use the VarDumper component here to print the value ? (BTW thanks a lot for this first collector, it's really nice!)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Oh. That is a good idea. I'll see what I can do. Thanks.

Choose a reason for hiding this comment

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

the representation should be handled by the data collector, using the cloneVar method of the base DataCollector

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I cannot use theDataCollector::cloneVar in this context. TheRecordingAdapter is not a data collector. However, I use thecloneVar once the data is moved to theCacheDataCollector

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

@Renrhaf I've updated the PR using your suggestion. Thank you!

Choose a reason for hiding this comment

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

@Nyholm seems good to me ;) thank you !

*
* @return object
*/
privatefunctiontimeCall($name,array$arguments =array())

Choose a reason for hiding this comment

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

what about using a stopwatch instead?

sstok reacted with heart emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sure!

*
* @param $call
*/
privatefunctionaddCall($call)

Choose a reason for hiding this comment

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

this function wrapper for a oneliner could be removed

/**
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
interface RecordingAdapterInterface
Copy link
Member

@nicolas-grekasnicolas-grekasDec 28, 2016•
edited
Loading

Choose a reason for hiding this comment

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

I'd remove this interface entirely: we don't want a contract here, we need only a behavior (an implem)

foreach ($callsas$call) {
$statistics[$name]['calls'] +=1;
$statistics[$name]['time'] +=$call->time;
if ($call->name ==='getItem') {

Choose a reason for hiding this comment

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

what about getItems?

Copy link
MemberAuthor

@NyholmNyholmDec 28, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Thank you. If one do getItems(['a', 'b', 'c']). I will count that as 1 call and 3 reads.

$result =$call->result;
$hits =0;
$items =array();
foreach ($resultas$item) {

Choose a reason for hiding this comment

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

In the case of a generator, I see two issues here:

  • this is consuming the generator. Since these are not rewindable,return $result; returns a non usable generator.
  • this is couting hits, but maybe the app didn't consume the generator on its side.

This means I think we should have two code paths here: one for array (doing the same as the current code),
and one for Traversable, which should wrap this in a generator-closure, and do things lazily.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I've wrapped the result in a generator closure as you suggested. I fail to understand we need a code path for arrays. I know that getItems will return something that you can do foreach with. Why can't I just always wrap it in a generator-closure?

Choose a reason for hiding this comment

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

no strong reason, good also via a generator

* @author Aaron Scherer <aequasi@gmail.com>
* @author Tobias Nyholm <tobias.nyholm@gmail.com>
*/
finalclass RecordingAdapterimplements AdapterInterface
Copy link
Member

@nicolas-grekasnicolas-grekasDec 28, 2016•
edited
Loading

Choose a reason for hiding this comment

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

Oh, I guess this should be named TraceableAdapter for consistency with the code base (eg TraceableUrlMatcher, etc.)

sstok reacted with thumbs up emoji
*
* @return string
*/
privatefunctiongetValueRepresentation($value)

Choose a reason for hiding this comment

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

I think we don't need this logic, let's just use$value directly.
The clone may fail (implem can throw in__clone), and is fragile (not recursive).
and for other types of data, we don't care (resources cannot come up from a cache pool)
removing also will make the layer slimmer, which is also a nice target

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Will do. Thank you.

namespaceSymfony\Component\Cache\Adapter;

usePsr\Cache\CacheItemInterface;
usePsr\Cache\CacheItemPoolInterface;

Choose a reason for hiding this comment

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

not used anymore (mybad)

<?php

/*
* This file is part of php-cache\cache-bundle package.
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong

/*
* This file is part of php-cache\cache-bundle package.
*
* (c) 2015-2015 Aaron Scherer <aequasi@gmail.com>, Tobias Nyholm <tobias.nyholm@gmail.com>
Copy link
Member

Choose a reason for hiding this comment

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

Symfony code must use the Symfony copyright

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sorry, Copy/paste issue.. An my IDE does not show file headers. I will fix these

{%blockhead %}
{{parent() }}
<style>
.color-red {
Copy link
Member

Choose a reason for hiding this comment

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

would be better to include necessary styles in the CSS, to be usable for all collectors

{{include('@WebProfiler/Icon/cache.svg') }}
</span>
<strong>Cache</strong>
<spanclass="count">
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should have the time here anymore, for consistency with other panels

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Do you want me to remove it?


{%blockpanel %}
<h2>Cache</h2>
{%forname,callsincollector.calls %}
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to have the aggregated stats first as big numbers (as done in other panels), before having the detail for each pool

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Done!

Copy link
Member

Choose a reason for hiding this comment

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

no it is not done

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sorry, I misunderstood you. I put the aggregated stats for each adapter asbig numbers.
screen shot 2017-01-02 at 18 46 11

$container->register($id.'.recorder', TraceableAdapter::class)
->setDecoratedService($id)
->addArgument(newReference($id.'.recorder.inner'))
->addArgument(newReference('debug.stopwatch'))
Copy link
Member

Choose a reason for hiding this comment

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

The implementation does not use Stopwatch

@NyholmNyholm mentioned this pull requestDec 28, 2016
@Nyholm
Copy link
MemberAuthor

screen shot 2016-12-28 at 20 39 35
screen shot 2016-12-28 at 20 39 25

@xabbuh
Copy link
Member

Just by looking at the screenshot I was not able to easily recognise what "ratio" refers to. Does it mean hits/reads?

@Nyholm
Copy link
MemberAuthor

Sorry about that. Yes, the ratio is the hits/reads ratio. See new screenshot:

screen shot 2016-12-28 at 20 59 03

FYI: I created aseparate PR with the adapters. I added some tests and fixed some bugs in it. I will rebase this PR when/if#21082 gets merged.

@xabbuh
Copy link
Member

I think I would change the label from "ratio" to "hits/reads" then.

Nyholm reacted with thumbs up emoji

@xabbuh
Copy link
Member

And what about providing aggregated data across all pools?

@Nyholm
Copy link
MemberAuthor

I will.

And what about providing aggregated data across all pools?

That is only in the toolbar atm.

@ro0NL
Copy link
Contributor

Nice :)

What about a tab per pool? That should organize things right?

The table with calls looks rather technical, so maybe we can improve that UX-wise.

  • Try to keep simplekey | value | time headers (only save calls seem to have extra metadata)
  • Differ with colors between gets, deletes or saves (an in-row label or so)

👍

nicolas-grekas added a commit that referenced this pull requestDec 29, 2016
This PR was squashed before being merged into the 3.3-dev branch (closes#21082).Discussion----------[Cache] TraceableAdapter| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | Related to#21065| License       | MIT| Doc PR        | n/aI moved the TraceableAdapter to a separate PR and added some tests. I found a small bug which was fixed.Commits-------5cc4baf [Cache] TraceableAdapter
@Nyholm
Copy link
MemberAuthor

Thank you@ro0NL. I agree with your comments that this could be improved and I really like your suggestions. I think this is "good enough" and I will let other PRs improve this. Does it sound reasonable?

I've rebased this PR on master.

Renrhaf reacted with thumbs up emoji

$statistics[$name]['misses'] +=$count -$call->misses;
}elseif ($call->name ==='hasItem') {
$statistics[$name]['reads'] +=1;
if ($call->result->getRawData()[0][0] ===false) {
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This looks a bit ugly.

$call->result is aSymfony\Component\VarDumper\Cloner\Data. I know thathasItem will always return a boolean thanks to PSR-6. So I believe this is safe.

Choose a reason for hiding this comment

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

No better way for now, I may work on something better soon but this is the way to go for now.

@stof
Copy link
Member

stof commentedJan 2, 2017

And what about providing aggregated data across all pools?

That is only in the toolbar atm.

That's unfortunate, as it means that the main info displayed in the toolbar is not available anymore when looking at details.
Btw, I made the same comment earlier about that (and you said it was done while it is not

@Nyholm
Copy link
MemberAuthor

Nyholm commentedJan 3, 2017•
edited
Loading

Ping@stof

screen shot 2017-01-03 at 10 46 53

@xabbuh
Copy link
Member

@Nyholm Any reason not to aggregate missed and deletes? I would also change the order to be consistent with how it is for the cache pools.

@Nyholm
Copy link
MemberAuthor

I updated the order and added deletes. Thank you for the quick feedback.
screen shot 2017-01-03 at 10 52 16

@rvanlaak
Copy link
Contributor

We chain aapcu pool with afilesystem pool, which means we have three pools in the profiler overview. It would be nice (for a future PR) to think about some chain-specific aggregates, or maybe allow to disable profiler data collection per pool?

From DX perspective it also would be great to have some kind of (javascript?) filtering in the panel, as that would allow you to instantly see the behavior path of a specific cache item.

Nyholm reacted with thumbs up emoji

@Nyholm
Copy link
MemberAuthor

Is there anything else I can do to help getting this PR merged?

@fabpot
Copy link
Member

@Nyholm If you can rebase on current master, I can merge it :)

@nicolas-grekas
Copy link
Member

👍 from me
we should open an issue with the remaining ideas to not forget about them
for this next issue : about the compiler pass, ideally, we could keep track of removed private pools so that the panel show them eg grayed.

@Nyholm
Copy link
MemberAuthor

Thank you. The PR is rebased and tests are all green.

@fabpot
Copy link
Member

Thank you@Nyholm.

@fabpotfabpot closed thisJan 18, 2017
fabpot added a commit that referenced this pull requestJan 18, 2017
This PR was squashed before being merged into the 3.3-dev branch (closes#21065).Discussion----------Added cache data collector and profiler page| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#19297| License       | MIT| Doc PR        | n/aAdding a first version of cache profiler page. Most things are taken from PHP-cache.FYI:@aequasi### What is included?A collector, recording adapter and a profiler page.![screen shot 2016-12-27 at 16 07 35](https://cloud.githubusercontent.com/assets/1275206/21502325/4bee2ed4-cc4f-11e6-89fc-37ed16aca864.png)![screen shot 2016-12-27 at 16 07 45](https://cloud.githubusercontent.com/assets/1275206/21502326/4bee9450-cc4f-11e6-904d-527b7b0ce85b.png)### What is not included?* A good logo* Nice design on the profiler pageThis PR aims to pass as the minimum requirement for a cache page. Im happy to do fancy extra features but those should be a different PR.Commits-------7497f1c Added cache data collector and profiler page
@Nyholm
Copy link
MemberAuthor

Thank you for merging

rvanlaak, Simperfit, and Renrhaf reacted with hooray emoji

@Renrhaf
Copy link

Many thanks@Nyholm :)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@stofstofstof requested changes

+2 more reviewers

@RenrhafRenrhafRenrhaf left review comments

@cryptiklemurcryptiklemurcryptiklemur left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

12 participants

@Nyholm@javiereguiluz@Koc@xabbuh@ro0NL@stof@rvanlaak@fabpot@nicolas-grekas@Renrhaf@cryptiklemur@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp