-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
Performance enhancements (memory) #12561
Conversation
cfb7ab6
to
d77e7ba
Compare
def start | ||
case parsed_options.options.subcommand | ||
when :version | ||
$stderr.puts "Framework Version: #{Metasploit::Framework::VERSION}" | ||
else | ||
spinner unless parsed_options.options.console.quiet | ||
start_profiler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if we should wrap this for rescue LoadError
and notify unable to start profiler encase someone has env variables set in an install that does not supply the gems required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what the purpose suggesting that we move the gems into developer tags was, if we now have to deal with the extra complexity of the gems probably not being there.
Surely it can't be space savings. This gem is 80K. The aws-* gems that are used by one module are almost 10MB. Is there some other reason to add this complexity, or could I just move it to be a normal gem so anyone can memory profile in the field as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it, I'd advocate for making this something that can just be used by anyone at anytime, so move the gem to the normal group (we can still load on demand) and probably the CPU profiling ones as well. That would allow someone who is just noticing Metasploit doing something funny in either case the ability to supply debug info in case we cannot reproduce it locally. Sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that deserves its own discussion in another PR. I would suggest undoing the code move and contemplating what user-facing profiler would look like and how useful it would be later.
I don't think there needs to be LoadError
rescues FWIW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All sounds reasonable, this was just suggestion based on scenarios possible. Also note that anything added in Gemfile
vs .gemspec
adds different scoping requirements as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I can move out the testing bits and just leave the other improvements
This avoids computing a stack trace on every single log message that is never used in any of the logging sinks. This is one of the number one profiled memory allocation/deallocation events in Metasploit as shown with memory_profiler.
…ditionally on Ruby 2.6), it uses a lot of memory
I'm going to pull in these changes now since there are no further comments. Thanks! |
Release NotesThis adds a number of low-hanging fruit reductions in profiled memory operations when starting Metasploit Framework. |
This is a PR to look at memory utilization enhancements across Metasploit Framework. This is the first attempt, which is focused on on low-hanging fruit provided by analysis from the
memory_profiler
gem.To reproduce these results, invoke msfconsole with the changes from #12652
Not all of these changes provide huge results alone, but enough in aggregate should provide lowered CPU and Memory resource utilization, long-term memory fragmentation.
Verification
msfconsole
in profiling mode as above without the other changes appliedmemory.profile
msfconsole
with changes applied.memory.profile
are goneThe first two changes here remove a few thousand string allocations on initial boot by removing an unused stacktrace calculation in all of the library logging functions, and by prefering str.casecmp over creating a new lowercase string in the datastore.