Repository navigation
Improve readability of C EventPipe #49664
Description
Great writeup! Any further suggestions for improving the readability of the library is highly welcomed.
Sure, I think my main challenges were (1) picking out the small portions of identifiers that were the important distinctive part within otherwise very long names, and (2) the abstractions that made typical datastructures and operations like array, pointer, indexing or derefencing more opaque.
Suggestions:
- Now that type names show up in functions there is probably good benefit to using more compact naming choices. For example perhaps thread_session_state -> tsession
- Converging on a single definition of growable array and linked list rather than using wrapper abstractions. (dictionary too, but maybe at lower priority)
- Where possible avoid using verbose iterator patterns and instead use simple C operators. For example growable array could expose the a T* and a length to use in a for loop with incrementing index variable. Linked list could expose a node with T* next field that can be dereferenced and it is null at the end.
- Reconsider if we are getting enough value from the getter/setter function pattern vs. directly accessing the fields? If we want a visually distinctive indicator of public/private to catch accidental misuse, perhaps a compact naming convention such as 'all private fields start with _' would suffice?
Originally posted by @noahfalk in #49542 (comment)
Reply:
I need to put focus on Mono runtime Diagnostic features at the moment, but could make a pass after that looking into some changes to compact names.
For arrays we already have a len and data functions that can be used directly to iterate arrays. Guess we could complement for similar things for list, but still keep abstraction not exposing the underlying implementations.
We could probably do other things to compact code when using iterators, like having a for loop macro that will take care of begin/end, next, value so you just code the inner parts of the loop having access to the value on each iteration, but no need to write the boiler plait code over and over again.
Getter/Setter have some nice features and capabilities, we have introduce them in Mono as well in some places, but I believe the main issue is increase length in identifiers. If we can make struct member names more compact (but they still need to be clear and understandable) that will of course have positive side effects of the rest of the code. A parallel option could be to change how the getter and setters are used to simplify reading. Instead of:
ep_sequence_point_get_thread_sequence_numbers_ref (instance)
we could do:
ep_sequence_point_get_ref (instance, thread_sequence_numbers)
and compacting some identifiers we could get:
ep_seq_point_get_ref (instance, tseq_nums)
That will turn the getter into a function first taking struct instance as first parameter and field as second, but still keep the logic of getter (and inline function, or direct struct member access under the hood). Personally I think that increase readability a lot since you can just look at the last part to detect operator and the last parameter to see the member.
Alternative:
ep_get_ref (seq_point, instance, tseq_nums)
I believe setters could be done something like this:
ep_seq_point_set (instance, tseq_nums) = 2
Alternative:
ep_set (seq_point, instance, tseq_nums) = 2
Originally written by @lateralusX
Activity
josalem commentedon Mar 15, 2021
I created a separate issue to consolidate this conversation and get it off a closed PR 😄.
My 2 cents: I don't think we should bother with attempting to fix the macros that mask the C++ and C differences, e.g., the getter/setter pairs. This library is written in C now, so I'd rather get rid of the complexities of shimming between two feature sets. Specifically, I don't see ArrayLists, Lists, or HashMaps as key differentiators between the coreclr and Mono, so it seems fruitless to try and shim these container types now that we've shifted to only using the C implementation. What are peoples' thoughts on just writing up a set of basic container classes that live in
src/nativesomewhere and are usable throughout the shared code between Mono and CoreCLR?jkotas commentedon Mar 15, 2021
+1. I have made similar comment in one of the EventPipe rewrite reviews. I think it can even start as something specific to eventpipe, and generalized later as needed.
lambdageek commentedon Mar 15, 2021
src/mono/mono/eglibdoes not depend on the rest of mono, is pure C. A lot of it is not relevant, but, for example,GSList,GList,GHashTable,GPtrArray, etc could be shared. (I think their only dependency on the rest of eglib isg_malloc/g_free.) We would need to break up the monolithic glib.h header, but that would be a good thing, IMO.josalem commentedon Mar 15, 2021
That sounds like a good starting point, @lambdageek.
alloc/freeare already shimmed in the EventPipe library, so it should be pretty easy to just use the container classes and not worry about runtime dependencies. We should keep that in mind whenever we decide to do this work.lateralusX commentedon Mar 16, 2021
Current EventPipe shim for Mono runtime already uses eglib for all container type implementations, so if we standardize on that, we have something that's already been well tested together with EventPipe library, but if we are going to break them out that will come with some refactoring as @lambdageek pointed out and we will need a way to associate custom allocators as well as maybe name adjustments unless we think the eglib naming will be something to preserve in generalized container library shared between runtimes. Another way could be to take needed container implementations from eglib, split it up into separate headers and source files under native, adjust naming and allocators and then let libraries like EventPipe use those sources in its library build.
Shorten the getter/setters that I suggested above would also address the issue with long names, simplify ability to spot important relevant parts when names get longer, I believe that would greatly improve readability, but need to take it for a test ride in some compact code to see how well it plays out.
lateralusX commentedon Oct 11, 2022
@lambdageek, @noahfalk, @davmason @tommcdon, @jkotas, @LakshanF, @AaronRobinsonMSFT:
I'm thinking about rebooting this initiative, sharing a set of eglib container types used in EventPipe with all runtimes. That would remove all the container abstractions from EventPipe shim API and simplify port of EventPipe to more runtimes. It is also a "blocker" doing other kinds of enhancements to the library at the moment so if we are OK pursuing that, I would start up some prototype work coming weeks.
AaronRobinsonMSFT commentedon Oct 11, 2022
I think there is value in doing this in the long term. I'm not privy to all the other possible enhancements that EventPipe might benefit from so not in a position to say this is higher priority or not. Is there a collection of possible work items in this space?
lateralusX commentedon Oct 12, 2022
In all discussions we had so far related to the potential work as described in this issue, leads back to breaking out the containers as its starting point. There are other things described in this issue that could be done first, but that is mainly internal "cosmetic" changes to EventPipe library and won't have any direct impact on initiatives taking the library to new runtimes/platforms, so from that perspective, breaking out the containers would probably be the work item having the most impact.
lateralusX commentedon Apr 11, 2023
Containers are now shared, src/native/containers and used by EventPipe, simplify the runtime shim as well as usage patterns through EventPipe.
noahfalk commentedon Apr 11, 2023
Thanks @lateralusX! That was a very nice step 👍