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

Document the process anti pattern of sending large data #13194

Merged

Conversation

PragTob
Copy link
Contributor

@PragTob PragTob commented Dec 17, 2023

As per usual, thanks to all of ya for your great work!

Follow up to/extension of #13173

First draft, happy to adjust and extend it in many ways. I had trouble coming up with a simple example as we needed a bunch of data to make sure it's not good. I could have employed the first anti pattern myself and did repeated statistics calculation, but that'd have been worse :sweat-smile:

I remembered José's comment around sending along the conn and figured it's central enough in elixir to not throw anyone off.

If someone has a better example, happy to redo it!

Thanks y'all! :green-heart:

Follow up to/extension of elixir-lang#13173

First draft, happy to adjust and extend it in many ways. I had
trouble coming up with a simple example as we needed a bunch of
data to make sure it's not good. I could have employed the first
anti pattern myself and did repeated statistics calculation, but
that'd have been worse :sweat-smile:

I remembered José's comment around sending along the `conn` and
figured it's central enough in elixir to not throw anyone off.

If someone has a better example, happy to redo it!

Thanks y'all! :green-heart:
@PragTob PragTob force-pushed the process-anti-patterns-send-lots-of-data branch from 5806d5a to 1e22915 Compare December 17, 2023 10:08
@josevalim
Copy link
Member

The original anti-patterns mentioned exactly this, sending data between processes, but it was removed because it is hard to precisely say when you are sending large data. Sometimes you do have to send it and there isn't much we can do.

Therefore, can you please rephrase this as accidentally capturing large data on spawn? I.e. without the sending considerations? Thank you.

@PragTob
Copy link
Contributor Author

PragTob commented Dec 17, 2023

@josevalim 👋 Thanks for the context, I was unaware of the previous version.

Rephrasing to validate my understanding:

  • Rewrite section to focus on spawn/Task.async aka where the function captures the large data (to be fully clear, should I write it with spawn or Task.async as I think the latter is more common)
  • Remove mention of this problem around send as it's ambiguous what large data is and when it's needed/vs. not needed

Happy to do that! Question though, shouldn't we at least mention send/2? While most people will learn about the copying behavior on their elixir/erlang journey I couldn't find it in the elixir docs (at least where I checked, Process, send/2 and the Process getting started mentions it's "is stored in the process mailbox" which could be more explicit, however a beginner guide is probably not the best space for this). I don't mean the mention of the "anti-pattern" but that messages are deep copied to the receiving process. Wouldn't want to litter everything, but maybe Process docs?

@josevalim
Copy link
Member

Hrm. It is fine to mention send or even GenServer.call, but the framing matters. The issue not really sending large data, but sending unnecessary data. Don't send or pass as argument to GenServer.call/GenServer.start_link a large data structure, only to pluck part of it from the server. Pluck the parts that you need on the caller/client.

@PragTob
Copy link
Contributor Author

PragTob commented Dec 17, 2023

Makes sense, thanks for the clarification! I'll get back to it later today!

@PragTob
Copy link
Contributor Author

PragTob commented Dec 17, 2023

Thanks for the feedback and guidance! New version is up, hope that that's the direction you envisioned. Happy to adjust and iterate in any case :) 💚

IMG_20170521_203155

Copy link
Contributor

@Nezteb Nezteb left a comment

Choose a reason for hiding this comment

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

Some minor capitalization/wording suggestions. Regardless, I love this content!

lib/elixir/pages/anti-patterns/process-anti-patterns.md Outdated Show resolved Hide resolved
lib/elixir/pages/anti-patterns/process-anti-patterns.md Outdated Show resolved Hide resolved
lib/elixir/pages/anti-patterns/process-anti-patterns.md Outdated Show resolved Hide resolved
lib/elixir/pages/anti-patterns/process-anti-patterns.md Outdated Show resolved Hide resolved
lib/elixir/pages/anti-patterns/process-anti-patterns.md Outdated Show resolved Hide resolved
lib/elixir/pages/anti-patterns/process-anti-patterns.md Outdated Show resolved Hide resolved
lib/elixir/pages/anti-patterns/process-anti-patterns.md Outdated Show resolved Hide resolved
Excellent suggestions and improvements on wording from @Netzteb!

Co-authored-by: Noah Betzen <noah@nezteb.net>
@PragTob
Copy link
Contributor Author

PragTob commented Dec 20, 2023

@Nezteb thanks for taking the time to review & make suggestions!

IMG_20221012_141750

And @whatyouhide also came in with a round of improvements, thank you!

Co-authored-by: Andrea Leopardi <an.leopardi@gmail.com>
@PragTob
Copy link
Contributor Author

PragTob commented Dec 21, 2023

And a big thank you to @whatyouhide !

IMG20230429163945

Copy link
Contributor

@v0idpwn v0idpwn left a comment

Choose a reason for hiding this comment

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

Looks great. I suggest also adding an example (either here or on GenServer docs) about doing this when sending messages. While it's way more obvious, it also happens :)

@josevalim josevalim merged commit 82be192 into elixir-lang:main Dec 21, 2023
1 check failed
@josevalim
Copy link
Member

💚 💙 💜 💛 ❤️

josevalim pushed a commit that referenced this pull request Dec 21, 2023
@PragTob
Copy link
Contributor Author

PragTob commented Dec 21, 2023

@josevalim thanks for the merge and as I don't think I'll open another issue/MR that soon: Happy holidays! 💚

IMG_20180603_163744

@PragTob PragTob deleted the process-anti-patterns-send-lots-of-data branch December 21, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants