Jyri, Thanks a lot for your feedback. This is my first (as an individual).
My comments inline and I request some clarifications from you too. Jyri Virkki wrote: > Sivakumar Shanmugasundaram wrote: >> Enhancing Ruby with DTrace probes. > > Hi, > Some comments below... > >> This case seeks Micro Release Binding. > > 'micro' isn't really used in Solaris world, so just say Minor here. Accepted. > >> /usr/ruby/1.8/bin/ruby >> /usr/ruby/1.8/lib/libruby.so.1 > > Those files aren't being delivered by this case so no need to mention > them. The DTrace probes are part of this binary, hence my adding these lines. I will remove them, as these are not NEWly delivered. > >> /usr/ruby/1.8/lib/ruby/1.8/i386-solaris2.11/dtrace.h > > Who is the consumer for this one? > No one AFAIK. This is part of the patch and is consumed by the ruby binary itself. The Joyent's patch had this file and when you run 'dmake install', this file ends up in a sub-directory under the /usr/ruby/1.8 directory. So does other .h files from Ruby source, irrespective of whether it is consumed by others or not. The rubyinst.rb has logic, where it installs ALL (*.h) header files to the target directory. I was just being consistent with the existing Ruby ARC case, where all such .h files were being listed. In this case, dont you think this should stay? >> 2.2. Versioning >> >> The DTrace interfaces for Ruby are based on subversion trunk version >> 501. The interfaces are evolving, and it is unknown whether they will >> remain stable across releases. > > Given this info, it sounds like this is not very mature yet. Is it > yet time to call them Uncommitted interfaces? While Uncommitted is > more desirable, if they truly are so immature as the above paragraph > seems to suggest, perhaps they need to be Volatile? > We are yet to receive an official answer from Joyent on the stability. Though it is not 'mature', I am reasonably confident that it wont be changing very often to be considered. 'Volatile'. >> 3. DTrace Probe dependencies. >> >> The DTrace probes depends on the DTrace packages in Solaris SUNWdtrc >> and SUNWdtrp, Ruby packages SUNWruby18u and SUNWruby18r. > > The ruby dtrace probes will be delivered in the ruby packages, so it's > not right to say it depends on them. Accepted. Will remove SUNWruby* from the dependencies. Will leave the SUNWdtr* packages though. > >> 5. Ruby DTrace Documentation. >> The documentation for DTrace probes is available from Joyent's Ruby >> DTrace page [6]. The documentation covers the available probes, their >> arguments and usage examples. > > Since these are [will be] now in OpenSolaris, there should be some > level of documentation available without having to go to an external > site to get it. > >> The man page for Ruby will be suitably modified as well, to indicate >> the addition of DTrace probes in the interpretor. > > I suggest adding the list of probe names and the table documenting > their arguments into the manpage itself, since it is only a couple > dozen lines of info. > Accepted. I will add the probes and their descriptions here. BTW, any one knows how to add tables to man page (ruby.1) document? >> 6. Interfaces >> >> 6.1. Imported Interfaces. >> >> NAME STABILITY NOTES >> Ruby language implementation Uncommitted PSARC/2007/600 > >> SUNWruby18u Uncommitted PSARC/2007/600 >> SUNWruby18r Uncommitted PSARC/2007/600 > > What exactly is the dtrace code importing from the package names > SUNWruby18u, SUNWruby18r? The first import of the ruby language makes > sense since the dtrace project patches ruby, but the package name > imports seem unnecessary. Accepted. > >> SUNWdtrc Uncommitted PSARC/2001/466 >> SUNWdtrp Uncommitted PSARC/2001/466 > > Here I realize you probably used the package names as a shortcut for > stating the dtrace patches make use of the APIs from dtrace for adding > application probes, but that's not the best way to express an imported > interface reference. Instead, call out the actual dtrace API names(s) > being used. Accepted. The interfaces used is 'DTrace Provider API'. There are no C API's used. The patch uses a .d file with 'probe' definitions which gets compiled to a .o file. > >> 6.2. Exported Interfaces. > >> More details about the probes, their decription, the arguments can be >> found in Joyents web site [6] > > Please include the probe descriptions & arguments here (as well as in > the man page) since it is vital info to being able to do anything with > these probes. But wouldnt adding it here be a duplicate of the 'documentation' section from above? > > >> Appendix 1: Ruby man page changes >> >> FEATURES >> ... >> DTrace Probes >> Ruby has a bunch of DTrace probes that can be used to debug a running > > "a bunch of" isn't quite the formal language one expects in a man page.. ;-) > Please clean up the language a bit. > Hmm. I thought I fixed 'bunch of' :) Seriously. I had my vim crash on me and had probably recovered an older version. Will definitely fix it. >> Ruby/Rails application. NOTE: This is on x86/x64 platform only. > > Please add a section earlier in the functional spec document > explaining the reasons why there is a difference between > platforms. That's key info that needs to be documented, not just > mentioned in a footnote. > As you are aware, because of CR 6659110, the Sparc version of Ruby does not have the probes. I was hoping that this will be resolved soon, before the Indiana release, so I could fix the Sparc Ruby version too. That way, I need to just update the man page only and not change the ARC case. Please let me know if this is okay or not. Apart from the above one (x86/sparc difference), I have updated the draft. It is attached to this mail. Please review again. Regards Siva > -------------- next part -------------- An embedded and charset-unspecified text was scrubbed... Name: Ruby_Dtrace_ARC_Draft_4.txt URL: <http://mail.opensolaris.org/pipermail/webstack-discuss/attachments/20080226/4c4d70c2/attachment.txt>
