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

[Debug] More aggressively aggregate silenced notices per file+line #24895

Merged
merged 1 commit into from Nov 10, 2017

Conversation

Projects
None yet
6 participants
@nicolas-grekas
Member

nicolas-grekas commented Nov 10, 2017

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

By aggregating silenced notices (deprecations mostly) per file+line instead of just per message, we loose some accuracy, but gain performance.

@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Nov 10, 2017

Member

Do we have any idea about how significant this change can be? (performance-wise)

Member

sroze commented Nov 10, 2017

Do we have any idea about how significant this change can be? (performance-wise)

@iltar

This comment has been minimized.

Show comment
Hide comment
@iltar

iltar Nov 10, 2017

Contributor

I'm not really seeing a performance difference (sadly). Used 3.4.0-BETA3 as reference vs your commit.

Before your patch

image

image

After your patch

image

image

Either that was not what was causing it, or there's something wrong on my end, but I did double check the results.

Contributor

iltar commented Nov 10, 2017

I'm not really seeing a performance difference (sadly). Used 3.4.0-BETA3 as reference vs your commit.

Before your patch

image

image

After your patch

image

image

Either that was not what was causing it, or there's something wrong on my end, but I did double check the results.

@fabpot

fabpot approved these changes Nov 10, 2017

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Nov 10, 2017

Member

Thank you @nicolas-grekas.

Member

fabpot commented Nov 10, 2017

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 9ab7559 into symfony:3.4 Nov 10, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Nov 10, 2017

minor #24895 [Debug] More aggressively aggregate silenced notices per…
… file+line (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[Debug] More aggressively aggregate silenced notices per file+line

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

By aggregating silenced notices (deprecations mostly) per file+line instead of just per message, we loose some accuracy, but gain performance.

Commits
-------

9ab7559 [Debug] More aggressively aggregate silenced notices per file+line
@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Nov 10, 2017

Member

I've tested this and all the other perf. improvements done recently between beta3 and 3.4.x-dev. Results are impressive:

BEFORE - timeline

before - homepage

AFTER - timeline

after - homepage

As Nicolas said, the problem is that some deprecation logs lose information (trace 1 maintains everything but trace 2 is lost):

BEFORE

Trace 1 Trace 2
before - trace 1 before - trace 2

AFTER

Trace 1 Trace 2
after - trace 1 after - trace 2

The Blackfire comparisons are awesome too:

Comparison 1

https://blackfire.io/profiles/compare/aec26970-f664-4344-9231-642289039428/graph

comparison 1

Comparison 2

https://blackfire.io/profiles/compare/459084bd-ea27-4cea-87d8-e5dfa9cd5564/graph

comparison 2

@nicolas-grekas thank you for your amazing work with all these improvements!!

Member

javiereguiluz commented Nov 10, 2017

I've tested this and all the other perf. improvements done recently between beta3 and 3.4.x-dev. Results are impressive:

BEFORE - timeline

before - homepage

AFTER - timeline

after - homepage

As Nicolas said, the problem is that some deprecation logs lose information (trace 1 maintains everything but trace 2 is lost):

BEFORE

Trace 1 Trace 2
before - trace 1 before - trace 2

AFTER

Trace 1 Trace 2
after - trace 1 after - trace 2

The Blackfire comparisons are awesome too:

Comparison 1

https://blackfire.io/profiles/compare/aec26970-f664-4344-9231-642289039428/graph

comparison 1

Comparison 2

https://blackfire.io/profiles/compare/459084bd-ea27-4cea-87d8-e5dfa9cd5564/graph

comparison 2

@nicolas-grekas thank you for your amazing work with all these improvements!!

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:deprec-aggreg branch Nov 10, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Nov 10, 2017

Member

Cool, thanks for the confirmation! Happy it gives so good results!

Member

nicolas-grekas commented Nov 10, 2017

Cool, thanks for the confirmation! Happy it gives so good results!

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