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

Removing MMTK_MIN_HSIZE_G and MMTK_MAX_HSIZE_G from CI #224

Closed
wants to merge 5 commits into from

Conversation

udesou
Copy link
Contributor

@udesou udesou commented Jan 23, 2025

This PR is the same as #222 but it removes MMTK_MIN_HSIZE_G and MMTK_MAX_HSIZE_G from our CI configuration, using the default values from the Stock GC.
It works as a check to see if we still need to specify those values with the stock GC heuristics because of the specs from the github runner.

@udesou
Copy link
Contributor Author

udesou commented Jan 23, 2025

julia-version
JULIA_REPO=udesou/Julia
JULIA_REF=9f8df7b5d1c6760c0488525dea0a7efa0f609f53

@udesou udesou changed the title Removing MMTK_MIN_HSIZE_G and MMTK_MAX_HSIZE_G Removing MMTK_MIN_HSIZE_G and MMTK_MAX_HSIZE_G from CI Jan 23, 2025
@qinsoon
Copy link
Member

qinsoon commented Jan 23, 2025

I don't know if we want to do this change. There seems no benifit of removing the specified heap size.

@udesou
Copy link
Contributor Author

udesou commented Jan 23, 2025

I don't know if we want to do this change. There seems no benifit of removing the specified heap size.

I was just curious to see if the CI will run without it.

At the same time, the semantics for the heap size environment variables is now a bit unclear. Do we want to use them with membalancer or use the stock heuristics? Should we support both? Or should we keep everything as it is and only use those for membalancer, using --heap-size-hint for the other case? (what if we have both?)

@qinsoon
Copy link
Member

qinsoon commented Jan 23, 2025

I don't know if we want to do this change. There seems no benifit of removing the specified heap size.

I was just curious to see if the CI will run without it.

This is the exact reason that I think we should not remove the heap size -- we no longer know if we can run the tests. It would highly depend on the machine we run it. Also if a test fail in CI, it would be harder to reproduce it locally, as the heap size would be different.

At the same time, the semantics for the heap size environment variables is now a bit unclear. Do we want to use them with membalancer or use the stock heuristics? Should we support both? Or should we keep everything as it is and only use those for membalancer, using --heap-size-hint for the other case? (what if we have both?)

We can get this right. If Julia allows us to specify heap size, we should be compatible with whatever Julia provides. If Julia does not allow it, we need our own way to do that.

@udesou
Copy link
Contributor Author

udesou commented Jan 23, 2025

Closed but opened an issue to keep track and implement a proper solution in the future.

@udesou udesou closed this Jan 23, 2025
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