These are chat archives for ManageIQ/manageiq/performance

18th
Jan 2018
Keenan Brock
@kbrock
Jan 18 2018 13:40
um. guys? Has anyone tried the reproducer with a regular string in the path? I'm pretty sure it is showing up as a leak for me. Maybe I'm doing something wrong (on mac, ruby 2.4.2)
require 'pathname'
require 'fileutils'

puts Process.pid

Dir.mkdir("foo") unless Dir.exists?("foo")
$LOAD_PATH.unshift("foo") # string

FileUtils.touch("empty.rb")
loop { 1500.times { require "empty" }; print "."; GC.start; }
the increase is not as consistent, but surprised to see any increase at all
Keenan Brock
@kbrock
Jan 18 2018 13:59
10,563,584
10,575,872
10,592,256
10,616,832 x2
10,620,928
10,653,696 x7
10,694,656 x2
10,706,944 x3
10,715,136 x6
10,731,520 x13
Jason Frey
@Fryguy
Jan 18 2018 14:23
running with the leak detector to see
Keenan Brock
@kbrock
Jan 18 2018 14:25
ok, think I'm able to get a constant state with:
require 'pathname'
require 'fileutils'

puts Process.pid

Dir.mkdir("foo") unless Dir.exists?("foo")
#$LOAD_PATH.unshift(Path.new "foo") # path
$LOAD_PATH.unshift("foo") # string

EMPTYI="empty".intern #error
EMPTYF="empty".freeze #intern
FileUtils.touch("empty.rb")
#loop { 1500.times { require "empty" }; print "."; GC.start; } # regular
#loop { 1500.times { require "em" << "pty" }; print "."; GC.start; } # different string every time
#loop { 1500.times { require "empty".intern }; print "."; GC.start; } # intern string
#loop { 1500.times { require EMPTYI }; print "."; GC.start; } # intern string - error
loop { 1500.times { require EMPTYF }; print "."; GC.start; } # frozen string
ugh - nope, still leaking
Jason Frey
@Fryguy
Jan 18 2018 14:25
I don't see it with the leak tool
Keenan Brock
@kbrock
Jan 18 2018 14:25
ok - cool
I was using nick's script (modified of course)
10,416,128
10,522,624 (𝛥106,496)
10,661,888 (𝛥139,264)
10,694,656 (𝛥32,768)
10,735,616 (𝛥40,960)
10,788,864 (𝛥53,248) x21
10,797,056 (𝛥8,192)
10,805,248 (𝛥8,192) x5
10,809,344 (𝛥4,096) x11
10,825,728 (𝛥16,384) x2
10,829,824 (𝛥4,096) x7
10,842,112 (𝛥12,288) x17
10,858,496 (𝛥16,384) x6
10,866,688 (𝛥8,192) x12
I'm looping for a while of course
and it does start to show stable as you can see there are 21 of the same values there, and 17, and ...
@Fryguy you are attaching the pid with the mac inspector?
Keenan Brock
@kbrock
Jan 18 2018 14:32
aah - I was running against the system ruby 2.0
I'll circle back with 2.3
Keenan Brock
@kbrock
Jan 18 2018 14:56
thanks - it was the wrong ruby version.
Joe Rafaniello
@jrafanie
Jan 18 2018 15:11
by the way, even if they fix the leak, this comment makes me believe Pathname in the load path are second class citizens and are probably slow
/* Construct expanded load path and store it to cache.
   We rebuild load path partially if the cache is invalid.
   We don't cache non string object and expand it every time. We ensure that
   string objects in $LOAD_PATH are frozen.
 */
Joe Rafaniello
@jrafanie
Jan 18 2018 15:19
I did a quick test on ruby 2.5.0. 1 Pathname in load path in a empty ruby file require loop script takes 16 seconds. If I do .to_s on the pathname, it's 1.7 seconds
Joe Rafaniello
@jrafanie
Jan 18 2018 15:25
diff --git a/18_test_ruby_require_leak.rb b/18_test_ruby_require_leak_with_string.rb
index 94f067b..94f2cd3 100644
--- a/18_test_ruby_require_leak.rb
+++ b/18_test_ruby_require_leak_with_string.rb
@@ -2,7 +2,7 @@ require 'pathname'

 puts Process.pid

-$LOAD_PATH.unshift(Pathname.new(__dir__))
+$LOAD_PATH.unshift(Pathname.new(__dir__).to_s)

 dot      = "."
 filename = "empty"
18_test_ruby_require_leak.rb             10.07s user 5.93s system 99% cpu 16.052 total
18_test_ruby_require_leak_with_string.rb  1.76s user 0.01s system 99% cpu 1.789 total
Joe Rafaniello
@jrafanie
Jan 18 2018 15:31
ruby 2.5.0:
Same script but with a different number of the exact same pathnames in the $LOAD_PATH:
1 pathname: 16 seconds
2 pathnames: 28 seconds
3 pathnames in load path: 40 seconds
12 pathnames in load path: 145 seconds
Keenan Brock
@kbrock
Jan 18 2018 15:33
@jrafanie but with a string - the numbers are similar?
Is gitter.app working for other people? only browser working for me
ok. think it is back
had to login again
Joe Rafaniello
@jrafanie
Jan 18 2018 15:52
12 strings in load path: 1.8 seconds @kbrock ^
Nick LaMuro
@NickLaMuro
Jan 18 2018 15:55
@jrafanie So based on https://gitter.im/ManageIQ/manageiq/performance?at=5a60b9355a9ebe4f75a28b0d , does it maybe make sense then to patch Rails then as well to do a .to_s when adding to the $LOAD_PATH?
Joe Rafaniello
@jrafanie
Jan 18 2018 15:55
@NickLaMuro maybe, let's open the bug and see what people say
Nick LaMuro
@NickLaMuro
Jan 18 2018 15:56
good point
Joe Rafaniello
@jrafanie
Jan 18 2018 15:56
is it common for engines to use pathnames off of Rails.root?
Nick LaMuro
@NickLaMuro
Jan 18 2018 16:00
well, halfway down in the Rails guides of the autoload_paths section, they give a String as an example
so... no? maybe?
let me get a room, @NickLaMuro are you ready to open the bug?
Nick LaMuro
@NickLaMuro
Jan 18 2018 16:13
give me 5 min
Joe Rafaniello
@jrafanie
Jan 18 2018 16:25
I'll start, just whenever you're free https://bluejeans.com/6328789311/
Jason Frey
@Fryguy
Jan 18 2018 16:50
I haven't read back yet, but can we add a test to manageiq proper to just verify there are not Pathname objects in the LOAD_PATH? This way if anyone updates a plugin or engine with a Pathname, and doesn't know about this performance issue, we can catch it early?
Mainly just a safety net test for ourselves but also it would document what the problem is so we remember WHY we can't put Pathname objects into the LOAD_PATH
Nick LaMuro
@NickLaMuro
Jan 18 2018 16:58
@Fryguy already working on doing that
Jason Frey
@Fryguy
Jan 18 2018 17:22
:+1:
Nick LaMuro
@NickLaMuro
Jan 18 2018 17:30

@Fryguy Oh, I misread that (thought you meant can we test this on an appliance). But yeah, good idea. I will include a test for that in the main repo.

Probably will also include a test for if we are using ruby < 2.5 in there as well, and if we aren't, fail so we can switch back once we finally upgrade past it.

Also, @jrafanie and I found out that this was not an issue in ruby 2.2, so this is only an issue for euwe on up
Nick LaMuro
@NickLaMuro
Jan 18 2018 17:37
@Fryguy WHY DID YOU MERGE!1! [ref]
Jason Frey
@Fryguy
Jan 18 2018 17:59
Cause it was ready?
Nick LaMuro
@NickLaMuro
Jan 18 2018 18:01
You wanted me to add a test, didn't you? And Joe even said we should add a comment mentioning the ruby bug...
Nick LaMuro
@NickLaMuro
Jan 18 2018 19:07

Welp, PRs created to add a bunch of comments (mostly):

ManageIQ/manageiq#16847
ManageIQ/manageiq-automation_engine#148
ManageIQ/manageiq-ui-classic#3276
ManageIQ/manageiq-graphql#36

Added a spec file in the main repo to test for this as well, and one to fail when we finally get to ruby 2.5.0 or greater

Joe Rafaniello
@jrafanie
Jan 18 2018 20:39
@NickLaMuro what do you think of using this BZ for all of these .to_s PRs for backport?
Joe Rafaniello
@jrafanie
Jan 18 2018 20:52
doh, it's in some of the PRs but not all of them. I'll fix them
@NickLaMuro so, I only have permission to the do the manageiq PR description update. Can you add the BZ link to the others? I already asked Chris about the graphql one. We'll need that to backport it
Nick LaMuro
@NickLaMuro
Jan 18 2018 21:07
I can fix them in a bit then
Joe Rafaniello
@jrafanie
Jan 18 2018 21:12
Sure, no rush
Nick LaMuro
@NickLaMuro
Jan 18 2018 21:33
For what it is worth, we only need to do the manageiq-ui-classic for the fine backport. Everything else didn't exist in the release prior to that.
Joe Rafaniello
@jrafanie
Jan 18 2018 21:35
sounds Fine to me
Chris Arcand
@chrisarcand
Jan 18 2018 21:38
Never gets old
Keenan Brock
@kbrock
Jan 18 2018 22:50
think hammer may get old (not yet for me), but the Fine puns will always be great
Chris Arcand
@chrisarcand
Jan 18 2018 22:56
Hammer will never get old.
It will always be Hammer time.
I'm 2 legit 2 quit making Hammer puns.
Keenan Brock
@kbrock
Jan 18 2018 23:23
sounds like a plan
@NickLaMuro sad to see ManageIQ/manageiq#14261 fade into obscurity
Nick LaMuro
@NickLaMuro
Jan 18 2018 23:26
yeah, though I am considering resurrecting it
got a few other things on the TODO list first though
OMG... it's all over the frick'n place...
(in our tool specs that is...)