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

Register subscription doc under doc_id key rather than field_key #1268

Merged
merged 3 commits into from
Aug 16, 2023

Conversation

martosaur
Copy link
Contributor

We've been running some performance tests against Absinthe subscriptions and discovered a curious scenario: if large number of clients were disconnecting at the same time, the app would become unresponsive and remain in this state until ETS memory consumption caused by the test did not drop to normal levels. And that was taking a lot of time.

After further investigation we were able to narrow down the following scenario:

  1. Have a lot of clients subscribed to the same topic. (In our case, 1000 clients x 5 subscriptions each = 5000 subscriptions to the same topic)
  2. Put some noticeable data into subscription context. (In our case it was two fairly big ecto structs)
  3. Make sure to set compress_registry? settings to true.
  4. Finally, make all those clients disconnect at the same time.

What happens next is that Registry begins to cleanup data associated with Absinthe channel processes using ets match_delete call. And since the table is compressed, it has to uncompress values to run match against them, which turned out to be a slow enough operation to occupy all cpu cores and starve other processes. Registry is smart enough to scope this operation by key, but the problem is that Absinthe stores subscription data under field_key which is shared between subscriptions, so the scoping doesn't help.

The solution I came up in this PR is to split data which Absinthe puts in Registry from {field_key, {doc_id, doc}} into 2 records: {field_key, doc_id} and {doc_id, doc}. This way total amount of data stored under shared field_key should be much smaller and thus quicker to traverse.

Copy link
Contributor

@benwilson512 benwilson512 left a comment

Choose a reason for hiding this comment

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

Thanks so much for the PR! Let me do a bit of poking around on my end but this seems like a solid approach. In your testing, does this fix the issue introduced by enabling compression?

@martosaur
Copy link
Contributor Author

Thanks! Yep, in our test it does solve the problem.

@benwilson512
Copy link
Contributor

OK the main hold up at the moment is that I need to benchmark the impact of doubling the count of keys in the ets table. It's possible we want to make this key structure a recommended option for when people use compression, and not something that people should use when not using compression.

@martosaur
Copy link
Contributor Author

If I read it correctly, #1261 sliced number of keys in half, so doubling it should get us back to the volume Absinthe's latest released version has? Another idea is to maybe create second registry with unique keys setting?

Anyways, thanks for looking into it and let me know if I can help!

@benwilson512
Copy link
Contributor

@martosaur great point about it returning things to the status quo. I think the overall point still stands though. My understanding is that the benefit of this 2 key structure is that it solves the compression issue. If compression is not used, 1/2 the number of keys seems good at least on the face of it.

Need to test!

@benwilson512
Copy link
Contributor

Tangentially if you want to try out #1270 and absinthe-graphql/absinthe_phoenix#97 it uses an even more compact way of storing the initial phases.

Copy link
Contributor

@benwilson512 benwilson512 left a comment

Choose a reason for hiding this comment

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

In discussions with @josevalim we realized that this allows other optimizations so we're going to use the 2 key thing in all cases.

@benwilson512 benwilson512 merged commit 8a5c293 into absinthe-graphql:master Aug 16, 2023
7 checks passed
@martosaur martosaur deleted the am/split_doc_registry branch August 16, 2023 16:06
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.

3 participants