perf: reduce memory usage upon running server tests #3949

apoorv-mishra · 2022-08-09T18:30:27Z

This attempts to plug the memory leaks that occur upon running yarn test:server. I almost completely relied upon the --detect-leaks flag to spot and fix them. This flag gives room for the GC to sweep by awaiting for a few ticks in between.

Following are the results collected after making changes across the board:

node@16.16

$ node -v
v16.16.0

$ npx jest-heap-graph "yarn test:server --logHeapUsage --detect-leaks"

Test Suites: 120 passed, 120 total
Tests:       890 passed, 890 total
Snapshots:   86 passed, 86 total
Time:        335.578 s
Ran all test suites.
Force exiting Jest: Have you considered using `--detectOpenHandles` to detect async operations that kept running after all tests finished?

--- n: 120 ---
    1719.00 ┤                                                    ╭───────╮
    1568.60 ┤                                               ╭────╯       │
    1418.20 ┤                                          ╭────╯            │
    1267.80 ┤                                     ╭────╯                 │
    1117.40 ┤                                 ╭───╯                      │                       ╭────
     967.00 ┤                             ╭───╯                          │                  ╭────╯
     816.60 ┤                         ╭───╯                              │             ╭────╯
     666.20 ┤                     ╭───╯                                  │         ╭───╯
     515.80 ┤                 ╭───╯                                      │    ╭────╯
     365.40 ┼───╮╭─╮╭─────────╯                                          ╰────╯
     215.00 ┤   ╰╯ ╰╯

node@16.10

$ node -v
v16.10.0

$ npx jest-heap-graph "yarn test:server --logHeapUsage --detect-leaks"

Test Suites: 120 passed, 120 total
Tests:       890 passed, 890 total
Snapshots:   86 passed, 86 total
Time:        291.507 s
Ran all test suites.
Force exiting Jest: Have you considered using `--detectOpenHandles` to detect async operations that kept running after all tests finished?

--- n: 120 ---
     235.00 ┤                                      ╭╮
     228.40 ┤                                      ││╭╮                    ╭╮  ╭─╮
     221.80 ┤                                      ││││                  ╭─╯│╭─╯ │  ╭─╮
     215.20 ┤                        ╭╮            ││││             ╭╮╭──╯  ╰╯   ╰╮ │ │           ╭──╮
     208.60 ┤           ╭─╮          ││╭╮          │╰╯╰╮           ╭╯││           │ │ │╭─╮  ╭╮ ╭╮╭╯  │
     202.00 ┤           │ │╭╮        ││││   ╭╮   ╭─╯   │           │ ││           ╰╮│ ╰╯ ╰──╯╰─╯╰╯   │
     195.40 ┤           │ ╰╯╰╮ ╭╮╭─╮ ││││   │╰───╯     │    ╭╮     │ ││            ││                │
     188.80 ┤        ╭╮ │    │╭╯││ ╰╮│││╰─╮ │          ╰╮  ╭╯╰─────╯ ╰╯            ╰╯                ╰
     182.20 ┼────────╯╰─╯    ││ ╰╯  ││╰╯  │╭╯           ╰──╯
     175.60 ┤                ││     ╰╯    ││
     169.00 ┤                ╰╯           ╰╯

node@14.19

$ node -v
v14.19.3

$ npx jest-heap-graph "yarn test:server --logHeapUsage --detect-leaks"

Test Suites: 120 passed, 120 total
Tests:       890 passed, 890 total
Snapshots:   86 passed, 86 total
Time:        262.933 s
Ran all test suites.
Force exiting Jest: Have you considered using `--detectOpenHandles` to detect async operations that kept running after all tests finished?

--- n: 120 ---
     228.00 ┤                                                                                        ╭
     222.30 ┤                                                                        ╭╮   ╭──────────╯
     216.60 ┤                                                                ╭╮ ╭────╯╰───╯
     210.90 ┤                      ╭─╮╭╮╭╮╭╮ ╭─╮                     ╭╮ ╭────╯╰─╯
     205.20 ┤       ╭╮      ╭──────╯ ╰╯╰╯╰╯╰─╯ │             ╭╮╭─────╯╰─╯
     199.50 ┤       ││      │                  │╭─╮╭─────────╯╰╯
     193.80 ┤       ││      │                  ╰╯ ╰╯
     188.10 ┤      ╭╯│      │
     182.40 ┤╭╮╭╮╭╮│ ╰╮ ╭─╮ │
     176.70 ┼╯╰╯╰╯╰╯  ╰─╯ ╰╮│
     171.00 ┤              ╰╯

apoorv-mishra · 2022-08-09T18:32:21Z

server/routes/api/documents.test.ts


 const server = getTestServer();
+afterAll(() => disconnectdb().then(() => server.close()));


