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

MiqQueue - remove MiqWorker lookup #14620

Merged
merged 3 commits into from
Apr 5, 2017

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Apr 3, 2017

For MiqQuery.get, we perform an unneeded worker lookup, which is always the same for a given process.

ms bytes objects query query ms rows comments
1,894.9 31,002,739 353,789 602 1,373.5 955 count#before-1
1,778.1 26,014,249 289,589 502 1,328.1 955 count#after-5
6.2% 16% 18% 17% 3.3% 0 difference

TMI

MiqWorker.my_worker ; 100.times { MiqQueue.put(:class_name  => 'MyClass', :method_name => 'method1', :args => [1, 2]) }
bookend("miq_queue.count") { 102.times { MiqQueue.get } }

I highlighted theSELECT which is present before but not after.

ms query query ms rows comments
1,900.1 602 1,356.4 955 count#after-1
448.1 102 448.1 .SELECT COUNT(count_column)
458.4 100 435.0 955 .SELECT "miq_queue".*
64.9 100 54.9 .SELECT "miq_workers".*
43.8 100 43.8 .BEGIN
261.6 100 261.6 .UPDATE "miq_queue" SET "state" = 'dequeue', "handler_id" = 1, "handler_type" = 'MiqServer', "updated_on" = .{28}, "lock_version" = 1
79.6 100 79.6 .COMMIT

The numbers I collected for this one were a little less consistent, but make they do show the trend

ms bytes objects query query ms rows comments
1,894.9 31,002,739 353,789 602 1,373.5 955 count#before-1
1,900.1 31,002,725 353,789 602 1,356.4 955 count#before-2
2,008.4 31,002,759 353,789 602 1,477.2 955 count#before-3
1,856.9 31,003,215 353,789 602 1,331.8 955 count#before-4
ms bytes objects query query ms rows comments
1,816.7 26,013,787 289,589 502 1,368.8 955 count#after-1
1,820.6 26,013,789 289,589 502 1,372.6 955 count#after-2
1,857.9 26,013,819 289,589 502 1,393.4 955 count#after-3
1,771.0 26,014,391 289,589 502 1,317.5 955 count#after-4
1,778.1 26,014,249 289,589 502 1,328.1 955 count#after-5

else
msg.update_attributes!(:state => STATE_DEQUEUE, :handler => w)
end
w = MiqWorker.server_scope.find_by(:pid => Process.pid) || MiqServer.my_server
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call it handler instead of w in a second commit?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that better - BUT, after looking at MiqQueue.get I have another optimization here :)

@kbrock kbrock changed the title MiqQueue - consolidate MiqWorker lookup MiqQueue - remove MiqWorker lookup Apr 3, 2017
@kbrock kbrock force-pushed the miq_queue_worker_refactor branch from a246ca9 to c340673 Compare April 3, 2017 19:34
@miq-bot
Copy link
Member

miq-bot commented Apr 3, 2017

Checked commits kbrock/manageiq@a568b74~...c340673 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 👍

Copy link
Member

@isimluk isimluk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -267,6 +267,8 @@ def self.start_worker(*params)
w
end

cache_with_timeout(:my_worker) { server_scope.find_by(:pid => Process.pid) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbrock what happens with subclasses that call my_worker? MiqGenericWorker.my_worker as an example. Is it sharing a class variable or is it not shared?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be weird if we're in a priority worker process and MiqGenericWorker.my_worker returned a MiqPriorityWorker row.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok, but it's kinda weird if subclasses can return rows of not of it's type.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will multiple workers run on the same pid?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kbrock no, I guess what I'm saying is it doesn't really make sense at the subclass level. We only want this on MiqWorker and not be inherited. I'm fine with it, just curious. I wasn't sure if you tried it out.

@jrafanie jrafanie added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 5, 2017
@jrafanie jrafanie merged commit 2a5daf4 into ManageIQ:master Apr 5, 2017
@kbrock kbrock deleted the miq_queue_worker_refactor branch April 5, 2017 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants