On Wed, Aug 10, 2022 at 09:22:53PM -0400, Elad Lahav wrote: > Hi Paul, > > I have a few comments on chapter 5. If you agree with any of them I > can try to provide a patch. Thank you for looking this over! > 5.2.2: Not really related to counters. Is it really possible for the > compiler to use any location for temporary storage, even global > variables? That seems a bit excessive on the compiler's part. I have > definitely seen GCC reuse stack storage, but even then only when it > thought that the previous value there was out of scope (erroneously in > my case, as the function behaved like setjmp()). According to the standard, a compiler could re-use any location for temporary storage. I have not seen compilers do this aside from (as you say) stack storage, but it is quite possible that a new generation of one-penny CPUs could force further aggression in this direction. Better safe than sorry! > 5.2.3: Perhaps the code should be updated to use ISO C instead of GCC? > _Thread_local and inline are part of the language. There will probably continue to be a need to use non-standard extensions, but it seems sensible to at least try the standard facilities as they appear. Use of ISO standard inlining directives should be fine, as should things like _Thread_local, at least as long as they are as fast as are their gcc equivalents. So please do give it a try! Let's see what can be done to drag the code into the 2020s. ;-) > Listing 5.5: There is a mix of thread_id_t from CodeSamples and > pthread_create() from POSIX. One of those should be changed. Good catch! That thread_id_t should not be what is passed to pthread_create(). My guess is that changing the type of the "tid" local variable is the right way to go, but what are your thoughts? > 5.2.4: The wording suggests that the counting threads are not impacted > by the reader. But doesn't a cache line changing from Modified to > Shared incur a cost on the counter when it next comes to update the > value? We are comparing the "eventually consistent" approach with the traditional approach where each read scans all the counters, correct? If so, then the relative performance will depend on the frequency of readers in the traditional approach compared to the sleep time for the eventual() function, which in Listing 5.5 is a single millisecond. It will also depend on the rate at which each CPU does an update. So it is possible to compute breakevens. Would you like to write up something to this effect, possibly expanding on the last paragraph (starting with "This approach gives...")? > 5.3.2: "references only per-thread variables, and should not incur any > cache misses" - except that the thread can migrate to other cores and > can thus incur cache misses. Perhaps adding "in the common case" to that sentence? Or did you have something else in mind? > 5.3.3: I think that some clarification, or a simple example, is due > for explaining how a failure can occur when the count is nowhere near > the global max. That does indeed sound like a good addition. What did you have in mind? > 5.4.2: "it is worthwhile looking for algorithms with better read-side > performance" - should it not be "write-side performance"? Good catch! You are absolutely right. I look forward to seeing your patches. Thanx, Paul
