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

[HttpKernel] FileProfilerStorage remove expired profiles mechanism#47352

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

Merged

Conversation

@alamirault
Copy link
Contributor

@alamiraultalamirault commentedAug 21, 2022
edited
Loading

QA
Branch?6.2
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#45831
LicenseMIT
Doc PRsymfony/symfony-docs#...

This is a first attempt for limit number of profiles saved (discussed in#45831).
When we save new profile, all expired profiles are removed. Expiration is one day for the moment.

Questions:

  • Not sure how to deal, profiles with time0, like an unexpired profile ?
  • I assume profiles are sorted and oldest profile is on firstline. -> avoid readind all index file if first profile is not expired
  • Is there a best way ? (cpu/memory, io efficient) (I'm not sure I have the necessary skills, changes are welcome :))

TODOS:

  • Changelog
  • Complete tests
  • Symfony docs PR

IonBazan and andreybolonin reacted with thumbs up emoji
@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@alamiraultalamiraultforce-pushed thefeature/45831-remove-expired-profiles branch fromc670f8a to6cd2a51CompareAugust 21, 2022 13:27
@alamiraultalamirault changed the title[HttpKernel] FileProfilerStorage has remove expired profiles mechanism[HttpKernel] FileProfilerStorage remove expired profiles mechanismAug 21, 2022
@alamiraultalamirault marked this pull request as ready for reviewAugust 21, 2022 13:34
@carsonbotcarsonbot added this to the6.2 milestoneAug 21, 2022
@alamiraultalamiraultforce-pushed thefeature/45831-remove-expired-profiles branch fromcb75093 tocf10b53CompareOctober 29, 2022 12:26
@nicolas-grekasnicolas-grekas modified the milestones:6.2,6.3Nov 5, 2022
@nicolas-grekasnicolas-grekasforce-pushed thefeature/45831-remove-expired-profiles branch 2 times, most recently fromda36747 to600596fCompareDecember 16, 2022 10:25
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

I updated the PR with the following changes:

  • set expiration at 2 days
  • run randomly 1 / 10
  • never write to index.csv but store a seek offset in index.csv.offset

The seek offset allows looking up for files to prune very quickly, that's why 1/10 is enough IMHO.
index.csv is going to grow but this file is small and doing so saves us from race conditions.

@alamirault
Copy link
ContributorAuthor

Thanks Nicolas, I like your rework !

@fabpot
Copy link
Member

Thank you@alamirault.

alamirault reacted with hooray emoji

@alamiraultalamirault deleted the feature/45831-remove-expired-profiles branchDecember 18, 2022 11:42
@alamiraultalamirault restored the feature/45831-remove-expired-profiles branchDecember 18, 2022 11:53
@fabpotfabpot mentioned this pull requestMay 1, 2023
nicolas-grekas added a commit that referenced this pull requestJul 13, 2023
…age (radar3301)This PR was merged into the 6.3 branch.Discussion----------[HttpKernel] Skip corrupted CSV data in FileProfilerStorage| Q             | A| ------------- | ---| Branch?       | 6.3| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | Partiallyfixes#50942, improves/fixes on PR#50913, partially fixes#50816| License       | MITTechnically, part of this fix could go all the way back to 2.1, but since we're not sure why/how exactly the index.csv file is being corrupted in the first place, this PR should just serve as a stop-gap measure until the root cause can be identified.A corrupted index.csv file is especially noticeable since 6.3, when the "Remove Expired Profiles" feature was added by#47352 .RE: improves/fixes on PR#50913:Warnings are (usually?) only escalated to errors in debug mode, so having a try/catch is a bit more overhead than I think is actually needed, and I believe just checking that the line read from the csv is not corrupted is the better/faster option. Granted, the profiler should really only be active in debug/non-production modes anyway, but "should" and reality usually don't align...`@alamirault` `@MatTheCat` `@benjaminfunk` `@Pelagoss` `@derrabus`Commits-------b4e942d Update FileProfilerStorage.php
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@94noni94noni94noni approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

6.3

Development

Successfully merging this pull request may close these issues.

[WebProfiler] Add a limit how many profiles should be saved

5 participants

@alamirault@carsonbot@fabpot@nicolas-grekas@94noni

[8]ページ先頭

©2009-2025 Movatter.jp