Conversation
philiplantz
left a comment
There was a problem hiding this comment.
I don't think any of these comments are showstoppers.
| { | ||
| bool result = 0; | ||
| __asm__ volatile ("lock bts%z1 %2, %1\n; setc %0" | ||
| : "=q"(result), "+m"(m) : "Ir"(T(bit)) : "memory", "cc"); |
There was a problem hiding this comment.
__asm__ volatile ("lock bts%z1 %2, %1"
: "=@ccc"(result), "+m"(m) : "Ir"(T(bit)) : "memory");
This requires gcc version 6.
It may be better to use an intrinsic.
There was a problem hiding this comment.
mmm interesting. Almost all the machines we regularly use have gcc 5.4. If intrinsics would about the dependency with gcc 6, that could be enough good reason.
There was a problem hiding this comment.
I am not sure I follow. Intrinsic will need gcc6? Cause this instruction works with gcc 5 and 4.
|
|
||
| while (1) { | ||
| uint16_t r = xadd(_rw_lock, READER_INCR); | ||
| assert((r & LOCK_READER_MASK) != LOCK_READER_MASK); |
There was a problem hiding this comment.
Probably this should throw instead of assert.
|
|
||
| // This function should be called only when the caller already possesses | ||
| // a read lock. Rather than making this recursive, we are relying on the | ||
| // caller to use this correctly, hence this is not visible to applications. |
There was a problem hiding this comment.
What does "not visible to applications" mean?
|
|
||
| template <typename T> | ||
| static inline void atomic_and(volatile T &m, T v) | ||
| { asm volatile ("lock and%z0 %1, %0" : "+m"(m) : "ri"(v)); } |
There was a problem hiding this comment.
Perhaps this should have a "memory" clobber, since it is used as a release. But if the caller of unlock ensures this, then it isn't necessary here.
There was a problem hiding this comment.
asm volatile ("lock and%z0 %1, %0" : "+m"(m) : "ri"(v) : "memory" );
| { | ||
| bool result = 0; | ||
| __asm__ volatile ("lock bts%z1 %2, %1\n; setc %0" | ||
| : "=q"(result), "+m"(m) : "Ir"(T(bit)) : "memory", "cc"); |
There was a problem hiding this comment.
I'm not sure whether this needs a memory clobber or not. I don't think the acquire semantics require it. Nor is it required if bit < sizeof (T). If bit can be ≥ sizeof (T), then replacing the memory clobber with "+m"((&m)[bit/sizeof (T)]) might be an improvement.
It may be better to use an intrinsic.
There was a problem hiding this comment.
For now we have decided to leave it there. Need to revisit this for PMGD code.
| if (_readonly) | ||
| _tx = new Transaction(*_db, Transaction::ReadOnly); | ||
| else | ||
| _tx = new Transaction(*_db, Transaction::ReadWrite); |
There was a problem hiding this comment.
I suggest:
int transaction_options = _readonly ? Transaction::ReadOnly : Transaction::ReadWrite;
_tx = new Transaction(*_db, transaction_options);
| assert(_tx == NULL); | ||
| _dblock->lock(); | ||
|
|
||
| // Assuming one query handler handles one TX at a time. |
There was a problem hiding this comment.
We are safe with this one. It will require a lot more design changes to affect that.
|
I have done some of the fixes above. So pushing an update and creating a new request. |
This is a temporary fix until PMGD becomes fine-grained concurrent. It would be good if we could find out the performance difference.