Skip to content

Conversation

@Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Jul 17, 2019

Description

Permit to have multiple messages with the same old name. There are optimisations done that make Pylint a little faster.

Type of Changes

Type
✨ New feature

Related Issue

This is it, after this PR the real review for #1164 can start.

@coveralls
Copy link

coveralls commented Jul 17, 2019

Coverage Status

Coverage increased (+0.007%) to 90.03% when pulling 877a68e on Pierre-Sassoulas:multiple-messages-with-the-same-old-name into ac25a27 on PyCQA:master.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the multiple-messages-with-the-same-old-name branch 3 times, most recently from 44d5236 to 9268001 Compare July 22, 2019 17:44
@Pierre-Sassoulas Pierre-Sassoulas changed the title Multiple messages with the same old name WIP - Multiple messages with the same old name Jul 22, 2019
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the multiple-messages-with-the-same-old-name branch from 9268001 to 79e81fe Compare August 4, 2019 18:39
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Aug 4, 2019
Also fixed spelling in C0112 to C0116 messages following the review
of pull-request pylint-dev#2036 by Ashley Whetter.

See also pylint-dev#2075, pylint-dev#2077, pylint-dev#2262, pylint-dev#2654, pylint-dev#2805, pylint-dev#2809, pylint-dev#2844, pylint-dev#2992
and pylint-dev#3013 for full historical context.
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the multiple-messages-with-the-same-old-name branch from 79e81fe to 6278b85 Compare August 10, 2019 11:40
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Aug 11, 2019

I made some performance review (This is why I removed the MessageId class). Regarding performance, I think this is now making things better but it's not very clear cut:

Benchmark used

Pylint analysing itself (~= 65s) :

Total time

time python3 -m cProfile -s tottime pylint/__main__.py pylint/ > result

Sum of the time for the function containing "message_"

cat result|grep message_|cut -c 14-18|paste -sd+ | bc  

Pylint analysing astroid (~= 1,25s) :

Not shown here because it's too fast.

Before

Total time

68,30s user 0,46s system 99% cpu 1:08,81 total

Sum of the time for the function containing "message_"

.453

Longest message function

ncalls  tottime  percall  cumtime  percall filename:lineno(function)
40359    0.138    0.000    0.369    0.000 message_handler_mix_in.py:175(is_message_enabled)
51443    0.122    0.000    0.140    0.000 message_definition_store.py:152(get_message_definitions)
40359    0.070    0.000    0.091    0.000 message_handler_mix_in.py:197(is_one_message_enabled)
40358    0.028    0.000    0.028    0.000 message_handler_mix_in.py:186(<listcomp>)
9800    0.013    0.000    0.041    0.000 message_handler_mix_in.py:153(_message_symbol)

After

Total time

65,46s user 0,38s system 99% cpu 1:05,92 total

Sum of the time for the function containing "message_"

.573

Longest message function

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
 40396    0.166    0.000    0.542    0.000 message_handler_mix_in.py:173(is_message_enabled)
    51484    0.130    0.000    0.187    0.000 message_id_store.py:108(get_active_msgids)
    51484    0.097    0.000    0.325    0.000 message_definition_store.py:52(get_message_definitions)
    40396    0.066    0.000    0.085    0.000 message_handler_mix_in.py:195(is_one_message_enabled)
    51477    0.040    0.000    0.040    0.000 message_definition_store.py:60(<listcomp>)

Conclusion

It seems that overall the analysis is faster, but the time sepcifically for the part we changed is a little longer. We could probably merge MessageIdStore and MessageDefinitionStore to have less function call (51484 call to get_active_msgids are added) but I think it would affect maintenability a little.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the multiple-messages-with-the-same-old-name branch from 6278b85 to cad617d Compare August 11, 2019 11:29
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the multiple-messages-with-the-same-old-name branch from cad617d to 736db37 Compare August 16, 2019 16:20
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the multiple-messages-with-the-same-old-name branch from 736db37 to 2a266cc Compare August 19, 2019 16:16
@Pierre-Sassoulas Pierre-Sassoulas changed the title WIP - Multiple messages with the same old name Multiple messages with the same old name Aug 19, 2019
@Pierre-Sassoulas
Copy link
Member Author

@PCManticore this one is ready for review, it should be a piece of cake after #2992 ;)

Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Aug 19, 2019
Also fixed spelling in C0112 to C0116 messages following the review
of pull-request pylint-dev#2036 by Ashley Whetter.

See also pylint-dev#2075, pylint-dev#2077, pylint-dev#2262, pylint-dev#2654, pylint-dev#2805, pylint-dev#2809, pylint-dev#2844, pylint-dev#2992
and pylint-dev#3013 for full historical context.
@Pierre-Sassoulas
Copy link
Member Author

Hi, @PCManticore, I won't be super available from mid-September to the end of October, now would be the best time to review this and #3021 :)

@PCManticore PCManticore merged commit 4c2e1a5 into pylint-dev:master Sep 10, 2019
@PCManticore
Copy link
Contributor

Awesome stuff @Pierre-Sassoulas ! 👍Sorry for the delay in getting to this, I was out as well.

Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Sep 10, 2019
Also fixed spelling in C0112 to C0116 messages following the review
of pull-request pylint-dev#2036 by Ashley Whetter.

See also pylint-dev#2075, pylint-dev#2077, pylint-dev#2262, pylint-dev#2654, pylint-dev#2805, pylint-dev#2809, pylint-dev#2844, pylint-dev#2992
and pylint-dev#3013 for full historical context.
@Pierre-Sassoulas Pierre-Sassoulas deleted the multiple-messages-with-the-same-old-name branch September 10, 2019 10:49
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this pull request Sep 10, 2019
Also fixed spelling in C0112 to C0116 messages following the review
of pull-request pylint-dev#2036 by Ashley Whetter.

See also pylint-dev#2075, pylint-dev#2077, pylint-dev#2262, pylint-dev#2654, pylint-dev#2805, pylint-dev#2809, pylint-dev#2844, pylint-dev#2992
and pylint-dev#3013 for full historical context.
PCManticore pushed a commit that referenced this pull request Sep 10, 2019
Also fixed spelling in C0112 to C0116 messages following the review
of pull-request #2036 by Ashley Whetter.

See also #2075, #2077, #2262, #2654, #2805, #2809, #2844, #2992
and #3013 for full historical context.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants