Fixed CORE-5611 - Higher memory consumption for prepared statements.#114
Fixed CORE-5611 - Higher memory consumption for prepared statements.#114asfernandes merged 11 commits intomasterfrom
Conversation
asfernandes
commented
Oct 3, 2017
asfernandes
commented
- Remove some fields from nodes, including the pool reference and list of children nodes
- Organize some fields for better alignment and possible reduction on node size
- Delete DSQL scratch pool for DML statements
asfernandes
changed the title
|
Can someone please review? |
|
I'm reviewing it right now. |
| Parser* parser = FB_NEW_POOL(*scratchPool) Parser(tdbb, *scratchPool, scratch, clientDialect, | ||
| scratch->getAttachment()->dbb_db_SQL_dialect, text, textLength, | ||
| tdbb->getAttachment()->att_charset); | ||
|
|
There was a problem hiding this comment.
What is the point in allocating the parser instance dynamically?
| att_pools.add(pool); | ||
| return pool; | ||
| } | ||
|
|
There was a problem hiding this comment.
Why is the parent-based new pool not accounted in att_pools? It's still a proper pool and must be destroyed via deletePool() in ~Attachment() if not deleted explicitly.
There was a problem hiding this comment.
When the scratch pool is manually deleted (DML statement compiled without error) the thing works with or without registering it in att_pools, as deletePool will be called and remove it.
But for the non-DML case or DML with exception in compilation, that means only the statement pool is explicitly destroyed (scratch is child of statement). In this case, it will try to destroy an already deallocated pool in detach.
Is there any real problem in deleting a child pool via its parent?
There was a problem hiding this comment.
After some tests I see that current usage (let child pool die with parent) does not decrement child pool's memory statistics when parent dies.
I think parent pools should maintain a list of its active children and when parent is destroyed it should decrement statistics from children.
There was a problem hiding this comment.
@asfernandes Does child pool unmap memory got from OS when parent is destroyed? As far as I remember we always used to explicitly destroy child pools before parent.
There was a problem hiding this comment.
Is there any real problem in deleting a child pool via its parent?
AFAIK, it just never happened in our code, so I cannot answer for sure. "Parent" concept was used to redirect allocations up to some amount, but the child pool may have its own memory blocks that the parent is not aware of (and thus cannot deallocate).
There was a problem hiding this comment.
In other words, "delete by pool" concept does not apply to child pools. And I'm not sure it should - it would complicate things without any real gain.
There was a problem hiding this comment.
Then how should you prefer? Making the scratch pool independent (not as child of statement) or use parent-child implementing some logic related to att_pools to never delete child after parent?
There was a problem hiding this comment.
When a child pool is going to allocate large amounts of memory, are these blocks requested from parent or are mapped directly with the OS?
They are mapped directly from OS.
There was a problem hiding this comment.
I'd prefer to make the scratch pool independent but please ensure that it dies when necessary (prepare failure?).
|
@dyemanov is it ok to merge in master now? |
|
I don't have other complaints, so it can be merged. Let's wait a bit before considering a backport into v3 though. |
…de for better alignment.
To make it work, change MAKE_parameter to allocate parameters in the message pool (statement pool).
Children lists will be created and destroyed on demand.
Added a scope block to delete it before the scratch pool is destroyed in DsqlDmlRequest::dsqlPass.
CORE-5646 - Parse error when compiling a statement causes memory leak until attachment is disconnected.
asfernandes
force-pushed
the
work/nodes-memory-core-5611
branch
from
513580f to
5f5a869
Compare
|
@asfernandes , can we get back to this issue and backport into v3? |
I'm rebasing and completing the old patch. |
3 participants
