8000 Sync leaking memory by hluchas · Pull Request #9299 · mautic/mautic · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Sync leaking memory #9299

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
May 15, 2021
Merged

Sync leaking memory #9299

merged 13 commits into from
May 15, 2021

Conversation

hluchas
Copy link
Contributor
@hluchas hluchas commented Oct 14, 2020 •
edited
Loading
Q A
Branch? 3.0 for bug fixes
Bug fix? yes
New feature? no
Deprecations? no
BC breaks? no
Automated tests included? yes
Related user documentation PR URL -
Related developer documentation PR URL -
Issue(s) addressed -

Description:

Sync is leaking memory with huge amount of data. After measurement we decided to fix growing array in CommonRepository::usedParameterNames

Steps to test this PR:

  1. Load up this PR
  2. Try to sync data with any plugin
  3. Try to use segments & segment build
  4. View contacts in UI

@cla-bot cla-bot bot added the cla-signed The PR contributors have signed the contributors agreement label Oct 14, 2020
@hluchas hluchas added bug Issues or PR's relating to bugs performance-scalability Anything related to performance and scalability labels Oct 14, 2020
@TravisBuddy
Copy link

Travis tests have failed

Hey @hluchas,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 55404f80-0e2c-11eb-964d-5d54b4986782

@hluchas
Copy link
Contributor Author
hluchas commented Oct 14, 2020

This has the same problem as here #9297 (comment) , but target is correct for bugfix

@RCheesley RCheesley added the hacktoberfest Issues that would be great for Hacktoberfest participants to work on label Oct 26, 2020
@escopecz
Copy link
Member

@hluchas can you double-check? The 3.1 branch and the staging branch both have the condition correct: https://github.com/mautic/mautic/blob/3.1/app/bundles/FormBundle/Helper/FormUploader.php#L154 https://github.com/mautic/mautic/blob/staging/app/bundles/FormBundle/Helper/FormUploader.php#L154

Your base must be off.

@escopecz escopecz added the pending-feedback PR's and issues that are awaiting feedback from the author label Oct 28, 2020
@hluchas
Copy link
Contributor Author
hluchas commented Oct 29, 2020

@escopecz Fixed in 7820a2f
Taking a look on #9297 with the same prob
TY!

@codecov
Copy link
codecov bot commented Oct 29, 2020 •
edited
Loading

Codecov Report

Merging #9299 (b613af0) into features (28ba55e) will increase coverage by 9.22%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##             features    #9299      +/-   ##
==============================================
+ Coverage       31.96%   41.19%   +9.22%     
- Complexity      33592    34555     +963     
==============================================
  Files            1946     2060     +114     
  Lines          115952   111477    -4475     
==============================================
+ Hits            37066    45923    +8857     
+ Misses          78886    65554   -13332
Impacted Files Coverage Δ Complexity Δ
app/bundles/UserBundle/Event/UserEvent.php 0.00% <0.00%> (-71.43%) 3.00% <0.00%> (ø%)
...dles/CoreBundle/Controller/ExceptionController.php 0.00% <0.00%> (-67.25%) 22.00% <0.00%> (ø%)
...les/CoreBundle/EventListener/ExceptionListener.php 8.82% <0.00%> (-32.36%) 11.00% <0.00%> (ø%)
...ty/Cryptography/Cipher/Symmetric/OpenSSLCipher.php 26.66% <0.00%> (-30.01%) 10.00% <0.00%> (ø%)
...dles/PluginBundle/EventListener/LeadSubscriber.php 72.22% <0.00%> (-27.78%) 5.00% <0.00%> (ø%)
...undles/UserBundle/EventListener/UserSubscriber.php 17.39% <0.00%> (-21.74%) 10.00% <0.00%> (ø%)
...ndle/Security/Authentication/Token/PluginToken.php 56.52% <0.00%> (-21.74%) 8.00% <0.00%> (ø%)
app/bundles/PointBundle/Entity/Trigger.php 22.82% <0.00%> (-18.48%) 28.00% <0.00%> (ø%)
...IntegrationsBundle/Bundle/AbstractPluginBundle.php 0.00% <0.00%> (-16.67%) 2.00% <0.00%> (-2.00%)
.../bundles/PluginBundle/Entity/IntegrationEntity.php 76.62% <0.00%> (-15.59%) 19.00% <0.00%> (ø%)
... and 1004 more

kuzmany
kuzmany previously approved these changes Jan 26, 2021
View reviewed changes
Copy link
Member
@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

Works for me.
I sync our integration and rebuild segments.
Code looks good.
Unit tests included, then approve it 👍

@kuzmany kuzmany added the pending-test-confirmation PR's that require one test before they can be merged label Jan 26, 2021
@kuzmany kuzmany added this to the 3.3.0 milestone Jan 26, 2021
@npracht npracht removed the pending-feedback PR's and issues that are awaiting feedback from the author label Jan 27, 2021
@RCheesley RCheesley changed the base branch from 3.1 to features February 12, 2021 22:34
@RCheesley RCheesley dismissed kuzmany’s stale review February 12, 2021 22:34

The base branch was changed.

@RCheesley
Copy link
Member

GTG when tests pass based on the dismissed review and that it has test coverage.

@RCheesley RCheesley added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged T1 Low difficulty to fix (issue) or test (PR) and removed pending-test-confirmation PR's that require one test before they can be merged labels Feb 12, 2021
@RCheesley RCheesley removed the ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged label Feb 13, 2021
@RCheesley RCheesley added the needs-automated-tests PR's that need automated tests before they can be merged label Feb 13, 2021
@npracht npracht modified the milestones: 3.3.0, 3.3.1 Feb 15, 2021
@hluchas hluchas requested a review from kuzmany February 15, 2021 15:21
@hluchas
Copy link
Contributor Author
hluchas commented Feb 15, 2021

I added missing coverage ;-)

@RCheesley RCheesley modified the milestones: 3.3.1, 3.3.2 Feb 23, 2021
@kuzmany kuzmany added pending-test-confirmation PR's that require one test before they can be merged and removed needs-automated-tests PR's that need automated tests before they can be merged labels Mar 13, 2021
@RCheesley RCheesley modified the milestones: 3.3.2, 3.3.3 Mar 15, 2021
@RCheesley RCheesley modified the milestones: 3.3.3, 4.0-beta Apr 16, 2021
@npracht npracht modified the milestones: 4.0-beta, 4.0-rc May 14, 2021
@RCheesley RCheesley merged commit 7728247 into mautic:features May 15, 2021
@RCheesley
Copy link
Member

@all-contributors please add @hluchas for code

@allcontributors allcontributors bot mentioned this pull request May 15, 2021
Merged
@allcontributors
Copy link
Contributor

@RCheesley

I've put up a pull request to add @hluchas! 🎉

@RCheesley RCheesley modified the milestones: 4.0-rc, 4.0-beta May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kuzmany kuzmany kuzmany approved these changes

Assignees
No one assigned
Labels
bug Issues or PR's relating to bugs cla-signed The PR contributors have signed the contributors agreement hacktoberfest Issues that would be great for Hacktoberfest participants to work on pending-test-confirmation PR's that require one test before they can be merged performance-scalability Anything related to performance and scalability T1 Low difficulty to fix (issue) or test (PR)
Projects
None yet
Milestone
4.0.0-beta
Development

Successfully merging this pull request may close these issues.
7 participants
@hluchas @TravisBuddy @escopecz @RCheesley @kuzmany @npracht @dennisameling
0