Bulk Load CDK: Unit tests for memory manager #45091
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
| } | ||
| usedMemoryBytes.addAndGet(memoryBytes) | ||
| while (usedMemoryBytes.get() + memoryBytes > availableMemoryBytes) { | ||
| releaseUpdates.receive() |
There was a problem hiding this comment.
if memoryBytes > availableMemoryBytes, we just wait forever, no?
There was a problem hiding this comment.
this isn't memory safe. 2 threads could allocate more memory than is available.
There was a problem hiding this comment.
I added a throw and a test for it
There was a problem hiding this comment.
also, as soon as I typed if, copilot knew what I was doing :)
| class MemoryManager { | ||
| private val availableMemoryBytes: Long = Runtime.getRuntime().maxMemory() | ||
| class MemoryManager(availableMemoryProvider: AvailableMemoryProvider) { | ||
| private val availableMemoryBytes: Long = availableMemoryProvider.availableMemoryBytes |
There was a problem hiding this comment.
really should be totalMemoryBytes. Once they're reserved, they're not really available anymore
| } | ||
|
|
||
| @Test | ||
| fun testReserveBlocking() = runTest { |
There was a problem hiding this comment.
we need tests imvolving several threads
There was a problem hiding this comment.
Yep, it took a lot but I did finally flush out the bug you were afraid of. See testReserveBlockingMultithreaded()
There was a problem hiding this comment.
took a lot of efforts, or a lot of jobs?
There was a problem hiding this comment.
A lot of jobs to get one error.
There was a problem hiding this comment.
yeah, I usually test those by having some king of test-triggered breakpoint in the production code. As much as I dislike having test-only code in production codepath, anything else is always going to be very flaky or involve a lot of jobs/threads/workers...
johnny-schmidt
force-pushed
the
issue-9615/load-cdk-unit-tests-memory-manager
branch
from
73f4ed4 to
a7de516
Compare
johnny-schmidt
force-pushed
the
issue-9615/load-cdk-unit-tests-memory-manager
branch
from
a7de516 to
b382d98
Compare
| @@ -17,31 +19,49 @@ import kotlin.concurrent.withLock | |||
| * TODO: Some degree of logging/monitoring around how accurate we're actually being? | |||
There was a problem hiding this comment.
nit: add a TODO: Be more fair. The current implementation will guarantee a first-come first-served order, which means if a function requests a large amount of memory that's not available yet, any subsequent call will block until the large memory request is satisfied, even though there may be enough available memory to serve the smaller requests
| val reserved = AtomicBoolean(false) | ||
|
|
||
| try { | ||
| withTimeout(5000) { memoryManager.reserveBlocking(900) } |
There was a problem hiding this comment.
nit: can we use a lock (or a channel) instead of a timeout here?
There was a problem hiding this comment.
I'm testing that it doesn't block, so I'm not sure what else I can do.
| } | ||
|
|
||
| @Test | ||
| fun testReserveBlocking() = runTest { |
There was a problem hiding this comment.
took a lot of efforts, or a lot of jobs?
| withContext(Dispatchers.IO) { | ||
| memoryManager.reserveBlocking(1000) | ||
| Assertions.assertEquals(0, memoryManager.remainingMemoryBytes) | ||
| val nIterations = 100000 |
There was a problem hiding this comment.
I guess a lot of jobs...
There was a problem hiding this comment.
Yeah, 10,000 wasn't enough to fail consistently. 100,000 and I'd always hit it at least once.
| } | ||
|
|
||
| mutex.withLock { | ||
| while (usedMemoryBytes.get() + memoryBytes > totalMemoryBytes) { |
There was a problem hiding this comment.
yeah, I'm pretty sure this is correct.
Just as an interesting case, we could also do without the AtomicLong and a volatile would suffice :)
Thank you for fixing it.
johnny-schmidt
deleted the
issue-9615/load-cdk-unit-tests-memory-manager
branch
What
AvailableMemoryProviderfor testing