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

[core] Limit typescript:ci step memory limit #8796

Merged
merged 15 commits into from
May 4, 2023

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Apr 28, 2023

A continuation of #6850. This is an effort to try and reduce the flakiness of typescript:ci run, where it sometimes errors out with 137 error code signifying an out-of-memory issue.
It might not help in the long term.

Reducing concurrency to the number of CPU cores (2) seems to help the most with memory usage as it avoids the extra work that the OS scheduler needs to do in order to run 4 concurrent processes, but it does decrease the pipeline runtime. A middle ground between the two is 3 concurrent processes, that both produce a decent memory usage graphic as well as basically the same pipeline runtime as with concurrency 4.

  • Upgrading node might help a bit with the runtime performance
  • Or we might end up increasing the container to medium+ with 3 CPU cores and 6GB of RAM.
    The underlying problem is that the docker runs out of memory because tsc consumes quite a lot of resources and garbage collection does kick in.

But for now, we can try sticking with a cheaper container and seeing if the reduced node process heap size and even further reduced concurrency work for us. 🙏

Set the maximum allowed node process memory limit to 3GB and concurrency to 3 instead of 4.

P.S. Simply limiting the allowed memory for a node process is not the final remedy, because we also have other OS processes and especially GC consuming quite a lot of memory.

@LukasTy LukasTy added the core Infrastructure work going on behind the scenes label Apr 28, 2023
@mui-bot
Copy link

mui-bot commented Apr 28, 2023

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-8796--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 642.6 1,060.4 642.6 828.78 190.147
Sort 100k rows ms 603.8 1,297 1,297 974.26 223.933
Select 100k rows ms 234.2 500 275.1 310.74 98.708
Deselect 100k rows ms 150.1 313.8 274.3 251.32 63.576

Generated by 🚫 dangerJS against 91e6fa9

@LukasTy LukasTy changed the title Set maximum allowed node process memory limit [core] Limit typescript:ci step memory limit May 1, 2023
@LukasTy LukasTy self-assigned this May 1, 2023
Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Thank you!

@alexfauquette
Copy link
Member

Would it somewhat make sense in the future to run git diff to know which projects are updated by the PR and run TypeScript only on the modified packages?

@LukasTy
Copy link
Member Author

LukasTy commented May 2, 2023

Would it somewhat make sense in the future to run git diff to know which projects are updated by the PR and run TypeScript only on the modified packages?

Hm. An interesting idea. I think that it would definitely make sense to optimize this case, but I don't think that diff would be enough, because of the relationship between packages. The complexity of how to identify when a certain package needs it might not be doable with simple static git diff.
That is the reason why we have TurboRepo or NX, where a relationship between packages is analyzed, and build/scripts can be run only on relevant changes, given that previous builds are being cached.

@oliviertassinari oliviertassinari added the scope: code-infra Specific to the core-infra product label May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants