use a waitgroup to wait for reserve holders of MFiles before unmapping

[carlhoerberg]

WHAT is this pull request doing?

Please fill me in.

HOW can this pull request be tested?

Specs? Manual steps? Please fill me in.

[spuun]

Since we're in #close I guess we can be sure here that counter won't be increased, only decreased? So no risk that counter is zero in this check but non-zero when unmap is called?

What about always spawning instead of this if-statement? It would be nice to not have to access an instance variable. Or maybe monkey patch WaitGroup with a done? method.

[carlhoerberg]

"risk" that it is zero? the optimization here was for that fact that it will always be zero for single node clusters and for transient queues etc, and then we don't need to spawn a fiber.

could monkey patch though.

[spuun]

I mean if it's zero when we do the check, but it has changed when we call unmap so we're unmapping even though it's reserved. However, I think we can assume that the counter will never be increased at this point since we're in #close so the MFile shouldn't be used anymore?

[spuun]

What about naming it done and call it from the action loop when an Action has been sent, instead of assuming that send will be called only once for an Action and call unreserve from inside send?

[carlhoerberg]

makes sense, thanks!

[carlhoerberg]

put done in ensure in the Action#send method instead

[carlhoerberg]

yes, the assumption is that Action#send is only called once

[spuun]

Instead of assuming that, which would be an internal implementation detail the caller may need to know about, I think it would make sense to do

action.send(lz4, Log)
action.done

in the action loop.

[carlhoerberg]

It gets ugly because you have to have a begin/recue block, and then you have two loops, so two blocks on top of that

[spuun]

True

[spuun]

Two things that we probably must remove

• unmap from Queue#rm_consumer
• unmap from USR2 handler

[carlhoerberg]

Two things that we probably must remove
* unmap from Queue#rm_consumer
* unmap from USR2 handler

I don't know, both call MessageStore#unmap_segments, which doesn't unmap the last segment that is still being written to.

lavinmq/src/lavinmq/amqp/queue/message_store.cr

Lines 231 to 237 in c8ca36a

	def unmap_segments(except : Enumerable(UInt32) = StaticArray(UInt32, 0).new(0u32))
	@segments.each do |seg_id, mfile|
	next if mfile == @wfile
	next if except.includes? seg_id
	mfile.unmap
	end
	end

[spuun]

I don't know, both call MessageStore#unmap_segments, which doesn't unmap the last segment that is still being written to.

Isn't it possible that a segment that isn't yet fully replicated is unmapped? If it's a large message I think the unmap may occur in the middle of the lz4 compression.

Will these unmaps be necessary if we handle unmapping properly, which this PR hopefully solves?

[carlhoerberg]

I don't know, both call MessageStore#unmap_segments, which doesn't unmap the last segment that is still being written to.

Isn't it possible that a segment that isn't yet fully replicated is unmapped? If it's a large message I think the unmap may occur in the middle of the lz4 compression.

Will these unmaps be necessary if we handle unmapping properly, which this PR hopefully solves?

True, there's a chance for that, if the unmap is triggered just when we're rolling over to a new segment.

And yes, shouldn't be needed anymore.

[carlhoerberg]

also started to tinker with a ref counter as you did, but yes, it fast gets messy, as we return a BytesMessage from MessageStore, but it points into the mmaping.. so we don't know when the client has sent them..

class MFile
  @counter = 0
  @counter_lock = Mutex.new(:unchecked)

  def borrow : self
    @counter_lock.synchronize do
      counter = @counter
      if counter.zero?
        mmap
      end
      @counter = counter + 1
    end
    self
  end

  def unborrow : Nil
    @counter_lock.synchronize do
      counter = @counter -= 1
      if counter.zero?
        unmap
      end
    end
  end
end

[carlhoerberg]

I'm starting to long for the io_uring implementation, then we could skip mmap:ings all together..

[spuun]

I'm starting to long for the io_uring implementation, then we could skip mmap:ings all together..

Yes! But I think this PR is working pretty well? I've done some runs without crashes.