Skip to content

Slow performance in dev mode since "Use VarDumper in the profiler" #19978

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
Closed
gharlan opened this issue Sep 19, 2016 · 12 comments
Closed

Slow performance in dev mode since "Use VarDumper in the profiler" #19978

gharlan opened this issue Sep 19, 2016 · 12 comments

Comments

@gharlan
Copy link
Contributor

gharlan commented Sep 19, 2016 •
edited
Loading

Since 41a7649 forms are very slow in dev mode.

Example: Symfony-Demo, PostType (/de/admin/post/1/edit), added these two fields:

        $builder
        // ...
            ->add('country', CountryType::class, [
                'mapped' => false,
            ])
            ->add('choice', ChoiceType::class, [
                'mapped' => false,
                'choices' => range(1, 10),
                'expanded' => true,
                'multiple' => true,
            ])

Blackfire comparison (from commit before 41a7649 to current master): +645%, 227 ms → 1.69 s

In a real-world form in a project it is: 180 ms → 4.2 s

ping @wouterj
@gharlan
Copy link
Contributor Author

gharlan commented Sep 19, 2016

The memory usage difference is also remarkable: +501% (see blackfire comparison)

@fabpot
Copy link
Member

fabpot commented Sep 19, 2016

ping @nicolas-grekas

@nicolas-grekas nicolas-grekas mentioned this issue Sep 20, 2016
Merged
@nicolas-grekas
Copy link
Member

@gharlan can you please try #19986?

@gharlan
Copy link
Contributor Author

gharlan commented Sep 20, 2016

Thanks a lot, much better, but still slower than before 41a7649

My real world form (measured with symfony profiler):
commit before 41a7649: ~ 180 ms
41a7649: ~ 4.2 s
your pr: ~ 500 ms

@gharlan
Copy link
Contributor Author

gharlan commented Sep 20, 2016 •
edited
Loading

Symfony Demo with two additional fields (blackfire)
commit before 41a7649: 227ms
current master: 1.69 s
your pr: 363 ms

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 20, 2016 •
edited
Loading

Can you try changing the setMaxItems(50) to setMaxItems(1) in FormDataCollector to see if it gets acceptable? We'll then have to decide which number between 1 & 50 is best for the use case.

@gharlan
Copy link
Contributor Author

gharlan commented Sep 20, 2016

my real world form with..
setMaxItems(1): ~ 340 ms
setMaxItems(10): ~ 400 ms
setMaxItems(20): ~ 425 ms
setMaxItems(30): ~ 445 ms
setMaxItems(40): ~ 460 ms
setMaxItems(50): ~ 470 ms (500 ms was too high in previous comment)

@nicolas-grekas
Copy link
Member

So, should we stop here? Is 50 OK or should be lower it? To me it's OK if we're slower than before: this time is added only to kernel.terminate. And the benefit is worth it (look at the profiler how beautiful it is :) )
WDYT?

@gharlan
Copy link
Contributor Author

gharlan commented Sep 20, 2016 •
edited
Loading

Let us go with 50. 👍 Imo it is not worth to decrease the number.

Also: I think my real world form has an unusual amount of checkboxes etc.
4.2 s wouldn't be acceptable, but I think 470 ms is ok for this case. ;)

look at the profiler how beautiful it is :)

Didn't pay attention to it until now, only had a look to the performance difference. ;)
Loving it! ❤️ Thanks all of you.

@nicolas-grekas
Copy link
Member

PR updated, not to 50, but to limit to the 2nd depth level.

fabpot added a commit that referenced this issue Sep 21, 2016
@fabpot
bug #19986 [Form][EventDispatcher] Fix VarDumper usage related to per…
e09510a 
…f regression (nicolas-grekas)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Form][EventDispatcher] Fix VarDumper usage related to perf regression

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #19978
| License       | MIT
| Doc PR        | -

Commits
-------

294868e [Form][EventDispatcher] Fix VarDumper usage related to perf regression
@fabpot fabpot closed this as completed Sep 21, 2016
@gharlan
Copy link
Contributor Author

gharlan commented Sep 22, 2016

@nicolas-grekas updated to current master. It is much slower than your pr 2 days ago.

commit before 41a7649: ~ 180 ms
41a7649: ~ 4.2 s
your pr 2 days ago: ~ 470 ms
current master: ~  3.7 s

@nicolas-grekas
Copy link
Member

See #20022, setMaxItems(25)

@gharlan gharlan mentioned this issue Sep 22, 2016
Merged
@nicolas-grekas nicolas-grekas mentioned this issue Feb 20, 2017
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@fabpot @nicolas-grekas @gharlan