FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

v3.2.x: Fix leaks in C++ code by QuLogic · Pull Request #17468 · matplotlib/matplotlib · GitHub

Repository navigation

v3.2.x: Fix leaks in C++ code - #17468

Merged
tacaswell merged 3 commits into
matplotlib:v3.2.xfrom
QuLogic:v3.2.x-cpp-leaks
May 21, 2020
Merged

tacaswell merged 3 commits into
matplotlib:v3.2.xfrom
QuLogic:v3.2.x-cpp-leaks

Conversation

QuLogic commented May 20, 2020

Copy link
Copy Markdown
Member

PR Summary

This is a cherry-pick of the leak fix in #17458, and another leak fix in the png code (which doesn't exist on master.)

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • [N/A] New features are documented, with examples if plot related
  • [N/A] Documentation is sphinx and numpydoc compliant
  • [N/A] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • [N/A] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

QuLogic added Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. Performance labels May 20, 2020
QuLogic added this to the v3.2.2 milestone May 20, 2020
Comment thread src/_png.cpp
PyObject *meta_key, *meta_val;
Py_ssize_t pos = 0;
int meta_pos = 0;
std::vector<PyObject*> temp_strs;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

don't know if it is worth it, but when doing push_back()s to a vector, one can save a fair amount of memory and allocation time by doing temp_strs.reserve(meta_size); before the loop for strings since we know the upper limit for this vector.

https://en.cppreference.com/w/cpp/container/vector/reserve

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I expect it won't be more than 10 or maybe 20, but I've added it since we do know the limit.

WeatherGod left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

seems to make sense to me.

QuLogic added 2 commits May 21, 2020 15:43
If `info_ptr` isn't created, we should still destroy `png_ptr`.
QuLogic force-pushed the v3.2.x-cpp-leaks branch from 23fa03e to 8e13db1 Compare May 21, 2020 19:54
tacaswell merged commit b47e31d into matplotlib:v3.2.x May 21, 2020
QuLogic deleted the v3.2.x-cpp-leaks branch May 21, 2020 21:47
tacaswell mentioned this pull request May 22, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Performance Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL