fix Flowvar allocation #33
Conversation
arnetheduck
commented
Jul 4, 2023
arnetheduck
commented
- flowvar was allocating the wrong size for the spsc channel
- refactor channel to use an ordinary type for its buffer simplifying construction / allocation, remove 256-byte limitation
- fix under-aligned allocation in channel test
* flowvar was allocating the wrong size for the spsc channel * refactor channel to use an ordinary type for its buffer simplifying construction / allocation, remove 256-byte limitation * fix under-aligned allocation in channel test
|
|
||
| createThread(threads[0], thread_func, ThreadArgs(ID: Receiver, chan: chan)) | ||
| createThread(threads[1], thread_func, ThreadArgs(ID: Sender, chan: chan)) | ||
|
|
||
| joinThread(threads[0]) | ||
| joinThread(threads[1]) | ||
|
|
||
| c_free(chan) | ||
| tp_freeAligned(chan) |
There was a problem hiding this comment.
Example code tends to be copy/pasted, so it's better to use defer to schedule the calling of this function right after it's being allocated.
There was a problem hiding this comment.
making this example safe would require some more significant refactoring, so I think it's beyond the scope of this PR (ie handling thread creation failure etc - there's just too much to fix)
| chan[].initialize(itemSize = sizeof(int)) | ||
| var chan = tp_allocAligned( | ||
| ChannelSPSCSingle[int], sizeof(ChannelSPSCSingle[int]), 64) | ||
| zeroMem(chan, sizeof(ChannelSPSCSingle[int])) |
There was a problem hiding this comment.
This patterns seems to be repeated in few places. It would be nice to introduce a simpler helper called tp_alloc0Aligned?
There was a problem hiding this comment.
I thought about that too but it would significantly grow the scope of this PR since all of the library should ideally be updated
There was a problem hiding this comment.
also, in some cases the library opts to not zero the full range of allocated bytes, so a "0" helper could not universally be applied, again pointing in the direction of a full review rather than a drive-by fix in this PR
The current version of `batchVerifyParallel` calls `syncAll` which syncs on all executing tasks. This PR changes this to syncing a Flowvar instead thus allowing `batchVerifyParallel` to be called as a task itself. Requires status-im/nim-taskpools#33
* avoid blocking batchVerifyParallel The current version of `batchVerifyParallel` calls `syncAll` which syncs on all executing tasks. This PR changes this to syncing a Flowvar instead thus allowing `batchVerifyParallel` to be called as a task itself. Requires status-im/nim-taskpools#33 * autoselect too --------- Co-authored-by: zah <zahary@gmail.com>
