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

Memory Profile for Ruby #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Memory Profile for Ruby #8

wants to merge 5 commits into from

Conversation

hmistry
Copy link

@hmistry hmistry commented Oct 13, 2017

Added some memory profile usage for Ruby ones - the results are in prof_results.txt. I'm not sure if this is the right way or not especially with external libraries involved - open to feedback for doing it the right way.

@hmistry hmistry mentioned this pull request Oct 13, 2017
@janko
Copy link
Collaborator

janko commented Oct 13, 2017

@hmistry Thank you for the PR, though there are a lot of different changes here.

Firstly, I think we shouldn't create separate Ruby files for each memory profiling method, I think that profilers should be activated via environment variables. So when we run MEMORY_PROFILE=1 ruby ruby_io_speed.rb, the memory profiler is activated.

Secondly, I think there is no point using MemoryProfiler, as we cannot compare its results to Node (since we cannot use it there), and because as you said, I'm pretty sure it doesn't work for gems that have C bindings. In that case we can make the other memory profiler you wrote that watches RSS run by default, without any environment variables.

Thirdly, I think bringing ImageProcessing in is a bit of an overkill, because we can already do the same thing with MiniMagick. Also, MiniMagick likely won't provide a meaningful comparison, as it shells out, which is slower than calling C functions, which is what ruby-vips and sharp do. Ultimately, we don't want to expand the scope of this repo to benchmark multiple processing libraries, we just wanted to pick one to compare the languages.

Is there some generic tool for monitoring memory usage? I mean, I think the Ruby implementation of monitoring RSS is great, I was just wondering if we could leverage some existing tool so that it's more language-agnostic.

@hmistry
Copy link
Author

hmistry commented Oct 13, 2017

@janko-m All valid points... here some context.

I needed to compare for my use case between node vips, ruby vips, and ruby minimagick to better understand the performance and this project was a good starting point. I'm sharing the results back incase others are interested. (I'm ok if you choose to not to merge/close it. 😃) If there's any PR to merge, it is #6 because the node implementation is not preserving the aspect ratio.

Commenting to your points:

  1. Good point. I wasn't thinking of best code, only what's the quickest way to get an answer.
  2. The RSS method was suspect as it gave me KB's memory consumption. MemoryProfiler seemed like a good way from quick research and the results are more believable. I agree for node memory profiling but I'm not too familiar with node.
  3. ImageProcessing - agree with your premise but I thought it wouldn't hurt and I'd like to know if it was adding significant overhead over MiniMagick since I'm using it. To my surprise the result for total memory allocated for ImageProcessing was a little over half of MiniMagick (12,707,842 bytes vs 22,184,049 bytes) and no impact to time! 😃
  4. Generic memory tool - I agree. I don't know and had limited time researching this. Should I discover one in future or someone else knows of one, we can revisit this.

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.

2 participants