These are chat archives for ManageIQ/manageiq/performance

15th
Mar 2017
Keenan Brock
@kbrock
Mar 15 2017 14:40

@imtayadeway @lpichler https://github.com/ManageIq/manageiq/blob/master/spec/lib/miq_expression_spec.rb#L2588
When I pass tag "managed-location" into MiqExpression#parse_field_or_tag ( actually, MiqExpression::Tag.parse) it says "um, no => Nil"

Do we actually have code that passes in "managed-field" without the model?
Is this valid?

Libor Pichler
@lpichler
Mar 15 2017 14:41
@kbrock no it is not valid
such tag field can’t exist in any MiqExpression condition without model
Keenan Brock
@kbrock
Mar 15 2017 14:46
ugh, grep -r '[^_n]managed' product/ says otherwise
Libor Pichler
@lpichler
Mar 15 2017 14:51
it looks like it is only in col_order and there is it without model
REGEXs in children of MiqExpression::Target counts with form with model - because it is valid for MiqExpression
for col_order the model is taken from :db
Keenan Brock
@kbrock
Mar 15 2017 14:54
well, col_order is slightly different beast
Libor Pichler
@lpichler
Mar 15 2017 14:55
so maybe you should join db + tag before you will call parse_field_or_tag in get_col_info?
Keenan Brock
@kbrock
Mar 15 2017 14:55
hmm - thanks
don't think we call get_col_info on the order anyway
my goal is to drop get_col_info and push everything towards parse_field_or_tag
although, looks like we should call it parse_field_or_tag_or_count_or_... ;)
Keenan Brock
@kbrock
Mar 15 2017 15:00
I'm mixed on continuing our currenty format or moving over towards more sql/ruby friendly syntax
Jason Frey
@Fryguy
Mar 15 2017 16:23
curious why this just isn't a fix in ActiveRecord
seems pretty straightforward that AR would take a fix that all of those methods would work the same, IMO
Keenan Brock
@kbrock
Mar 15 2017 16:24
if there is no scope, then it works
sounds like if there is a scope, then it won't
Jason Frey
@Fryguy
Mar 15 2017 16:24
yeah, that's what I mean...sounds like an AR issue...
a scope or no scope is still just an AR relation
Oleg Barenboim
@chessbyte
Mar 15 2017 16:25
@kbrock want to put together an ActiveRecord PR or at least open an issue and then ping Aaron or Matthew?
Jason Frey
@Fryguy
Mar 15 2017 16:25
I get that they might want a method that always goes back to the database... a la size vs count vs length
but present? is just straight up wrong, IMO
Keenan Brock
@kbrock
Mar 15 2017 16:25

reading this, totally makes me want to get my hybrid AR methods into place

b/c we could use scopes on in memory and db scopes - and not take an extra hit

ems.vms.active
I just put in an AR PR fixing a bug - and it has just sat
Oleg Barenboim
@chessbyte
Mar 15 2017 16:26
instead have ems.active_vms?
Keenan Brock
@kbrock
Mar 15 2017 16:26
well, if vms is in memory
I want it to use ruby select
and if it is in the db, I want it to use a where clause filter
but you have to be able to specify the difference
um
but you have to be able to write code that both sql and ruby can understand
but yea
Oleg Barenboim
@chessbyte
Mar 15 2017 16:27
maybe we can fork on Array vs AR Relation?
Keenan Brock
@kbrock
Mar 15 2017 16:27
active_vms which is a vms scope would work. wouldn't leverage vms but could work
exactly!
have a prototype, but colleting dust somewhere
Oleg Barenboim
@chessbyte
Mar 15 2017 16:28
LOL
Keenan Brock
@kbrock
Mar 15 2017 16:28
#too_many_side_projects
Oleg Barenboim
@chessbyte
Mar 15 2017 16:28
what is this affecting?
Keenan Brock
@kbrock
Mar 15 2017 16:28
we are very MiqPreloader.preload happy
Oleg Barenboim
@chessbyte
Mar 15 2017 16:28
@kbrock I think even your side projects have side projects of their own
Keenan Brock
@kbrock
Mar 15 2017 16:29
but now that we are starting to use :through or active_vms - it means we are double downloading stuff
turtles all the way down
Ladislav Smola
@Ladas
Mar 15 2017 16:29
lol
Keenan Brock
@kbrock
Mar 15 2017 16:31
Parallel query in PG 9.6 might help with the COUNTs : https://wiki.postgresql.org/wiki/Parallel_Query
@hayesr
class Relation # somewhere in AR land
    # Returns true if there are no records.
    def empty?
      return @records.empty? if loaded?

      limit_value == 0 || !exists?
    end
end
Oleg Barenboim
@chessbyte
Mar 15 2017 16:36
what is limit_value?
Keenan Brock
@kbrock
Mar 15 2017 16:36
that is the value specified by limit
Vm.limit(3).limit_value == 3
Oleg Barenboim
@chessbyte
Mar 15 2017 16:36
so, meant to speed up: Vm.limit(0).empty?
Keenan Brock
@kbrock
Mar 15 2017 16:37
yea
sweet - hu?
and Vm.none.empty?
Oleg Barenboim
@chessbyte
Mar 15 2017 16:37
I guess -- who the heck does limit(0) though?
Keenan Brock
@kbrock
Mar 15 2017 16:38
well, you call a method and it returns Vm.none
and down the line you call empty?
also the use of loaded? is very cool
it is why we get optimizations by introducing to_a / preload
Oleg Barenboim
@chessbyte
Mar 15 2017 16:38
feels very inside-out kind of performance improvements
Keenan Brock
@kbrock
Mar 15 2017 16:38
well
we call any? and empty? all the time
because we're expecting there to be no rows
yea, it is a very small edge case
Oleg Barenboim
@chessbyte
Mar 15 2017 16:39
I understand, but they are saving a single query -- not really a big performance hit
Keenan Brock
@kbrock
Mar 15 2017 16:39
well
Oleg Barenboim
@chessbyte
Mar 15 2017 16:39
but, Jason's empty BEGIN/COMMIT just lingers for years
Keenan Brock
@kbrock
Mar 15 2017 16:39
removing count(*) from rbac was my first 20% page speed up on Vm screen
Oleg Barenboim
@chessbyte
Mar 15 2017 16:40
I meant it more as a criticism of Rails core team -- not you
Keenan Brock
@kbrock
Mar 15 2017 16:40
ooh
I thought it was smart
but yea
present?
rails introduced the method
but forgot to cover it
that is a no brainer
will have PR in there soon
Keenan Brock
@kbrock
Mar 15 2017 16:55
@chessbyte @Fryguy rails/rails#28360 -- could you +1 this?
Or if you have a better suggestion, please share
Oleg Barenboim
@chessbyte
Mar 15 2017 16:56
well, you have a dyslexic typo in LT;DR :-)
Keenan Brock
@kbrock
Mar 15 2017 17:00
aaah
that is why it hasn't been merged
Oleg Barenboim
@chessbyte
Mar 15 2017 17:01
LOL
Keenan Brock
@kbrock
Mar 15 2017 17:07
ugh, so much work to add a single method to rails
cool, rails has:
    assert_no_queries { assert_equal true, posts.empty? }
Oleg Barenboim
@chessbyte
Mar 15 2017 17:08
and the magic incantations to summon Aaron or Matthew are not working?
Keenan Brock
@kbrock
Mar 15 2017 17:09
I tried one ping
hmm
why am I implementing present?
ugh :sparkles: - /dev/null the thing
Jason Frey
@Fryguy
Mar 15 2017 17:26
@kbrock It's probably not getting merged because I don't think you sufficiently proved that transaction can be nil. So, from their perspective you have a fix for a problem they don't see existing
Joe Rafaniello
@jrafanie
Mar 15 2017 17:29
ugh, that transaction code is rough
hasn't changed that much from 2.3 days
Jason Frey
@Fryguy
Mar 15 2017 17:29
yeah it's pretty gnarly
it looks a bit cleaned up from then...at least the other methods around it
Joe Rafaniello
@jrafanie
Mar 15 2017 17:30
yeah, I agree with @Fryguy... no test and unclear reasoning for a nil transaction
Jason Frey
@Fryguy
Mar 15 2017 17:34
I should just bite the bullet and code up BEGIN COMMIT myself :/
Keenan Brock
@kbrock
Mar 15 2017 17:36
ok
so what do I have to do
they test for transaction.nil? all over the place
so I added code that did the same
but
Jason Frey
@Fryguy
Mar 15 2017 17:36
I think you have to prove how the stack gets to be nil, but gets to a commit
Keenan Brock
@kbrock
Mar 15 2017 17:36
they aren't convinced that the other transaciton.nil? even needed to exist?
Jason Frey
@Fryguy
Mar 15 2017 17:37
maybe a test if you can figure it out
Keenan Brock
@kbrock
Mar 15 2017 17:37
yea
no idea why it can be nil
Jason Frey
@Fryguy
Mar 15 2017 17:37
if you have a failing test, you are much more likely to get it merged

no idea why it can be nil

exactly why they won't merge

Keenan Brock
@kbrock
Mar 15 2017 17:37
I think it started happening when we started killing workers
Jason Frey
@Fryguy
Mar 15 2017 17:37
interesting...maybe a signal gets sent to the process?
Keenan Brock
@kbrock
Mar 15 2017 17:37
again
total speculation
Jason Frey
@Fryguy
Mar 15 2017 17:37
or like a SystemExit or some other Exception?
Keenan Brock
@kbrock
Mar 15 2017 17:37
but I'm guessing a signal goes into there
yea
ugh
Jason Frey
@Fryguy
Mar 15 2017 17:38
cool idea
Keenan Brock
@kbrock
Mar 15 2017 17:38
hmm
thanks for feedback
Joe Rafaniello
@jrafanie
Mar 15 2017 17:38
hwo about timeout?
Keenan Brock
@kbrock
Mar 15 2017 17:38
exactly
but the code looks simple enough
Joe Rafaniello
@jrafanie
Mar 15 2017 17:38
we know it can interrupt during an ensure
Keenan Brock
@kbrock
Mar 15 2017 17:38
it actually interrupted so transaction = nil
the first line
the ensure is not safe / doesnt' handle transaction.nil?
all the other code does handle that though

I can't believe I couldn't get this merged:

Topic.order(Topic.arel_table[:title].lower).reverse_order

I was told arel is not supported

Joe Rafaniello
@jrafanie
Mar 15 2017 17:41
in a loop, create a process that sits in a transaction (logging all the thigns), send a signal to that process randomly
Jason Frey
@Fryguy
Mar 15 2017 17:42
yeah try @jrafanie's suggestion to see if that's actually the problem, then figure out how to craft a test afterwards
Joe Rafaniello
@jrafanie
Mar 15 2017 17:42
in a loop, doing a Time.timeout with a random value, do random things in a transaction, try to get timeout to crush you in an ensure
Jason Frey
@Fryguy
Mar 15 2017 17:42
you may even be able to get away with one of their "rais-apps-in-a-script" things
Keenan Brock
@kbrock
Mar 15 2017 17:43
@Fryguy reference?
Jason Frey
@Fryguy
Mar 15 2017 17:43
@jrafanie reference?
Joe Rafaniello
@jrafanie
Mar 15 2017 17:43
yeah, I have a few...
one sec
Jason Frey
@Fryguy
Mar 15 2017 17:43
haha...I can't remember where they talk about that
Keenan Brock
@kbrock
Mar 15 2017 17:43
@jrafanie it doesn't die in an ensure, it dies in the transaction = - first line int he begin
here's the link for the templates ^
Keenan Brock
@kbrock
Mar 15 2017 17:45
thanks
Joe Rafaniello
@jrafanie
Mar 15 2017 17:45
@kbrock yeah, the idea is maybe an exception/interrupt gets handle improperly at lower layer causing the transcation to be nil
Keenan Brock
@kbrock
Mar 15 2017 17:45
ManageIQ/manageiq#14064 <== the issue that I was trying to fix
Joe Rafaniello
@jrafanie
Mar 15 2017 17:45
wait, can nested transactions cause it to be nil if it's already in a trans?
Keenan Brock
@kbrock
Mar 15 2017 17:46
felt like I was comming up with tons of questions like that
we're getting that stack trace in production more and more
61 line rails app. wow
I provide no answers, only questions
HAHA
Keenan Brock
@kbrock
Mar 15 2017 17:50
hmm
do you think I need to use postgres to reproduce?
or do you think sqlite works similar enough to postgres here?
Joe Rafaniello
@jrafanie
Mar 15 2017 17:51
I would use pg
transaction code is different with various adapters
Keenan Brock
@kbrock
Mar 15 2017 18:16
ok, so I was able to reproduce that warning only 1 time
Keenan Brock
@kbrock
Mar 15 2017 18:35
@jrafanie yay - I just reproduced 2 times
it does not show up in rails head
but 5.0.0.1 it does
Joe Rafaniello
@jrafanie
Mar 15 2017 18:36
wow, the nil? or the warning?
Keenan Brock
@kbrock
Mar 15 2017 18:42
the nil
was running and running
got the warning here and there
but switched over to 5.0.1 and BAM
Joe Rafaniello
@jrafanie
Mar 15 2017 18:43
5.0.1 and 5.0.0.1?
but not master?
Keenan Brock
@kbrock
Mar 15 2017 18:44
ooh
got it after running 30 times
yes 5.0.0.1, not master
interesting, you can not control-c this code
Joe Rafaniello
@jrafanie
Mar 15 2017 18:49
so, SIGINT/SIGTERM does it?
Keenan Brock
@kbrock
Mar 15 2017 18:56
well
for some reason, SIGINT/SIGTERM is doing nothign for me
can't control-c my script at all
someone is eating it
cool. I can reproduce on master
Jason Frey
@Fryguy
Mar 15 2017 19:03
good job @kbrock ... reproduction on master will go a long way
Keenan Brock
@kbrock
Mar 15 2017 19:11
and
we know WHY it happens
thanks for the pointers to code and others
Keenan Brock
@kbrock
Mar 15 2017 19:38
@jrafanie do you suggest I put this into a gist or into the PR itself?
Joe Rafaniello
@jrafanie
Mar 15 2017 19:47
Sorry, on a call... gist and link to the gist in your PR