Skip to content
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

Performance improvments #470

Merged
merged 17 commits into from Oct 10, 2019
Merged

Performance improvments #470

merged 17 commits into from Oct 10, 2019

Conversation

Rafiot
Copy link
Member

@Rafiot Rafiot commented Oct 3, 2019

Need to be checked.

@coveralls
Copy link

coveralls commented Oct 3, 2019

Coverage Status

Coverage increased (+0.04%) to 38.232% when pulling 2785d00 on perf into 742a579 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 38.948% when pulling a2b66e9 on perf into 742a579 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 38.948% when pulling a2b66e9 on perf into 742a579 on master.

mback2k and others added 7 commits October 3, 2019 19:30
- Path and modified time of JSON file are used as the cache key
- Global state is hidden away inside a root-class for re-use
- Maximum size is 150 considering the number of JSON definitions

During my tests the memory usage of the test suites was halved.
90 days and counting, folks.
@codecov
Copy link

codecov bot commented Oct 7, 2019

Codecov Report

Merging #470 into master will increase coverage by 0.07%.
The diff coverage is 63.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
+ Coverage   35.97%   36.05%   +0.07%     
==========================================
  Files          35       35              
  Lines        6679     6712      +33     
==========================================
+ Hits         2403     2420      +17     
- Misses       4276     4292      +16
Impacted Files Coverage Δ
tests/testlive_comprehensive.py 0% <0%> (ø) ⬆️
pymisp/tools/sshauthkeyobject.py 38.09% <0%> (ø) ⬆️
pymisp/tools/fileobject.py 30.64% <0%> (ø) ⬆️
pymisp/tools/peobject.py 22.54% <0%> (ø) ⬆️
pymisp/tools/emailobject.py 21.56% <0%> (ø) ⬆️
tests/test.py 0% <0%> (ø) ⬆️
pymisp/tools/machoobject.py 26.86% <0%> (ø) ⬆️
pymisp/tools/elfobject.py 25.71% <0%> (ø) ⬆️
pymisp/api.py 91.42% <100%> (+1.95%) ⬆️
pymisp/__init__.py 83.78% <100%> (ø) ⬆️
... and 6 more

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 742a579...2785d00. Read the comment docs.

Copy link
Contributor

@mback2k mback2k left a comment

Choose a reason for hiding this comment

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

@Rafiot Thanks for merging my PR. It looks good so far, but I have some comments you may want to look at.

I am currently testing this PR in our application and will report back about the result soon.

pymisp/abstract.py Show resolved Hide resolved
pymisp/api.py Outdated Show resolved Hide resolved
pymisp/aping.py Outdated Show resolved Hide resolved
pymisp/mispevent.py Show resolved Hide resolved
pymisp/abstract.py Show resolved Hide resolved
@Rafiot
Copy link
Member Author

Rafiot commented Oct 8, 2019

Okay, my test cases are passing now... let me know how that goes for you.

@mback2k
Copy link
Contributor

mback2k commented Oct 8, 2019

Thanks, with the previous changes we still had huge memory usage. So I am going to give the latest changes another try and will report back later today.

pymisp/abstract.py Outdated Show resolved Hide resolved
@mback2k
Copy link
Contributor

mback2k commented Oct 9, 2019

Sorry for my delayed response regarding my results. Unfortunately I am still analyzing the memory usage, because we are still hitting some limits at the moment. I hope to find the root cause soon.

@Rafiot
Copy link
Member Author

Rafiot commented Oct 9, 2019

Hmmm that's weird. If you give a try to rapidjson, the memory use was a little bit lower on my end.

@Rafiot Rafiot marked this pull request as ready for review October 10, 2019 07:45
@Rafiot
Copy link
Member Author

Rafiot commented Oct 10, 2019

Ok, I think it is ready to be merged, the tests are passing, and it is already an improvement.

Please let me know if you figure out what is going on on your side, and where we can improve the code from there.

@mback2k
Copy link
Contributor

mback2k commented Oct 10, 2019

I am going to test the latest changes now.

@Rafiot Rafiot merged commit 2785d00 into master Oct 10, 2019
Copy link
Contributor

@mback2k mback2k left a comment

Choose a reason for hiding this comment

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

Sorry, I forgot to submit this pending review. Have you seen my comment regarding remote_describe_types = describe_types?

pymisp/api.py Show resolved Hide resolved
@Rafiot Rafiot deleted the perf branch July 7, 2020 23:47
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.

None yet

3 participants