tarantool / queue

Create task queues, add and take jobs, monitor failed tasks
Other
237 stars 52 forks source link

Remove redundant full-scan on stat build #93

Closed olegrok closed 4 years ago

olegrok commented 5 years ago

Need for #92

olegrok commented 4 years ago

Hi! Thanks for the patch! Look good, but I am proposing to add some benchmarks with results.

The simplest variant. I use vynil as engine, because it allows to get more indicative results

local clock = require('clock')
local json = require('json')
local queue = require('queue')

box.cfg {
    vinyl_memory = 100000,
    log_level = 3,
    vinyl_write_threads = 5,
}

local tube = queue.create_tube('bench', 'fifo', {engine = 'vinyl'})

local count = 1e5

local task = string.rep('x', 10)
for _ = 1, count do
    tube:put({task})
end

for _ = 1, count / 2 do
    tube:take()
end

print('Inserted:', count)

collectgarbage()
collectgarbage()

local start = clock.time()
local stats = queue.stats()
local result = clock.time() - start
print(result, json.encode(stats))

box.snapshot()
os.exit(0)

Before: 2.53s After: 2.13s

LeonidVas commented 4 years ago

Hello. Thanks for the benchmark. It's look like the patch improves performance(~16%). And LGTM. But I think that calculating statistics during a work of the process looks better. This will allow to have relevant data at hand.

olegrok commented 4 years ago

Hello. Thanks for the benchmark. It's look like the patch improves performance(~16%). And LGTM. But I think that calculating statistics during a work of the process looks better. This will allow to have relevant data at hand.

Yes, I agree, I think we could call current stats implementation once at start then increment counter for every operations. But it should be done in separate patch.

As it was done here: https://github.com/moonlibs/xqueue/blob/master/xqueue.lua#L1326

Totktonada commented 4 years ago

I think the fix is okay. We can left the issue #92 open and implement a kind of materizlized view later.

Totktonada commented 4 years ago

Oleg, please, don't do this anymore. I had the plan to release all critical fixes in one release (it'll as stable as possible), but perf. improvements in another one.

Totktonada commented 4 years ago

Restricted master branch.

olegrok commented 4 years ago

Ok, sorry. I didn't know about your plan and understand your "approve" as "ready to push"