David.Comay at Sun.COM wrote:
>
> >> 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
>
> Please no - there's is of course no reason to add such verbiage to the
> comments
Erm... did you see the smiley ? "=:-)" means something like "little
devil"...:-)
> I am curious when (against which build) did you do your benchmarks and
> what sort of workload they consistent of?
B51, B61 and now B65 (thanks to Al Hopper and Tanja Pradetch for
coming-up with a larger range of benchmark machines) on Ultra5,
Blade1000, E280R, SF68k and SF69k. Someone from Fujitsu is going to test
the stuff on their range of machines, too.
Lets look at two benchmarks:
1. The main workload which benefits from using largepages on the _heap_
are scripts which work on large data stored in variables (e.g. arrays),
for example:
-- snip --
#!/usr/bin/ksh93
# simple bubblesort
# don't blame me about the quality - it was written at 3:00am and there
is no
# gurantee that this thing even works
function compare_string
{
typeset str1="$1"
typeset str2="$2"
integer i1=0
integer i2=0
[[ "${str1}" = "${str2}" ]] && { print "0" ; return 0 ; }
# this is horrible inefficient
while [[ "${str1:i1:1}" == "${str2:i2:1}" ]] &&
(( i1 < ${#str1} && i2 < ${#str2} )) ; do
(( i1++ , i2++ ))
done
(( '${str1:i1:1}' > '${str2:i2:1}' )) && print -- "-1" || print "1"
return 0
}
function bubblesort
{
nameref ar=$1 # data array
typeset cmp=$2 # compare function
# fixme: $3 should contain a "swap" function
typeset swapped="true"
integer i
typeset tmp
while ${swapped} ; do
swapped="false"
for (( i=0 ; i < (${#ar[*]}-1) ; i++ )) ; do
if (( $(${cmp} "${ar[i]}" "${ar[i+1]}") == -1 )) ; then
# swap
tmp="${ar[i]}"
ar[i]="${ar[i+1]}"
ar[i+1]="${tmp}"
swapped="true"
fi
done
done
}
typeset -a data
# build a very large string
largestring="largestring"
for ((i=0 ; i < 10 ; i++)) ; do
largestring+="${largestring}"
done
# fill array
for (( i=12 ; i > 0 ; i-- )) ; do
data+=( "${largestring}${i}" )
done
#print "# Sorting ${#largestring}"
bubblesort data compare_string
# print elemets, one per line
#(IFS=$'\n' ; print -- "${data[*]}")
-- snip --
Results on a Blade1000 look like this:
-- snip --
$ env - LC_ALL=C timex ppgsz -o stack=65536,heap=65536,anon=65536
/usr/bin/ksh93 bb.ksh
real 2:44.76
user 2:42.21
sys 0.17
$ env - LC_ALL=C timex ppgsz -o stack=8192,heap=8192,anon=8192
/usr/bin/ksh93 bb.ksh
real 2:47.12
user 2:44.91
sys 0.16
-- snip --
The difference isn't huge but remeber that the UltraSPARC-3 CPU used in
the Blade1000 (UltraSPARC-2 shows a better improvement with almost six
seconds) can't put the 64k pages into the large page tables (at least
with the current MMU code in Solaris) and has to use one of the 16 slots
of the small fully-associaive set...
... based on these results I think it's better to remove the
"-xpagesize_heap=65536" flag since it may be better to allow the
"LargePage Out Of the Box" code to pick a propper pagesize for heap+anon
memory.
2. Main workload which benefits from mapping the stack with 64k pages
are builtin commands which put all their temporary data on the stack
(instead of using global data) and scripts which use recursive
algorithms like this (this doesn't use any builtin commands from libcmd
because most of them include file I/O which makes the benchmarks
slightly more tricky):
-- snip --
$ (time env - LC_ALL=C
~/ksh93/ast_ksh_20070515/build_normal_8k_stack/arch/sol11.sun4/bin/ksh
-c 'for ((j=0 ; j < 5000 ; j++ )) ; do integer i=0 ; function incnum {
(( i++ , i < 900 )) && incnum ; } ; incnum ; done' )
real 2m4.183s
user 2m2.896s
sys 0m0.119s
$ (time env - LC_ALL=C
~/ksh93/ast_ksh_20070515/build_normal_64k_stack/arch/sol11.sun4/bin/ksh
-c 'for ((j=0 ; j < 5000 ; j++ )) ; do integer i=0 ; function incnum {
(( i++ , i < 900 )) && incnum ; } ; incnum ; done' )
real 1m59.337s
user 1m58.109s
sys 0m0.123s
-- snip --
As you can see there is a four second difference (for a _simple_ test
which doesn't really do anything with variables or builtin commands) by
just mapping the stack with 64k pages (the result is from a Blade1000
and testing the same on larger machine shows the same improvements) and
this alone justifies the -xpagesize_stack=65536 on all SPARC platform
(not lloking at the problem that the "LargePage Our Of the Box"-feature
doesn't appear to handle cases like "hit&&run" usage of the shell
correctly)
Anyway... I am going to remove the -xpagesize_* options completely for
now since this discussion may only hold up the review (and we're running
out of time) and can be added again with the next putback (but I am
still unhappy about the removal of -xpagesize_stack=65536 since it shows
a significant improvement on _all_ tested machines (exclusing Fujitsu
SPARC64-based machines for which I don't have any feedback yet) ... and
as described earlier the "Largepage Out Of the Box" feature will not
work in this case... ;-( ).
> >> 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...).
>
> But I'm talking about the code that is *not* under #if WORDEXP_KSH93.
> Unless I'm misreading the webrev, the *existing* wordexp() was
> mismerged with Roger's putback.
That file wasn't merged... I stomped over the code like a mad elephant.
Quick look at the merge session shows that I just copied it over because
the diff didn't apply (which was no suprise since the code changed) and
assuming that the diff generation I "silenced" the problem with a plain
copy. Grumpf (more coffee and a closer look may have saved us from the
whole "wordexp is out of sync"-issue... ;-( ) ... ;-(
... anyway... I fixed the problem with
http://bugs.grommit.com/show_bug.cgi?id=266 ("RFE: Implement changes per
David Comay's review comments") and re-merged the ksh93 version into the
new code and started the work on the |posix_spawn()|-based version of
the ksh93-|libc::wordexp()| in the case that someone feels it's
neccesary to make such a thing mandatory for this putback (note: I don't
like that idea because the current version has very good test
coverage... IMHO it's better keep the current version for this putback
and commit the more new (more risky) version with a seperate
putback...).
> >> 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 ... ;-(
>
> So then why are you updating llib-lcmd at all? Why not just remove
> it? Some potential future hope that it will be made public?
Yes... AFAIK some of these commands may be opened soon (one possible
consumer may be standalone command versions of these builtins, for
example take a closer look how "alias.sh" in closed-bins work... IMO it
may be nice to replace some of those consumers to use native code
instead of starting a full shell session to execute one command
(/usr/bin/test and /usr/bin/sleep are currently the top canidates for
such a change and some others may follow...)).
----
Bye,
Roland
--
__ . . __
(o.\ \/ /.o) roland.mainz at nrubsig.org
\__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer
/O /==\ O\ TEL +49 641 7950090
(;O/ \/ \O;)