This plugged up the leaks detected using --detect-leaks flag as shown here.

I recently (as part of this issue) changed this to use a single centralized test server – the thinking was that it would be fast than spinning up an instance/dbconnection in each file. I'm actually pretty surprised this works though and the rest of the suite doesn't fail after the server is closed? 🤔

I recently (as part of this issue) changed this to use a single centralized test server – the thinking was that it would be fast than spinning up an instance/dbconnection in each file.

Looks like this is unreachable. Not sure why, likely due to how jest handles running multiple test files 🤔

Oh, I'll admit I didn't explicitly check that. Must be a limitation of jest module scope, in that case how about we change getTestServer to the following:

export function getTestServer() { const app = webService(); const testServer = new TestServer(app.callback()); testServer.cleanup = async () => { await sequelize.close(); await testServer.close(); }; return testServer; }

Then we just need to add the following in individual test files

afterAll(server.cleanup);

You're right though, this cleanup makes a huge difference 🙏

The more we can centralize setup and teardown the less future similar mistakes, I'm okay centralizing – particularly as moving into the suites would also require importing sequelize

How about 👇

export function getTestServer() { const app = webService(); const server = new TestServer(app.callback()); server.disconnect = () => { await sequelize.close(); await server.close(); }; return server; } export function getTestdb() { const db = sequelize; db.flush = () => { /* flush! */ }; db.seed = () => { /* seed! */ }; db.disconnect () => { /* disconnect! */ }; };

And,

// Usage(in server/routes/api/) import { getTestServer, getTestdb } from "@server/test/utils" const db = getTestdb(); const server = getTestServer(); afterAll(server.disconnect); beforeEach(db.flush); beforeEach(db.seed); ////////////////////////////////////////////////////////////////////////// // Usage(in server/{commands, models...}) import { getTestdb } from "@server/test/utils"; const db = getTestdb(); afterAll(db.disconnect); beforeEach(db.flush); beforeEach(db.seed);

Okay, seems clean and sensible 👍 . But getTestdb -> getTestDatabase

And don't include seed, I'm trying to gradually get rid of it in favor of individual factories.

And don't include seed, I'm trying to gradually get rid of it in favor of individual factories.

So, we keep seed as it is in current scope? Also, are you exploring an equivalent of factory_bot? :)

Okay – include seed if you're prepared to make the changes 😆

apoorv-mishra · 2022-08-09T18:35:08Z

server/test/setup.ts

