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

PERF: Break reference cycle for all Index types #27840

Merged
merged 2 commits into from Aug 14, 2019

Conversation

@topper-123
Copy link
Contributor

commented Aug 9, 2019

  • xref #27585 & #27607
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

Generalizes the test in #27607 to be used for all index types. This finds ref cycle issues for PeriodIndex and CategoricalIndex which are solved in this PR.

@TomAugspurger
Copy link
Contributor

left a comment

@TomAugspurger TomAugspurger added this to the 0.25.1 milestone Aug 9, 2019

@jreback
Copy link
Contributor

left a comment

looks good -
question and comment

doc/source/whatsnew/v0.25.1.rst Outdated Show resolved Hide resolved
@@ -441,7 +442,9 @@ def _formatter_func(self):

@cache_readonly
def _engine(self):
return self._engine_type(lambda: self, len(self))
# To avoid a reference cycle, pass a weakref of self to _engine_type.
period = weakref.ref(self)

This comment has been minimized.

Copy link
@jreback

jreback Aug 9, 2019

Contributor

should we be using weakref in the other invocations for consistency?

This comment has been minimized.

Copy link
@topper-123

topper-123 Aug 9, 2019

Author Contributor

weakrefs cause some tests to fail.

I'm not sure currently if those failures are correct, could be worth an investigation. @crepererum , did you investigate if weakrefs not working is the correct behaviour?

This comment has been minimized.

Copy link
@crepererum

crepererum Aug 12, 2019

Contributor

No, I didn't test this.

IMHO, the weakref isn't required in the other cases since we're passing a member of self and not self itself. Using a weakref seemed to make things only unnecessary complicated, since then you have just a chain of indirections (lambda->scope->weakref->self->member instead of lambda->scope->member).

@crepererum

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

Thanks for providing this extended fix :)

@topper-123 topper-123 force-pushed the topper-123:breaf_index_ref_cycles branch from 86bf0fe to 36061a1 Aug 14, 2019

@topper-123

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

I think this should be ok to pull in after the rebase?

@TomAugspurger

This comment has been minimized.

Copy link
Contributor

commented Aug 14, 2019

Agreed, thanks!

@TomAugspurger TomAugspurger merged commit fae56d0 into pandas-dev:master Aug 14, 2019

15 checks passed

codecov/patch 100% of diff hit (target 50%)
Details
codecov/project 93.03% (target 82%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #20190814.6 succeeded
Details
pandas-dev.pandas (Checks) Checks succeeded
Details
pandas-dev.pandas (Docs) Docs succeeded
Details
pandas-dev.pandas (Linux py35_compat) Linux py35_compat succeeded
Details
pandas-dev.pandas (Linux py36_32bit) Linux py36_32bit succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow) Linux py36_locale_slow succeeded
Details
pandas-dev.pandas (Linux py36_locale_slow_old_np) Linux py36_locale_slow_old_np succeeded
Details
pandas-dev.pandas (Linux py37_locale) Linux py37_locale succeeded
Details
pandas-dev.pandas (Linux py37_np_dev) Linux py37_np_dev succeeded
Details
pandas-dev.pandas (Windows py36_np15) Windows py36_np15 succeeded
Details
pandas-dev.pandas (Windows py37_np141) Windows py37_np141 succeeded
Details
pandas-dev.pandas (macOS py35_macos) macOS py35_macos succeeded
Details
meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Aug 14, 2019

@topper-123 topper-123 deleted the topper-123:breaf_index_ref_cycles branch Aug 14, 2019

jreback added a commit that referenced this pull request Aug 14, 2019
quintusdias added a commit to quintusdias/pandas_dev that referenced this pull request Aug 16, 2019
PERF: Break reference cycle for all Index types (pandas-dev#27840)
* Generalize guards for index ref cycles

* add issue number
gabriellm1 added a commit to gabriellm1/pandas that referenced this pull request Aug 27, 2019
PERF: Break reference cycle for all Index types (pandas-dev#27840)
* Generalize guards for index ref cycles

* add issue number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.