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

Fix memory leak in pdfjs.js. #42

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@ltratt

ltratt commented Oct 2, 2016

A large amount of data is pushed into the global variable canvas_logs which isn't cleared in runPdfJS. On each iteration the list grows, eventually significantly so.

On a Linux machine with a recent-ish V8, it manages 2777 iterations before an allocation fails (at which point it's allocated over 2GiB of virtual memory, and used about 1.4Gib) and V8 crashes (Fatal error in CALL_AND_RETRY_LAST). From iteration ~800 onwards, things start slowing down gradually; at iteration ~1700, they start slowing down considerably; and at iteration ~2750, performance falls off a cliff; presumably as the GC comes under more and more strain. Here's a graph showing this (x axis=iteration number; y axis=time):

img1

It can be a little hard to see what's going on because the last iteration or two are so slow and, in a sense, distort the rest of the graph. If I chop off the last 80 iterations, one can still see that odd things are happening:

img2

By emptying canvas_logs at the end of each benchmark run, we make the
benchmark's performance more consistent. With this patch I can happily run this
benchmark in constant memory for 10000 iterations, with no discernible change in
iteration time over that.

Fix memory leak in pdfjs.js.
A large amount of data is pushed into the global variable 'canvas_logs' which
isn't cleared in runPdfJS. On each iteration the list grows, eventually
significantly so.

On a Linux machine with a recent-ish V8, it manages 2777 iterations before an
allocation fails (at which point it's allocated over 2GiB of virtual memory, and
used about 1.4Gib) and V8 crashes ("Fatal error in CALL_AND_RETRY_LAST"). From
iteration ~800 onwards, things start slowing down gradually; at iteration ~1700,
they start slowing down considerably; and at iteration ~2750, performance falls
off a cliff; presumably as the GC comes under more and more strain.

By emptying canvas_logs at the end of each benchmark run, we make the
benchmark's performance more consistent. With this patch I can happily run this
benchmark in constant memory for 10000 iterations, with no discernible change in
iteration time over that.

@ltratt ltratt force-pushed the ltratt:master branch from 6999eec to 1eafbe9 Oct 2, 2016

@natorion

This comment has been minimized.

Member

natorion commented Oct 14, 2016

@woess

This comment has been minimized.

woess commented Jan 9, 2017

@ltratt I'm afraid emptying canvas_logs in runPdfJS skips the correctness checks in tearDownPdfJS.

@ltratt

This comment has been minimized.

ltratt commented Jan 9, 2017

@woess Hello Andreas, yes, I think you're right. That said, IMHO, the current placement of the correctness checks means that one is forced to choose between a) predictable performance on each iteration b) correctness checks. Both options suck, though I think for a benchmark the former sucks marginally less. It would be nice to do both though!

I've had a quick look, and the code is doing something that I don't understand. If I try and move the correctness check into runPdfJS then on the first iteration canvas_logs[0].length == 6, which fails the check, but on all subsequent iterations canvas_logs[0].length == 36788... I do not pretend to understand why. Any thoughts?

@woess

This comment has been minimized.

woess commented Feb 22, 2017

@ltratt Agreed. You could clear the canvas_logs array at the beginning of runPdfJS so that you'll at least have the last iteration checked (or limit the length of the array to a reasonable size). I guess the benchmark is just not designed to be run for so many iterations...

I don't see the canvas_logs[0].length == 6 behavior you described.

Move the canvas logs flushing to the beginning of unPdfJs.
This means that the checks in tearDownPdfJs are still executed.
@ltratt

This comment has been minimized.

ltratt commented May 22, 2017

OK, so I meant to look at this and then... forgot.

I've made the change that Andreas (@woess) suggested. I believe that it does the right thing: the benchmark no longer leaks memory; but the checks are still run.

I'd suggest I squash this before any possible merge, assuming people agree that this is the right fix.

@ltratt

This comment has been minimized.

ltratt commented Feb 4, 2018

I think it's officially time to "ping" this one. Or we can let it die -- but, if I'm honest, I'd prefer someone to state explicitly that it won't be merged.

@natorion

This comment has been minimized.

Member

natorion commented Feb 7, 2018

It won't be merged. Octane is retired and no longer maintained. Sorry for the long communication cycle.

@natorion natorion closed this Feb 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment