David.Comay at Sun.COM wrote: > > * Webrev over all non-AST files (this includes the files in > > usr/src/cmd/ast/msgcc/ by accident): > > http://www.nrubsig.org/people/gisburn/work/solaris/ksh93_integration/ksh93_integration_prototype005_webrev_20070606/non_ast_files/webrev/ > > Here are my comments for round "three": > > usr/src/cmd/ksh/Makefile.com > > Lines 101-109 - As I indicated in an earlier review, I don't > believe this is necessary. Both Nevada and the Solaris 10 > patch gate do large pages automatically (or so-called out of > the box) and so including these options is unnecessary. > However, I've cc'ed Bart Smaalders who is an expert in this > area who can suggest whether or not it makes sense to include > this.
I disagree... the comment explicitly cites the benchmarks (mhhh... I hoped the current comment was enougth... should I add the whole benchmark code and all the results as comment there (which raises the question if there is a size limit for comment sections in Makefiles...) ? =:-) ). And neither on my Ultra5 (B48, B51) or our university machines (E280R/S10U2, V880/S10U1, Blade1000/B51) 64k pages are allocated by default (unless the code uses the matching compiler options to do it) for stack or heap, even if the usage grows over many continous pages. AFAIK there are more agressive settings for kernels on Niagara1 machines but they do not apply to the machines listed above where it makes sense to use 64k pages for ksh93, too. Additionally the code itself was modified and optimized for larger page sizes (see comment about "64k pages for stack", the code (like ksh93's libcmd) now uses significantly larger chunks of stack instead of heap+global vars) and further optimisations are queued for future putbacks. > usr/src/lib/libc/port/regex/wordexp.c > > Lines 70, 80, 331-332, 343-344, 384-385, 404-405, various other > lines in wordexp() - It appears there's some sort of mismerge > with Roger's > > PSARC 2006/659 fork extensions > 6497356 fork extensions Yes, but AFAIK April merged the (current) ksh88 version back (in her SCCS tree). For the ksh93 version I've talked with Roger Faulkner... his new version uses |posix_spawn()|&co. (which is IMO a very good idea) but it's very late now to port these parts to the ksh93 version of |libc::wordexp()| ... I only stumbled over the new ksh88 version of |libc::wordexp()| at the end of May and getting the new version created&&_tested_ will take some time (creating a new patch is no problem... getting it tested will be another xx@@@!!-story) ... and pushing a new version without a good testing coverage by all OpenSolaris distributions for such a very risky part is IMO not a good idea (remember we have this alternative |libc::wordexp()| version in the tree because otherwise SMF may blow-up and the OpenSolaris distributions had problems to deal with the issue... and that's why I am now dragging this pain with me...). The options are: 1. Leave the current ksh93 version unmodified in the tree 2. Create a new version which is in "feature-sync" with Roger's putback (e.g. use |posix_spawn()|&co.) 3. Remove the whole |libc::wordexp()| machinery and let the OpenSolaris distributions continue their LD_PRELOAD&co. hacks to patch around this problem Roger and I agreed to pick [1] for now and do [2] after the putback, get it tested by everyone involved and then putback ASAP (and if I fail to come up with a new version Roger may take over). This approach makes sure we have a version in the tree which won't cause unexpected problems. And IMO [3] is not a valid option (well, it's just listed here for completeness). > usr/src/lib/libshell/misc/buildksh93.ksh > > If we're going to putback this file, then I would suggest the > following changes: > > Line 169 - Instead of "Roland or April", I would provide a > pointer to the OpenSolaris project page > > http://opensolaris.org/os/project/ksh93-integration/ > > as individuals come and go, but the project should live on. Fixed. > Line 170 - s/weired/weird/ Fixed. > Lines 195-197 and 214 - See my above comments for > usr/src/cmd/ksh/Makefile.com concerning large pages. See my replies above... :-) > usr/src/pkgdefs/SUNWarc/prototype_com > usr/src/pkgdefs/SUNWarc/prototype_i386 > usr/src/pkgdefs/SUNWarc/prototype_sparc > > I know what's happened to libcmd as part of this project but > why are you no longer delivering the lint libraries via these > three files (especially since you're updating llib-lcmd > itself?) The Solaris libcmd API was moved to libc and therefore llib-lc takes over the duties for the |def*()|-API and the ksh93 parts of libcmd are currently not a public API, therefore we don't deliver libcmd.so and llib-lcmd ... ;-( > usr/src/pkgdefs/SUNWastdev/prototype_com > > Lines 46-57 - Please sort the list by pathname (column 3). Fixed. > usr/src/pkgdefs/SUNWcsl/prototype_com > > Line 77 - Could you please move this entry after the "libaio" > one to maintain the sorted order? Fixed. > usr/src/pkgdefs/SUNWcsl/prototype_i386 > > Line 228 - Could you please move this entry after the "libaio" > one to maintain the sorted order? Fixed. > usr/src/pkgdefs/SUNWcsl/prototype_sparc > > Line 217 - Could you please move this entry after the "libaio" > one to maintain the sorted order? Fixed. > usr/src/tools/findunref/exception_list > > Within each block, could you please sort the pathnames? Fixed (for the parts we own, not the whole file). ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.mainz at nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 7950090 (;O/ \/ \O;)
