AsyncIO compression part 1 - refactor of existing asyncio code #3021
Conversation
- Extracted asyncio code to fileio_asyncio.c/.h - Moved type definitions to fileio_types.h - Moved common macro definitions needed by both fileio.c and fileio_asyncio.c to fileio_common.h
programs/fileio_asyncio.h
Outdated
|
|
||
| /* WritePool_releaseIoJob: | ||
| * Releases an acquired job back to the pool. Doesn't execute the job. */ | ||
| void WritePool_releaseIoJob(io_job_t *job); |
There was a problem hiding this comment.
minor : this choice of symbol prefix, WritePool_, contrasts with the rest of the code base.
There's no absolute "wrong" about it, it's just a matter of coding convention consistency.
Suggested alternative prefix : WP_.
Also : it's a bit strange to have some FIO_ functions defined in different units.
In general, symbol prefix and units should be related to each other.
Maybe there is a need to pick a side here.
There was a problem hiding this comment.
Can definitely change WritePool_ to WP_ although I'd usually opt for more explicit names. Note that in the next patch I'll also introduce ReadPool_ functions. So the two options I see are to use WP_ and RP_ or AIO_writePool_ and AIO_readPool_.
As for FIO_fwriteSparse* -- I can add the same prefix to them as well, shouldn't be a problem. Note that my next PR will turn them into internal (static) functions for the fileio_asyncio module, and they will be removed from the header, so possibly just having them with no prefix is good enough.
There was a problem hiding this comment.
@Cyan4973 - Would you prefer AIO_writePool_ or WP_ (next patch would introduce AIO_readPool_ or RP_ respectively)?
My recommendation would be AIO_writePool_.
There was a problem hiding this comment.
It depends how much you value brevity.
Both look fine to me, hence AIO_writePool_ is fine.
Also, subtle thing, if WritePool is itself an object / type, then you can use AIO_WritePool_, since it refers directly to the type being manipulated (in which case, I would expect a WritePool* pointer as first argument).
programs/fileio_asyncio.h
Outdated
| ZSTD_pthread_mutex_t ioJobsMutex; | ||
| void* availableJobs[MAX_IO_JOBS]; | ||
| int availableJobsCount; | ||
| } io_pool_ctx_t; |
There was a problem hiding this comment.
A coding convention we try to follow in this code base :
- variables and function names start with a lowercase letter (excluding the PREFIX_)
- type names start with an Uppercase letter
There was a problem hiding this comment.
Thank you @Cyan4973 , I went through the codebase and have seen multiple different conventions for type names (including in fileio.c), so I am a bit confused.
Would IOPoolCtx_t be good in this case?
There was a problem hiding this comment.
Indeed, enforcing a coding style is a thankless job, and can be difficult when there are many contributions.
So, in some instances, conventions may not be properly followed.
This should not stop us from trying to converge onto a consistent set of conventions.
Would
IOPoolCtx_tbe good in this case?
Yes
|
I'm globally fine with the proposed refactoring exercise. |
|
@Cyan4973 - CR fixes done, approved to merge? |
…ter_refactor * origin/dev: AsyncIO compression part 1 - refactor of existing asyncio code (facebook#3021) cleanup double word in comment. slightly shortened status and summary lines in very verbose mode Change zstdless behavior to align with zless (facebook#2909) slightly shortened compression status update line More descriptive exclusion error; updated docs and copyright Typo (and missing commit) Suggestion from code review Python style change Fixed bugs found in other projects Updated README Test and tidy Feature parity with original shell script; needs further testing Work-in-progress; annotated types, added docs, parsed and resolved excluded files Using faster Python script to amalgamate
PR 1/2 for asyncio compression.
This PR focuses on refactoring fileio.c and extracts some functionality to fileio_asyncio.c with the purpose of creating a clearer separation between internal and external APIs.
Also has some ground works that prepares us for adding ReadPool in the next PR.