@@ -18,6 +18,7 @@ if (process.env.DATABASE_URL_TEST) {
 require("@server/database/sequelize");

 jest.mock("bull");
+jest.mock("i18next-http-backend");


This fixed the leak for shared/i18n/index.test.ts.

apoorv-mishra · 2022-08-09T18:40:30Z

shared/editor/embeds/Abstract.test.ts

@@ -1,8 +1,8 @@
-import Abstract from "./Abstract";
+import { URL_REGEX } from "./Abstract";


The embeds were problematic in node@16.16.0. It occurred that maybe this would fix it since we don't really require the entire component for tests. I tried running it multiple times and the leaks came to halt, although I didn't spot considerable difference in heap usage.

On the other hand, in node@16.10.0 and node@14.19.3, these seemed to behave sanely with --detect-leaks flag.

Oof, this is a painful change to make across all embeds though – and now there is an implicit requirement of both exports just for test compatibility.

I think that the /shared/ tests should just be split out into their own test:shared command that runs separately and if they're a touch leaky it at least won't affect the other server tests

I'm going to split these out now

Shouldn't have any conflicts, but heads up: #3951

Lets remove these changes in embeds, otherwise you killed it – memory behavior is great now!

Ahh..almost forgot! Thanks for pointing out. Reverted.

apoorv-mishra

These are just the sample changes for quick feedback. Once aligned, these changes would be propagated across all the tests.

apoorv-mishra · 2022-08-10T17:15:09Z

Ran into an issue after purging and re-pulling the postgres docker image 👇

● #documents.info › should return published document

    SequelizeConnectionError: database "outline-test" does not exist

      at Client._connectionCallback (node_modules/sequelize/src/dialects/postgres/connection-manager.js:189:24)
      at Client._handleErrorWhileConnecting (node_modules/pg/lib/client.js:305:19)
      at Client._handleErrorMessage (node_modules/pg/lib/client.js:325:19)
      at node_modules/pg/lib/connection.js:115:12
      at Parser.parse (node_modules/pg-protocol/src/parser.ts:102:9)
      at Socket.<anonymous> (node_modules/pg-protocol/src/index.ts:7:48)


  ● Test suite failed to run

    Server is not running.

Did you ever run into this issue @tommoor ?

tommoor · 2022-08-10T17:16:26Z

You can run make test to recreate the test environment including db

apoorv-mishra · 2022-08-10T17:18:25Z

Shoot! How did I forget this 😆

server/test/support.ts

apoorv-mishra

Fixed for tests under server/routes. I'll pause here for your review @tommoor so far 😆

tommoor · 2022-08-10T19:22:21Z

No red flags!

.jestconfig.json

server/test/setup.ts

apoorv-mishra commented Aug 9, 2022

View reviewed changes

apoorv-mishra force-pushed the fix/3939/server-tests-perf branch from dd590ff to 48f4ee9 Compare August 10, 2022 19:10

apoorv-mishra commented Aug 10, 2022

View reviewed changes

server/test/support.ts Outdated Show resolved Hide resolved

apoorv-mishra commented Aug 10, 2022

View reviewed changes

apoorv-mishra added 12 commits August 11, 2022 11:07

perf: reduce memory usage upon running server tests

3b11d96

perf: plug leaks in server/routes

090e554

perf: plug leaks in server/scripts

9dbe9a0

perf: plug leaks in server/policies

9f5f87a

perf: plug leaks in server/models

90ab2e1

perf: plug leaks in server/middlewares

309c279

perf: plug leaks in server/commands

c48b9f5

fix: missing await on db.flush

c9f37e1

perf: plug leaks in server/queues

76873c6

chore: remove unused legacy funcs

8933e3b

fix: await on db.flush

94ba691

perf: await on GC to run in between tests

7430110

apoorv-mishra force-pushed the fix/3939/server-tests-perf branch from 8b4c8b1 to 7430110 Compare August 11, 2022 06:08

apoorv-mishra commented Aug 11, 2022

View reviewed changes

.jestconfig.json Show resolved Hide resolved

apoorv-mishra marked this pull request as ready for review August 11, 2022 06:12

auto-assign bot requested a review from tommoor August 11, 2022 06:12

apoorv-mishra added 3 commits August 11, 2022 11:50

fix: remove db refs

23d9ce3

fix: revert embeds

3f25f7f

perf: plug leaks in shared/i18n

d3b0cb9

apoorv-mishra commented Aug 11, 2022

View reviewed changes

server/test/setup.ts Show resolved Hide resolved

tommoor approved these changes Aug 11, 2022

View reviewed changes

apoorv-mishra merged commit 0c51bfb into outline:main Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: reduce memory usage upon running server tests #3949

perf: reduce memory usage upon running server tests #3949

apoorv-mishra commented Aug 9, 2022 •

edited

apoorv-mishra Aug 9, 2022

tommoor Aug 9, 2022

apoorv-mishra Aug 10, 2022 •

edited

tommoor Aug 10, 2022

tommoor Aug 10, 2022

tommoor Aug 10, 2022

apoorv-mishra Aug 10, 2022 •

edited

tommoor Aug 10, 2022 •

edited

apoorv-mishra Aug 10, 2022 •

edited

tommoor Aug 10, 2022

apoorv-mishra Aug 9, 2022

apoorv-mishra Aug 9, 2022

tommoor Aug 9, 2022

tommoor Aug 10, 2022

tommoor Aug 10, 2022

tommoor Aug 11, 2022

apoorv-mishra Aug 11, 2022

apoorv-mishra left a comment

apoorv-mishra commented Aug 10, 2022

tommoor commented Aug 10, 2022

apoorv-mishra commented Aug 10, 2022

apoorv-mishra left a comment •

edited

tommoor commented Aug 10, 2022


		const server = getTestServer();
		afterAll(() => disconnectdb().then(() => server.close()));

		@@ -1,8 +1,8 @@
		import Abstract from "./Abstract";
		import { URL_REGEX } from "./Abstract";

perf: reduce memory usage upon running server tests #3949

perf: reduce memory usage upon running server tests #3949

Conversation

apoorv-mishra commented Aug 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoorv-mishra Aug 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoorv-mishra Aug 10, 2022 • edited

Choose a reason for hiding this comment

tommoor Aug 10, 2022 • edited

Choose a reason for hiding this comment

apoorv-mishra Aug 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apoorv-mishra left a comment

Choose a reason for hiding this comment

apoorv-mishra commented Aug 10, 2022

tommoor commented Aug 10, 2022

apoorv-mishra commented Aug 10, 2022

apoorv-mishra left a comment • edited

Choose a reason for hiding this comment

tommoor commented Aug 10, 2022

apoorv-mishra commented Aug 9, 2022 •

edited

apoorv-mishra Aug 10, 2022 •

edited

apoorv-mishra Aug 10, 2022 •

edited

tommoor Aug 10, 2022 •

edited

apoorv-mishra Aug 10, 2022 •

edited

apoorv-mishra left a comment •

edited