-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cilkscale documentation #148
Conversation
✅ Deploy Preview for sage-licorice-6da44d ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
f0fcabe
to
77f6382
Compare
I believe this is at a point that it could really benefit from an extra set of eyes. Anyone willing to review? @behoppe @timkaler @cleiserson @neboat |
@ailiop I think this an outstanding starting point. For now I will stick to very high-level feedback. I recommend reconsidering the arrangement of content with an eye to several different goals that seem to be in play:
IMO "how to use Cilkscale" is the top goal, and so we want to prioritize every arrangement of content in service to this goal. I think this means pushing Understanding work, span, and parallelism is intertwined with how to use Cilkscale. No way around that. But still, I recommend we establish other places that we can leverage for this understanding. For example, "What the #$!@# is parallelism" will be a useful blog post (with a new title). Maybe also a tutorial on work-span analysis? And the glossary of course. This user's guide article needs a clear sense of partnership with those (not-yet-ready) parts of the website, so that here we focus on how to use Cilkscale. On a more detailed stylistic note, I like how the "fine grained analysis" section breaks code into little pieces that are interspersed with commentary. I recommend more of that. For example, instead of one big block of bash that forces me to parse and scroll up and down etc... $ /opt/opencilk/bin/clang++ qsort.cpp -fopencilk -fcilktool=cilkscale -O3 -o qsort_cilkscale
$ ./qsort_cilkscale 100000000
Sorting 100000000 random integers
Sort succeeded
Time(sample_qsort) = 2.0279 sec
tag,work (seconds),span (seconds),parallelism,burdened_span (seconds),burdened_parallelism
,24.2875,2.96416,8.19373,2.96449,8.19283 We might break it up and exclude things that need no further commentary. $ /opt/opencilk/bin/clang++ qsort.cpp -fopencilk -fcilktool=cilkscale -O3 -o qsort_cilkscale
$ ./qsort_cilkscale 100000000 (Commentary here about two new lines before resuming the block of code...) tag,work (seconds),span (seconds),parallelism,burdened_span (seconds),burdened_parallelism
,24.2875,2.96416,8.19373,2.96449,8.19283 The goal is to allow the reader to parse the story as he goes, without having to back up and do comparisons between two code blocks that are not even showing simultaneously on the screen. Finally, I see code indents set at 4. We are wanting a standard for this. I personally advocate for 2, or the smallest possible tab we can all agree on. My perspective is that this code is not for editing. It is for display in locations that are sometimes very space-constrained. Wide tabs are an extravagant use of that precious space IMO. (See the homepage for an example.) I hope this is helpful. Great job! |
PS: I also think that showing the Cilkscale plots near the top (and/or in a tutorial) would help, saving the part about how to create the plots for the end. |
@behoppe Thank you for your comments! I pushed a set of updates to address (most of) them. |
c1b1474
to
8e15182
Compare
I have added a new page for Cilkscale in the Reference category. While I have not yet made any updates to the User's guide or Tutorial components of the overall Cilkscale documentation, I think the new Cilkscale reference manual page is at a good place for review. There are 4 places where I think the Cilkscale reference page needs some changes, either in content or in related website infrastructure. These are marked in "TODO" or "BUG" alert boxes in the page; I also summarize them below:
|
@behoppe The Netlify deployment checks are currently failing, but I think the failures are spurious. Let me explain:
Is it possible to somehow "refresh" the checks engine? If not — or if it's not easy anyway — I am happy to close this PR and create a new one, which I think will fix the problem. |
@ailiop it's easy to refresh the checks engine, and I just did so, and I got another failure. I think I agree with your assessment. I've seen this problem recently, and it had to do with outdated Node packages (specifically prismjs), which are not normally updated with a site build. So I "cleared cache" and redeployed, and the error I see is something about ".eleventy.js". I will look into this after lunch. |
@ailiop I am investigating per https://docs.netlify.com/configure-builds/troubleshooting-tips/. This PR/branch builds fine in my local system, but my local system is using older packages than Netlify. So I am going to update my local system and check again. |
@ailiop the deploy preview is built now. After I updated Node and npm on my local system, I set environment variables on Netlify to fix the same versions I observed locally: NODE_VERSION 16.17.0
NPM_VERSION 8.15.0 Then I "cleared cache" and redeployed, and it worked. |
Thank you @behoppe! I can now see the new Cilkscale reference page in the deploy preview. The checks still come up as "failed" from my end on the GitHub PR page but I won't worry about that. |
Mainly to focus more on the "how-to" aspect and refer to the Reference manual when needed.
- Remove TODO notes - Use publicly accessible URLs for Cilkscale papers - Small edits for clarity
@behoppe I have finished a pass over the Cilkscale documentation and split it into two parts: a user's guide and a reference manual. There is some tutorial-like content at the end of the user's guide (small section on reasoning about scalability). I think this would work better in a separate tutorial page but I don't have time to work on one at the moment. Until I or anyone else gets to that, I think it's still helpful to have something in the User's guide that briefly address the "what next?" question that follows naturally after one gets a bunch of results with Cilkscale. Anyway, this is ready for review! |
@behoppe There are a couple of styling issues in the Cilkscale reference manual page (preview):
Perhaps related to (1), alert boxes seem to have some of their own styling rules. Another notable difference is that there is no extra vertical space between paragraphs in alert boxes. I am hoping these issues are things you could fix, hopefully with little effort. |
Thanks @ailiop. This looks great. Sorry for my delayed review. I will use multiple comments. After reviewing the user's guide, I tested the instructions, and I could not get
|
@behoppe I believe that this is a spurious printout that does not actually correspond to a compilation error. That is, if you look at the files in your directory after compiling with This is a known issue with OpenCilk 2.0 and Cilk/C++ (OpenCilk/opencilk-project#129), and has been resolved with OpenCilk 2.0.1. I think if you either install OpenCilk 2.0.1 or simply ignore these printouts you should be able to go through the Cilkscale user's guide. |
@ailiop thanks for your formatting comments. The headers are an easy fix, which I included with my latest push 6f92f27 to this branch. I am less sure about fixing the jumbled shell-code that's inside On to the more substantive comments on the user's guide. (I have not yet reviewed the reference, except its title, as noted below.) This is really coming along!
I am torn about this line:
I have not yet made it to the plots and "insights" (very last section). |
@behoppe Thank you for these comments! I will go over them in more detail tomorrow, but let me offer a quick reply to some of them in the meantime:
I can see how that might be confusing. I think it'd be clearer if a similar comment was moved towards the end of the Cilkscale API section and in a Note box. I can play around with that tomorrow.
"Cilkscale reference guide" is actually the name I used initially but thought it has too much overlap with "Cilkscale user's guide". Maybe then it's better to just say "Cilkscale reference page"?
I'll have to think some more about why the text gave rise to this confusion. The notion of "benchmarking" is generally the same throughout: it refers to timing a (sub-)computation. The
In other words, the script does not produce any new analysis/benchmarking measurements that were not covered before. But it makes it way more convenient to collect and view results. |
- Minor edits in initial sections for clarity - Remove some distracting comments - Rephrase discussion on Cilkscale Python script to avoid ambiguity on what "benchmarking" means - Regenerate final Cilkscale table and plots, and update "insights" accordingly
@behoppe I made some revisions to the user's guide and a couple of minor tweaks to the reference page. My revisions touch most parts of the user's guide but are more substantial in the section that addresses the I also noticed that the Cilkscale plots were inconsistent with the tabular reports (I think the plots were actually obtained with a different base-case size) so I regenerated them and edited the tutorial-like epilogue accordingly. Incidentally, I generally like the section title changes you suggested. In the case of the last section, I felt like "Insights" was vague and attempted an alternative: "Discussion: diagnosing performance limitations". My goal was two-fold: (1) make it clear that this section different from the rest (a discussion as opposed to a how-to), and (2) clarify the specific task that is discussed. I am not sure I would want to stick with the current title, but I feel it mostly accomplishes these goals. Alternative suggestions are welcome! |
- Off-white axes foreground in plots - Dotted grid lines in plots - Correction in CLI-formatted CSV table output - Consistent plot sizes
@ailiop I reviewed the reference and -- pending my edits fcbcc0d and some thoughts on "processors" -- I think it's ready to publish. Great job.
Processors |
WHen the Cilkscale plots were updated to change the plot face color and add grid lines, an older-version legend was used inadvertently. This commit uses the correct version of `cilkscale.py` and updates all work/span analysis reports and corresponding discussion for consistency.
@behoppe Thank you. I like your edits and can certainly see your point regarding the terminology for processor/core/worker, especially in the context of the I made an attempt to clarify what I also noticed that I had messed up the Cilkscale visualization plotter base version when I changed the plot face color and added grid lines. I regenerated the plots with the correct (up-to-date) version and updated the reports and related discussion for consistency. |
Thanks, @ailiop. Looks great. I made very minor adjustments to image size and punctuation. Merging with main branch! |
This is an initial (and currently partial) draft of a User's guide page for Cilkscale.