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] Fix test-types out of memory error #6850

Merged
merged 5 commits into from
Nov 15, 2022

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Nov 15, 2022

Try tackling the same yarn error code 137 appearing when typescript:ci is being run.
This error is thrown when the system runs out of physical memory (RAM).

Done some testing using a few different configurations:

  • medium+ is a one step bigger container: 6GB RAM & 3-core CPU instead of 4GB RAM & 2-core CPU.

Based on the above experiments it seems that firstly going with a simple approach of reducing the concurrency is perfectly viable. Upsizing the container does yield a bit better performance, but if it costs more to us—it does not seem like it is worth the trade-off. At least at the moment, because we are not able to consume >75% of RAM.
Besides, lower concurrency does not necessarily mean longer task runtime as seen by examples and local experiments.

In any case we have somewhat low amount of cores and spawning a lot of processes requires a lot of task switching to happen when executing tasks. In our case—each of the task runtime is quite small and can benefit from lower concurrency and less CPU scheduler priority switching needed.

Related to mui/material-ui#31332

@LukasTy LukasTy added the core Infrastructure work going on behind the scenes label Nov 15, 2022
@mui-bot
Copy link

mui-bot commented Nov 15, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-6850--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 696.4 1,177.3 726.1 865.46 186.46
Sort 100k rows ms 630 1,286.4 1,286.4 1,007.6 217.042
Select 100k rows ms 226 295.5 247.9 256.48 24.155
Deselect 100k rows ms 158.9 304.9 203.5 220.48 51.914

Generated by 🚫 dangerJS against c3aa818

@LukasTy LukasTy marked this pull request as ready for review November 15, 2022 09:06
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.

The test results are quite surprising!
Let's see if the issue returns

@flaviendelangle
Copy link
Member

I sometime have crashes on yarn typescript on my local machine with 16go of RAM
I would not be surprised if we had a deeper problem somewhere

But I'm fine with starting like this 👍

@LukasTy
Copy link
Member Author

LukasTy commented Nov 15, 2022

The test results are quite surprising!

Yup. Naturally there are run-to-run variances on it's own. Also—we provide --no-sort argument, which further increases the variance of time each run might take depending on the order of packages scheduled for execution.

Testing most accurately would require the --no-sort argument removal, but in any case, it seems that with our current package sizes—lower concurrency is perfectly fine and even going down to 4 yields basically same results.

@LukasTy
Copy link
Member Author

LukasTy commented Nov 15, 2022

I sometime have crashes on yarn typescript on my local machine with 16go of RAM

Do you have memory swap turned on?
I'm not sure, but I have a feeling that docker instances do not have memory-swap turned on and it naturally crashes the process when system runs out of available memory.

@flaviendelangle
Copy link
Member

Do you have memory swap turned on?

Yep

@LukasTy LukasTy merged commit 5d13888 into mui:next Nov 15, 2022
@LukasTy LukasTy deleted the fix-test-types-out-of-memory branch November 15, 2022 09:37
@oliviertassinari
Copy link
Member

Looks great 👍

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 test typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants