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

Use actual code context for enumerating #1721

Merged
merged 6 commits into from
Sep 2, 2023

Conversation

BCSharp
Copy link
Member

@BCSharp BCSharp commented Aug 8, 2023

Partially addresses #1718 (when the wrapping type is list).

@slozier
Copy link
Contributor

slozier commented Aug 9, 2023

Interesting... any ideas about the underlying issue? Seems like the leak comes back when we replace list with tuple. Do we need to try to eliminate all calls that use the default context?

@BCSharp
Copy link
Member Author

BCSharp commented Aug 9, 2023

Do we need to try to eliminate all calls that use the default context?

I think so. The underlying issue appears to be that using the default context with objects of an arbitrary Python type (like OrderedDict, not any builtin types or .NET types) pollutes the conversion call sites (and probably any call sites) with types that cease to exist after the runtime instance is shut down.

I think it may be OK to use the default context for operations on objects of guaranteed .NET types or builtins, but not when an arbitrary object may appear as argument.

There should be more places still when the leak happens. E.g. I expect a leak when an OderedDict is splatted in a call with keyword arguments (untested, just my thought based on reading the code).

@BCSharp BCSharp marked this pull request as ready for review August 30, 2023 00:01
Copy link
Contributor

@slozier slozier left a comment

Choose a reason for hiding this comment

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

Sorry it took a while to review, just noticed it was no longer marked as draft...

Looks good to me.

@BCSharp BCSharp merged commit e8ed79b into IronLanguages:master Sep 2, 2023
5 of 8 checks passed
@BCSharp BCSharp deleted the ListLeak branch September 2, 2023 19:02
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.

2 participants