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

Mojo::IOLoop::Stream memory hog #1256

Closed
leonerd opened this Issue Sep 7, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@leonerd

leonerd commented Sep 7, 2018

  • Mojolicious version: 7.29
  • Perl version: 5.14.2 threaded
  • Operating system: Debian Linux x86

Steps to reproduce the behavior

The {buffer} field of a Mojo::IOLoop::Stream stores pending bytes for output. Once written, they are substr()ed out of the buffer. This means that the memory usage of the actual buffer SV is never actually returned to the malloc pool.

This has the problem that if e.g. a temporary network stall happens, large buffers are created that use up lots of memory, which perl never gets around to reusing even after the buffers are flushed.

Expected behavior

Once the underlying _write method has successfully flushed the contents of the buffer, it should be undefed to return the actual byte buffer back to the malloc pool, so it can be reused.

Actual behavior

Because currently it doesn't get undefed, any allocated byte storage remains, consuming memory that the rest of the process can't use.

@leonerd

This comment has been minimized.

leonerd commented Sep 7, 2018

It is important to actually undef the buffer - merely assigning "" into it won't suffice:

eval: my $buffer = "A"x1E6; $buffer = ""; Dump $buffer
SV = PV(0x564e1a222bf0) at 0x564e1a24eef8
  REFCNT = 2
  FLAGS = (POK,pPOK)
  PV = 0x7fb180c50010 ""\0
  CUR = 0
  LEN = 1000002

Here, the buffer itself is still using a million bytes of RAM (the LEN field)

eval: my $buffer = "A"x1E6; undef $buffer; $buffer = ""; Dump $buffer
SV = PV(0x564e1a222bf0) at 0x564e1a24eef8
  REFCNT = 2
  FLAGS = (POK,IsCOW,pPOK)
  PV = 0x564e1a25ebb0 ""\0
  CUR = 0
  LEN = 10
  COW_REFCNT = 1

The old buffer has been returned to the malloc pool, and a new one created at only 10 bytes long.

jberger added a commit that referenced this issue Sep 7, 2018

@jberger

This comment has been minimized.

Member

jberger commented Sep 7, 2018

So we've added this change to hopefully address the problem. We haven't come to a good way to test it in the test suite, so for a first check @leonerd can you see if this at least is effective?

@leonerd

This comment has been minimized.

leonerd commented Sep 10, 2018

Yeah, testing this kind of thing is always rather tricky. But the code looks to be doing the right thing on a visual inspection at least.

I can deploy it and see how it behaves, though due to the rare nature of the problem in practice it may be a while before it turns up again.

@jberger

This comment has been minimized.

Member

jberger commented Sep 10, 2018

In that case I think I'll close this for now and please feel free to either reopen or open a new bug and reference this one if you should notice it in the future.

Thanks again for the report.

@jberger jberger closed this Sep 10, 2018

preaction added a commit to preaction/mojo that referenced this issue Oct 22, 2018

fix uninitialized warnings in stream
After `undef $self->{buffer}`, we can no longer call `length` on it in
Perl 5.10 (later Perls more usefully treat `undef` as empty without
warning). But, according to mojolicious#1256, we can't just set `$self->{buffer}`
to the empty string without causing Perl to hold on to all the memory it
allocated for our buffer. So, we have to first `undef`, then set to
a defined value to prevent warnings from `length` on Perl 5.10.

preaction added a commit to preaction/mojo that referenced this issue Oct 22, 2018

fix uninitialized warnings in stream
After `undef $self->{buffer}`, we can no longer call `length` on it in
Perl 5.10 (later Perls more usefully treat `undef` as empty without
warning). But, according to mojolicious#1256, we can't just set `$self->{buffer}`
to the empty string without causing Perl to hold on to all the memory it
allocated for our buffer. So, we have to first `undef`, then set to
a defined value to prevent warnings from `length` on Perl 5.10.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment