8000 Investigate greedy GC breaking some test · Issue #2185 · pyscript/pyscript · GitHub
[go: up one dir, main page]

Skip to content

Investigate greedy GC breaking some test #2185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maint 8000 ainers 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

Closed
WebReflection opened this issue Sep 26, 2024 · 4 comments · Fixed by #2187
Closed

Investigate greedy GC breaking some test #2185

WebReflection opened this issue Sep 26, 2024 · 4 comments · Fixed by #2187
Labels
needs-triage Issue needs triage type: bug Something isn't working

Comments

@WebReflection
Copy link
Contributor
WebReflection commented Sep 26, 2024

What happened?

After latest tests update and latest playwright module landed, we've seen sketchy results and it looks like coincident receives GC calls to free local references even if these are used later on in the program.

A polyscript change was required to fix one test but now there's another test failing mostly only with playwright, although I can reproduce the issue via rage-clicking GC button in the performance devtools' panel to see the error myself, which is half good-news (I have a way to investigate that).

Although workers / main dance is fully orchestrated via numbers and it's not really easy to debug/understand what's going on, meaning:

  • I should likely improve coincident debug-ability with way more descriptive logs
  • I should likely avoid smart cached references in polyscript that might result into early GC calls for no reason
  • I should be sure our tests are never flaky and check that in PyScript we don't also cause issues with greedy GC operations

This is probably just a placeholder to allow MR to update various libraries but I think it has top priority, although it is not nearly as easy as it sounds to be fixed ... let's hope I manage in a way that is robust and surprises-less.

What browsers are you seeing the problem on? (if applicable)

No response

Console info

No response

Additional Context

No response

@WebReflection WebReflection added type: bug Something isn't working needs-triage Issue needs triage labels Sep 26, 2024
@WebReflection
Copy link
Contributor Author

This is what I could reason about and see as issue:

Screenshot From 2024-09-26 18-00-33

@WebReflection
Copy link
Contributor Author

The current investigation is around coincident module to provide it a way to either reference count per thread, an @ntoll suggestion, or simply never collect globally reachable namespaces or classes.

@WebReflection
Copy link
Contributor Author

This works already as solution to our tests issue but I am not 100% sure it's the best we can do ... it's surely the easiest thing to do and, as Proxies are fairly light anyway, maybe worth merging it regardless the possible better approach: WebReflection/coincident#49

@WebReflection
Copy link
Contributor Author

BTW, the issue is basically mostly only cause by ourselves in our tests because having both MicroPython and Pyodide in a worker is not really what we should recommend or suggest.

It is, however, true that the terminal case or the PyEditor makes this use case trivial to create but it's also true that these would rarely execute simultaneously unless forced to do so ... I am pretty confident that change in coincident would already mitigate the concurrent GC hazard and make things better for the time being but I am also convinced I need to fundamentally change some logic in there to allow some reference counting based architecture for multiple workers randomly doing similar things that results into similar references on the main ... this might take some time though, and unfortunately is not even that simple to test (but I'll find a way 'cause I want to make coincident rock solid for all scenarios).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage Issue needs triage type: bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant
0