Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

perf: fix RegexIdentifier.check regression #218

Merged
merged 2 commits into from
Oct 22, 2021

Conversation

P403n1x87
Copy link
Contributor

⚠ Pull Requests not made with this template will be automatically closed 🔥

Prerequisites

Why do we need this pull request?

It looks like #167 has introduced a performance regression by adding what seems to be an unnecessary deep copy. A shallow dictionary copy seems to work just fine.

Profiling data collected with Austin shows the before/after difference when running test_json_printing3

Before

regex_identifier_check_before

After

regex_identifier_check_after

What GitHub issues does this fix?

A performance regression

Copy / paste of output

poetry run pywhat fixtures/file | grep -A 2 -B 1 'Twilio Account SID'
Matched on: ACAQDrnjkGtf3iA46rtwsvRiscvMTCw30l
Name: Twilio Account SID
Exploit: Use the command below to verify that the SID is valid:
  $ curl -X GET 'https://api.twilio.com/2010-04-01/Accounts.json' -u ACAQDrnjkGtf3iA46rtwsvRiscvMTCw30l:[AUTH_TOKEN]
--
Matched on: ACAWIQTrhbtfozp14V6UTmPyMVUMT0fjjg
Name: Twilio Account SID
Exploit: Use the command below to verify that the SID is valid:
  $ curl -X GET 'https://api.twilio.com/2010-04-01/Accounts.json' -u ACAWIQTrhbtfozp14V6UTmPyMVUMT0fjjg:[AUTH_TOKEN]
--
Matched on: ACgkQ8jFVDE9H447pKwD6A5xwUqIDprBzr
Name: Twilio Account SID
Exploit: Use the command below to verify that the SID is valid:
  $ curl -X GET 'https://api.twilio.com/2010-04-01/Accounts.json' -u ACgkQ8jFVDE9H447pKwD6A5xwUqIDprBzr:[AUTH_TOKEN]

It looks like bee-san#167 has introduced a performance regression
by adding what seems to be an unnecessary deep copy. A
shallow dictionary copy seems to work just fine.
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2021

Codecov Report

Merging #218 (a80cabd) into main (b04de11) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #218      +/-   ##
==========================================
- Coverage   92.52%   92.51%   -0.01%     
==========================================
  Files          14       14              
  Lines        1204     1203       -1     
==========================================
- Hits         1114     1113       -1     
  Misses         90       90              
Impacted Files Coverage Δ
pywhat/regex_identifier.py 92.50% <100.00%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b04de11...a80cabd. Read the comment docs.

Copy link
Owner

@bee-san bee-san left a comment

Choose a reason for hiding this comment

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

This is super cool 👏🏻

@bee-san bee-san enabled auto-merge October 22, 2021 08:57
@bee-san bee-san merged commit 0ef8cc8 into bee-san:main Oct 22, 2021
@P403n1x87 P403n1x87 deleted the perf-regex-identifier-check-shallow branch October 22, 2021 10:46
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants