HPCC-15496: Refactor DataBuffer code #8606
Conversation
ghalliday
changed the title
Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>
|
@richardkchapman please review. The main changes are:
|
| @@ -5081,30 +5077,31 @@ class CDataBufferManager : public CInterface, implements IDataBufferManager | |||
| { | |||
| if (curBlock) | |||
| { | |||
| DataBufferBottom *bottom = (DataBufferBottom *) curBlock; | |||
| DataBufferBottom *bottom = curBlock; | |||
| assertex(((memsize_t)bottom & HEAP_PAGE_OFFSET_MASK) == 0); | |||
|
Automated Smoketest |
| bottom->freeChain = x->next; | ||
| x->next = NULL; | ||
| if (memTraceLevel >= 4) | ||
| DBGLOG("RoxieMemMgr: CDataBufferManager::allocate() reallocated DataBuffer - addr=%p", x); | ||
| return ::new(x) DataBuffer(); | ||
| } | ||
| else if ((memsize_t)(nextAddr - curBlock) <= HEAP_ALIGNMENT_SIZE-DATA_ALIGNMENT_SIZE) | ||
| else if ((((memsize_t)nextAddr) & HEAP_PAGE_OFFSET_MASK) != 0) // Is there any space in the current block |
There was a problem hiding this comment.
This feels like it is changing the semantics. Before it was testing whether there was room to add DATA_ALIGNMENT_SIZE, now it is testing if already full ?
There was a problem hiding this comment.
Yes.
It was filling up from DATA_ALIGNMENT_SIZE .. HEAP_ALIGNMENT_SIZE. The previous test could have equally said
(memsize_t)(nextAddr - curBlock) < HEAP_ALIGNMENT_SIZE)
Because HEAP_ALIGNMENT_SIZE is a multiple of DATA_ALIGNMENT_SIZE . (I will add a static assert to check that).
Stage 2 of this change is to move the DataBufferBottom out of the page, so I wanted to remove curBlock form the test. (Having though a little more about that I will need to change this check again to something similar, so maybe I will do that now).
|
@richardkchapman I have updated the code and responded to the comments. |
|
Automated Smoketest |
| @@ -5113,7 +5112,7 @@ class CDataBufferManager : public CInterface, implements IDataBufferManager | |||
| { | |||
| curBlock->Release(); | |||
| curBlock = NULL; | |||
| nextAddr = NULL; | |||
| nextBase = NULL; | |||
There was a problem hiding this comment.
Should you also clear nextOffset here (and above)?
There was a problem hiding this comment.
I suspect it may be important NOT to clear it - in which case a comment would be good
There was a problem hiding this comment.
I was lazy - it didn't need clearing. A comment would be good though - or clear it.
Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>
Signed-off-by: Gavin Halliday gavin.halliday@lexisnexis.com