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

remove count(*) from MiqQueue.get #14621

Merged
merged 1 commit into from
Apr 5, 2017
Merged

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Apr 3, 2017

For MiqQuery.get, we perform an unneeded COUNT(*), which is almost as expensive as the SELECT *

This PR removes that

ms bytes objects queries query (ms) rows comments
2,019.3 31,802,410 365,569 612 1,450.0 1,020 before-4
1,633.1 28,129,576 323,341 510 1,130.4 1,020 after-2
19.1% 12% 12% 17% 23% 0 diff

TMI

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

I highlighted the count(*) which is present in before but not after

ms query query ms rows comments
2,114.9 612 1,528.6 1,020 count#before-2
520.7 102 520.7 .SELECT COUNT(count_column)
511.2 102 482.3 1,020 .SELECT "miq_queue".*
108.0 102 97.7 .SELECT "miq_workers".*
48.7 102 48.7 .BEGIN
253.4 102 253.4 .UPDATE "miq_queue" SET "state" = 'dequeue', "handler_id" = 10000000000018, "handler_type" = 'MiqServer', "updated_on" = .{28}, "lock_version" = 1
86.6 102 86.6 .COMMIT

It is nice that overall, the query numbers are pretty consistent

ms bytes objects queries query (ms) rows comments
1,949.5 31,802,462 365,569 612 1,365.4 1,020 before-1
2,114.9 31,802,328 365,569 612 1,528.6 1,020 before-2
2,162.4 31,802,438 365,569 612 1,580.1 1,020 before-3
2,019.3 31,802,410 365,569 612 1,450.0 1,020 before-4
ms bytes objects queries query (ms) rows comments
1,608.7 28,129,620 323,341 510 1,106.5 1,020 after-1
1,633.1 28,129,576 323,341 510 1,130.4 1,020 after-2
1,655.4 28,129,582 323,341 510 1,151.2 1,020 after-3
1,511.1 28,129,690 323,341 510 1,018.5 1,020 after-4

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.

👍

take it to the limit!

@isimluk
Copy link
Member

isimluk commented Apr 4, 2017

@kbrock while this avoids the count, we can make it even more 🍨-y

I suggest we keep limit there and remove the line (return nil if msgs.empty?). Here is why:

The difference between take and limit is that take returns array, limit returns relation. When we call
msgs.empty? -> We either

  • run empty? on relation that runs count on that relation
  • or we run empty? on array

You changed the relation to the array, so we don't run the count. However (!) we don't need that empty? at all!!! See the code bellow. The empty? call is trying to be smarty, but it optimizes nothing really. See the code below the empty?.

So, I suggest, we just remove

return nil if msgs.empty? # Nothing available in the queue

and we are done.

@kbrock
Copy link
Member Author

kbrock commented Apr 4, 2017

@isimluk great suggestion.

@kbrock kbrock force-pushed the miq_queue_tweaks branch 2 times, most recently from 85162d4 to e3ff612 Compare April 4, 2017 21:41
@isimluk isimluk added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 5, 2017
@isimluk isimluk added the fine/no label Apr 5, 2017
@isimluk isimluk assigned isimluk and unassigned gtanzillo Apr 5, 2017
@isimluk isimluk closed this Apr 5, 2017
@isimluk isimluk reopened this Apr 5, 2017
@isimluk
Copy link
Member

isimluk commented Apr 5, 2017

@kbrock could you please clean this one? I think it is new:

❗️ - Line 178, Col 8 - Style/TrailingWhitespace - Trailing whitespace detected.

@miq-bot
Copy link
Member

miq-bot commented Apr 5, 2017

This pull request is not mergeable. Please rebase and repush.

to avoid logging a nil, moved logging up a few lines
@miq-bot
Copy link
Member

miq-bot commented Apr 5, 2017

Checked commit kbrock@eb4d55d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍪

@isimluk isimluk merged commit abe4184 into ManageIQ:master Apr 5, 2017
@kbrock kbrock deleted the miq_queue_tweaks branch April 6, 2017 01:23
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.

4 participants