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

test(gatsby): Add a memory test suite command to the memory benchmark #34810

Merged
merged 17 commits into from
Feb 16, 2022

Conversation

imjoshin
Copy link
Contributor

@imjoshin imjoshin commented Feb 14, 2022

Description

This PR adds a test suite capability to the benchmarking tool we created previously.

This allows us to measure the impact of changes we make to our build process. Notably, it gives us data showing that our efforts in recent memory improvements have a positive impact. In the future, we'll be able to use this to test other improvements.

This CSVs output from these test suites allow us to create graphs like this:
Capture

In the future, we may be able to use this to add smoke tests to our CI. 👀

For more information, check out the README.

Examples:

$ yarn test --memory 16g --num-nodes 1000 --node-size 1m

yarn run v1.21.0
$ node scripts/test.js --memory 16g --num-nodes 1000 --node-size 1m
$ ./scripts/docker-get-id

Starting container with 16g memory.
$ DOCKER_MEMORY_LIMIT=16g ./scripts/docker-start
$ ./scripts/docker-get-id

Starting test using container dcdb6681da50.
 - clearing cache
$ docker exec  dcdb6681da50 rm -rf .cache
 - running build with 1000 nodes of size 1m
$ docker exec -e BUILD_NUM_NODES=1000 -e BUILD_STRING_NODE_SIZE=1m -e NUM_KEYS_IN_LARGE_SIZE_OBJ=1 dcdb6681da50 yarn gatsby buildFinished test in 43.81s

Success!

Stopping container dcdb6681da50
$ ./scripts/docker-stop
Done in 49.53s.
$  yarn test-suite
...
Running test #14 for 4g mem, 1000 nodes of 1m
$ yarn test --memory 4g --num-nodes 1000 --node-size 1m
Failed with exit code 134 after 15.63s.

Running test #15 for 4g mem, 1000 nodes of 1m
$ yarn test --memory 4g --num-nodes 1000 --node-size 1m
Failed with exit code 134 after 16.05s.

All tests failed for 4g memory, 1000 nodes.

Running test #0 for 8g mem, 100 nodes of 1m
$ yarn test --memory 8g --num-nodes 100 --node-size 1m
Built after 18.55s!

Running test #1 for 8g mem, 100 nodes of 1m
$ yarn test --memory 8g --num-nodes 100 --node-size 1m
Built after 18.51s!
...

Documentation

Updated the README!

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Feb 14, 2022
@imjoshin imjoshin marked this pull request as ready for review February 15, 2022 19:59
@imjoshin imjoshin changed the title Add a memory test suite command to the memory benchmark tests(gatsby): Add a memory test suite command to the memory benchmark Feb 15, 2022
@imjoshin imjoshin changed the title tests(gatsby): Add a memory test suite command to the memory benchmark test(gatsby): Add a memory test suite command to the memory benchmark Feb 15, 2022
@imjoshin imjoshin added topic: performance Related to runtime & build performance and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Feb 15, 2022
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

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

Awesome!

RUN if [ "$jemalloc" = "1" ]; then \
echo "Using jemalloc for memory allocation" && \
apt-get update && apt-get install -y libjemalloc-dev=5.1.0-3 && \
echo "/usr/lib/x86_64-linux-gnu/libjemalloc.so" >> /etc/ld.so.preload && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking, just future opportunity:

Maybe we can just install jemalloc always in image and then have --use-jemalloc as toggle for cli (we could set LD_PRELOAD=/usr/lib/x86_64-linux-gnu/libjemalloc.so env var in test runner script to enable it), this could limit how often we have to (re)build image when testing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I think if we revisit this, that's a good improvement.

Comment on lines +53 to +55
// there's something buggy with the node/exec/docker-exec integration
// we're getting seg faults, so this loop is just a patch for that
// so we don't have to fix it right now
Copy link
Contributor

Choose a reason for hiding this comment

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

How often does that happen? We shouldn't be getting flakes like that :S

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe 1/5 times? Fairly often... I tried to debug it for ~30 minutes and couldn't find it. It has nothing to do with our build, but the docker exec. Still very confused by it.

I've accounted for this in our timings, so it only pulls the timing for the latest build. It'll slow the test run down a tiny bit, but doesn't affect much else. Just annoying. 😦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: performance Related to runtime & build performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants