These are chat archives for ManageIQ/manageiq/performance

1st
Dec 2016
Jason Frey
@Fryguy
Dec 01 2016 16:38
@kbrock Reading back... x.save if x.changed? is really only to avoid the empty BEGIN/COMMIT, right? See rails/rails#17937
Keenan Brock
@kbrock
Dec 01 2016 16:38
no
Jason Frey
@Fryguy
Dec 01 2016 16:38
then I don't understand what it's doing versus just calling save directly
Keenan Brock
@kbrock
Dec 01 2016 16:39
ooh, um
maybe for that
Jason Frey
@Fryguy
Dec 01 2016 16:39
because save already checks for changed? under the covers
Keenan Brock
@kbrock
Dec 01 2016 16:39
I was suggesting x.attr= new_value if x.attr != new_value
Jason Frey
@Fryguy
Dec 01 2016 16:39
versus what
Keenan Brock
@kbrock
Dec 01 2016 16:39
x.attr = new_value
Jason Frey
@Fryguy
Dec 01 2016 16:40
Interesting...does it make a difference?
Keenan Brock
@kbrock
Dec 01 2016 16:40
when attr is a serialized / double / who knows what
attr_changed? is wrong and changed? is wrong
only really matters when bulk loading a ton of stuff
Jason Frey
@Fryguy
Dec 01 2016 16:40
really? interesting
Keenan Brock
@kbrock
Dec 01 2016 16:40
and I don't totally understand why you would want that
I mean
Jason Frey
@Fryguy
Dec 01 2016 16:41
separate seriazlied from double and other primitives because serializd has always been different
Keenan Brock
@kbrock
Dec 01 2016 16:41
changed? should know if it changed. and it seems to just work
Jason Frey
@Fryguy
Dec 01 2016 16:41
if you have a double, I don't see why it would matter
Keenan Brock
@kbrock
Dec 01 2016 16:41
maybe it is date time
because the difference is micro seconds
Jason Frey
@Fryguy
Dec 01 2016 16:41
so basically types that you are expecting "conversion"
Keenan Brock
@kbrock
Dec 01 2016 16:41
yea
I tend not to do this
but when someone comes up and says "there are way too many saves here when nothing changed"
then I lean back on some odd experiences
Jason Frey
@Fryguy
Dec 01 2016 16:42
unfortunately, then you have to know those types ahead of time and treat them different then, right?
sucks for the caller
Keenan Brock
@kbrock
Dec 01 2016 16:42
yes
so we tend to say good enough
and never do this
and to be honest - it is good enough
until you are bulk loading 10,000vms
Jason Frey
@Fryguy
Dec 01 2016 16:42
yeah, I would expect in most cases it shouldn't make much difference, but yeah in bulk it might be bad
Keenan Brock
@kbrock
Dec 01 2016 16:42
yea
Jason Frey
@Fryguy
Dec 01 2016 16:42
do you have a specific case where this is a problem...like a log snippet or something I could see?
Keenan Brock
@kbrock
Dec 01 2016 16:43
and I was never able to fully get to the bottom of it
Jason Frey
@Fryguy
Dec 01 2016 16:43
(because I had fixed most of those problems in refresh)
and I think you fixed the serialized columns stuff in Rails :D
Keenan Brock
@kbrock
Dec 01 2016 16:43
and it may be different now. that stuff has changed over the years a lot
:)
I think the code base I was on was 2.2 - and no serialized columns
I'd like to say it was doubles and dates?
wonder even if booleans get (got?) confused.
Jason Frey
@Fryguy
Dec 01 2016 16:45
it only happens after round trip to the database
because it;s the DB that does the truncating
Keenan Brock
@kbrock
Dec 01 2016 16:45
it comes out in specs. because you reload, and the new value isn't exactly the same as the value set
especially with dates
Jason Frey
@Fryguy
Dec 01 2016 16:45
right, that's the round trip
yeah, that's why I wrote be_same_time_as :P
only have that issue as far as I know
Keenan Brock
@kbrock
Dec 01 2016 16:46
so @Ladas said that the changed? calculation was expensive in some instances
or rather, in save! it did some other stuff around changed? and it saved some time. like a few percent
Ladislav Smola
@Ladas
Dec 01 2016 16:46
actually doing obj.save! if obj.changed? , is the less expensive
Jason Frey
@Fryguy
Dec 01 2016 16:46
ah interesting...i'd be curious to know specfics on that
yeah, obj.save! if obj.changed? is definiltey less expensive because of the BEGIN/COMMIT stuff
Keenan Brock
@kbrock
Dec 01 2016 16:47
yea, so for his case, he has more solid numbers. and I think code snippets to prove it
Jason Frey
@Fryguy
Dec 01 2016 16:47
I'd rather not change that everywhere though and would rather get that fixed in Rails directly :/
Ladislav Smola
@Ladas
Dec 01 2016 16:47
yeah, the nil transaction
Jason Frey
@Fryguy
Dec 01 2016 16:47
yup
@Ladas : rails/rails#17937
Keenan Brock
@kbrock
Dec 01 2016 16:48
@Ladas is it cheaper for the nil transaction, or some other calculations. I though it wasn't purely the nil transaction
Ladislav Smola
@Ladas
Dec 01 2016 16:48
I have the obj.save! if obj.changed? in DtoRefresh now
Jason Frey
@Fryguy
Dec 01 2016 16:48
We actually "fixed" that YEARS ago in Rails, but never pushed upstream
Ladislav Smola
@Ladas
Dec 01 2016 16:48
but I can make it an configuration option
Keenan Brock
@kbrock
Dec 01 2016 16:48
but that is an extreme case. think we all agree not doign that everywhere
Jason Frey
@Fryguy
Dec 01 2016 16:48
no, for refresh, don't make it configurable, just do it
Keenan Brock
@kbrock
Dec 01 2016 16:48
just do it +1
Jason Frey
@Fryguy
Dec 01 2016 16:48
in fact, you can do it for regular refresh too
Keenan Brock
@kbrock
Dec 01 2016 16:49
::Settings.refresh.fast = true use that setting
:trollface:
Ladislav Smola
@Ladas
Dec 01 2016 16:49
well, I haven't tracked the exact cause, but doing the obj.save! if obj.changed?, is cutting the refresh time to half (tested on the 70k images)
@kbrock yeah :-)
@Fryguy yeah, we could do thati n the regular refresh too
@Fryguy I am slowly bringing the optimizations to old refresh too :-)
Keenan Brock
@kbrock
Dec 01 2016 16:50
I feel badly stealing this stuff back into regular refresh. reduces ladas' before and after numbers
Ladislav Smola
@Ladas
Dec 01 2016 16:50
but I don't want to break the old refresh, since it's my backup :-)
Keenan Brock
@kbrock
Dec 01 2016 16:50
lol
Jason Frey
@Fryguy
Dec 01 2016 16:51
nah...I should have put it in the old refresh to begin with when we realized we removed the BEGIN/COMMIT optimization
Ladislav Smola
@Ladas
Dec 01 2016 16:51
@kbrock yeah, exactly, makes my DTO benchmarks look bad :-)
Keenan Brock
@kbrock
Dec 01 2016 16:51
I thought obj.changed? only looks at obj where as obj.save looks at obj.children.changed? too
Jason Frey
@Fryguy
Dec 01 2016 16:51
changed? is not nearly as expensive as the empty begin/commit
Keenan Brock
@kbrock
Dec 01 2016 16:52
@Ladas you need to get a sha for the before work. so improvement to regular refresh will work
Jason Frey
@Fryguy
Dec 01 2016 16:52
@ladas regular refresh will still be around for a long time anyway, AND we can backport this change
Ladislav Smola
@Ladas
Dec 01 2016 16:52
@kbrock I don't do the save nesting in Dto, so it should be fine there :-)
@Fryguy yeah, it probably will be
Jason Frey
@Fryguy
Dec 01 2016 16:53
v = VmOrTemplate.first
v.attributes.each { |key, val| v.send("#{key}=", val) }
v.changed?
# => false
VmOrTemplate.columns.map(&:type).uniq.sort
# => [:boolean, :datetime, :integer, :string, :text]
Ladislav Smola
@Ladas
Dec 01 2016 16:54
@Fryguy though it's sucks to maintain 2 code paths, so I can see the old refresh will disappear for some providers in next^2 release
Jason Frey
@Fryguy
Dec 01 2016 16:55
I'm actually expecting even more refreshers depending on the usage cases but we will see
depends how generic the DAG refresh will end up being
Ladislav Smola
@Ladas
Dec 01 2016 16:56
@Fryguy well Adam is rewriting VMWare to DTO
Jason Frey
@Fryguy
Dec 01 2016 16:56
(I've been thinking we should stop calling it DTO refresh, because it's not...a DTO is basically a struct of data)
This is more about the DAG and the processing you are doing on that graph to optimize the refresh
Ladislav Smola
@Ladas
Dec 01 2016 16:56
@Fryguy we do fallback on a custom saving code for Vms though
@Fryguy but still, the custom saving code, if needed, can be written more effective using the dto
Jason Frey
@Fryguy
Dec 01 2016 16:57
In other words, we could introduce just the DTOs to the old refresh too, and then it would just be confusing :)
Ladislav Smola
@Ladas
Dec 01 2016 16:57
hehe
Jason Frey
@Fryguy
Dec 01 2016 16:57
@ladas yeah, waiting to see how adam's changes go
Ladislav Smola
@Ladas
Dec 01 2016 16:57
yes, it stopped being DTO when I added logic to it :-)
Jason Frey
@Fryguy
Dec 01 2016 16:58
yup
Ladislav Smola
@Ladas
Dec 01 2016 16:58
actually, it was never really DTO
Jason Frey
@Fryguy
Dec 01 2016 16:58
"DAG refresh"? I can't think of something better
Ladislav Smola
@Ladas
Dec 01 2016 16:59
not sure really
Jason Frey
@Fryguy
Dec 01 2016 17:00
think about it :) maybe we can discuss it at the next meeting
Ladislav Smola
@Ladas
Dec 01 2016 17:01
but the Dto class, that is almost Dto :-) expect of the blacklist/whitelist features
although in the end, it's really: DTO does not have any behavior except for storage and retrieval of its own data
it really just return it's data (in a more complex form) and knows about a unique index (which could be moved)
Jason Frey
@Fryguy
Dec 01 2016 17:06
right...a glorified struct :)
Ladislav Smola
@Ladas
Dec 01 2016 17:14
:-)
then the other logic is in DtoCollection and DtoLazy, which I guess needs it's own definition :-)