Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11377144:
add and enable validator tests for x86 (Closed)
|
Created:
8 years, 1 month ago by Robert Muth (chromium) Modified:
8 years, 1 month ago CC:
native-client-reviews_googlegroups.com Visibility:
Public. |
DescriptionEnable validator speed tests for x86.
These are driven by an archived tarball containing
various nexes including spec and translator nexes.
A script for updating the tarball is provided as well.
Also, some script cleanup - e.g. added "readonly"
attribute.
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=10304
Patch Set 1 #Patch Set 2 : #
Total comments: 5
Patch Set 3 : #
Total comments: 32
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Messages
Total messages: 14 (0 generated)
PTAL @Jan: What is the best way to make the timing output digestable by our graph plotting infrastructure.
https://chromiumcodereview.appspot.com/11377144/diff/2001/buildbot/buildbot_s... File buildbot/buildbot_spec2k.sh (right): https://chromiumcodereview.appspot.com/11377144/diff/2001/buildbot/buildbot_s... buildbot/buildbot_spec2k.sh:161: rm -rf CannedNexes Indentation is a bit inconsistent in these couple of functions. What generates the giant_nexe.tar.bz2? https://chromiumcodereview.appspot.com/11377144/diff/2001/buildbot/buildbot_s... buildbot/buildbot_spec2k.sh:193: /usr//bin/time ${validator} $b extra slash /usr//bin https://chromiumcodereview.appspot.com/11377144/diff/2001/buildbot/buildbot_s... buildbot/buildbot_spec2k.sh:222: ./run_all.sh TimeValidation SetupPnaclTranslatorArmOptHW "${TRYBOT_TESTS}" ||\ Can you merge the measure-validator-speed mechanism with this ./run_all.sh TimeValidation function ? In the end, you need to be able to print "RESULT ... " TimeValidation() uses the script tests/spec2k/emit_perf_log.sh to do that printing.
As discussed in person we will move the validator tests out of the spec harness which is getting a little bloated anyway. Instead the canned nexe archive also contains spec binaries and translator binaries. I added a script to update these. https://chromiumcodereview.appspot.com/11377144/diff/2001/buildbot/buildbot_s... File buildbot/buildbot_spec2k.sh (right): https://chromiumcodereview.appspot.com/11377144/diff/2001/buildbot/buildbot_s... buildbot/buildbot_spec2k.sh:161: rm -rf CannedNexes On 2012/11/14 18:45:30, jvoung wrote: > Indentation is a bit inconsistent in these couple of functions. > > What generates the giant_nexe.tar.bz2? Done. https://chromiumcodereview.appspot.com/11377144/diff/2001/buildbot/buildbot_s... buildbot/buildbot_spec2k.sh:193: /usr//bin/time ${validator} $b On 2012/11/14 18:45:30, jvoung wrote: > extra slash > /usr//bin Done.
http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.sh File buildbot/buildbot_spec2k.sh (right): http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:161: ${UP_DOWN_LOAD} DownloadArchivedNexes ${CANNED_NEXE_REV} \ Did you want to reuse tools/canned_nexe_tool.sh instead? http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:164: rm -rf CannedNexes indentation http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:183: echo "RESULT $1_$2: $3= $(bc) secs" What does the invocation of bc do here? Oh... I see from below. Why don't you compute the value within LogTimedRun, then just pass the value into LogTimeHelper instead of splitting the calculation up between the two functions? http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:202: echo "@@@BUILD_STEP validator speed test@@@" maybe print the ${arch} too inside the BUILD_STEP http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:210: validator="${QEMU_TOOL} ${validator}" Do we really want to run this in qemu, or can we do this on the hardware bot only, as "pnacl-trybot-arm-hw" does? Maybe add the step to pnacl-arm-hw too. http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:217: echo "next" why not echo "validating ${nexe}" at the beginning, instead of echo'ing "next" http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh File tools/canned_nexe_tool.sh (right): http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:8: #@ update those it can. Can you clarify in what cases will it not update? Known nexes that it will update are the spec ones and the pnacl translator ones? http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:47: # This generates "CannedNexes/" in the current directory Maybe clarify that "This generates" means untarring giant_nexe.tar.bz2 will generate ... (vs the rm -rf =P ) http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:61: # This generates "CannedNexes/" in the current directory Stale comment? http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:107: ./run_all.sh BuildBenchmarks 0 ${setup} train I don't think you need to specify train vs ref vs test while Building. http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:111: cp tests/spec2k/*/*.${fext} ${CANNED_DIR} extra space http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:112: rename 's/[.](arm|x8664|x8632)$//' ${CANNED_DIR}/*.${fext} Is it required to chop off the extension? If so, can you document what requires that?
http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.sh File buildbot/buildbot_spec2k.sh (right): http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:171: echo "$(pwd)/scons-out/opt-linux-x86-32/staging/ncval" Also, would the dfa-validator devs be interested in using this? I think they had some scripts to run benchmarks offline, but not actually get them benchmarks tracked on the bots.
+khim PTAL http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.sh File buildbot/buildbot_spec2k.sh (right): http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:164: rm -rf CannedNexes On 2012/11/15 19:10:48, jvoung (chromium) wrote: > indentation Done. http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:171: echo "$(pwd)/scons-out/opt-linux-x86-32/staging/ncval" absolutely, will send email to khim about it On 2012/11/15 19:14:19, jvoung (chromium) wrote: > Also, would the dfa-validator devs be interested in using this? I think they > had some scripts to run benchmarks offline, but not actually get them benchmarks > tracked on the bots. http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:183: echo "RESULT $1_$2: $3= $(bc) secs" I eliminated the mess with the temp files. so the output of /usr/bin/time is piped straight to LogTimeHelper that means that LogTimedRun does not see the output of "time". On 2012/11/15 19:10:48, jvoung (chromium) wrote: > What does the invocation of bc do here? > Oh... I see from below. > > Why don't you compute the value within LogTimedRun, then just pass the value > into LogTimeHelper instead of splitting the calculation up between the two > functions? > http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:202: echo "@@@BUILD_STEP validator speed test@@@" On 2012/11/15 19:10:48, jvoung (chromium) wrote: > maybe print the ${arch} too inside the BUILD_STEP Done. http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:210: validator="${QEMU_TOOL} ${validator}" The qemu part is useful when I manually tests this. I definitely want to enable this for arm-hw but I have some feeling it might break or could be flake. So, I first want to get it in for x86 and make sure it works there which is more predictable. On 2012/11/15 19:10:48, jvoung (chromium) wrote: > Do we really want to run this in qemu, or can we do this on the hardware bot > only, as "pnacl-trybot-arm-hw" does? > > Maybe add the step to pnacl-arm-hw too. http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:217: echo "next" that was debugging leftovers - remvoed http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh File tools/canned_nexe_tool.sh (right): http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:8: #@ update those it can. On 2012/11/15 19:10:48, jvoung (chromium) wrote: > Can you clarify in what cases will it not update? > > Known nexes that it will update are the spec ones and the pnacl translator ones? Done. http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:47: # This generates "CannedNexes/" in the current directory On 2012/11/15 19:10:48, jvoung (chromium) wrote: > Maybe clarify that "This generates" means untarring giant_nexe.tar.bz2 will > generate ... > > (vs the rm -rf =P ) Done. http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:47: # This generates "CannedNexes/" in the current directory On 2012/11/15 19:10:48, jvoung (chromium) wrote: > Maybe clarify that "This generates" means untarring giant_nexe.tar.bz2 will > generate ... > > (vs the rm -rf =P ) Done. http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:61: # This generates "CannedNexes/" in the current directory On 2012/11/15 19:10:48, jvoung (chromium) wrote: > Stale comment? Done. http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:107: ./run_all.sh BuildBenchmarks 0 ${setup} train On 2012/11/15 19:10:48, jvoung (chromium) wrote: > I don't think you need to specify train vs ref vs test while Building. Done. http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:111: cp tests/spec2k/*/*.${fext} ${CANNED_DIR} On 2012/11/15 19:10:48, jvoung (chromium) wrote: > extra space Done. http://codereview.chromium.org/11377144/diff/11002/tools/canned_nexe_tool.sh#... tools/canned_nexe_tool.sh:112: rename 's/[.](arm|x8664|x8632)$//' ${CANNED_DIR}/*.${fext} On 2012/11/15 19:10:48, jvoung (chromium) wrote: > Is it required to chop off the extension? If so, can you document what requires > that? Done.
http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.sh File buildbot/buildbot_spec2k.sh (right): http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:195: --output >(LogTimeHelper ${graph} ${benchmark} ${variant}) \ Does --output need an argument? http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:199: measure-validator-speed() { Perhaps add a cross reference to the canned-nexe tool to indicate how to regenerate the nexes if validation rules change.
http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.sh File buildbot/buildbot_spec2k.sh (right): http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:257: run-tests SetupPnaclTranslatorX8632Opt "${TRYBOT_TRANSLATOR_TESTS}" 0 1 Perhaps add it to the trybot runs too, so that we can see what the output would have been. It's safe in that it won't create superfluous graphs or tracked data points, since the trybots don't have the tracking enabled.
PTAL http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.sh File buildbot/buildbot_spec2k.sh (right): http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:195: --output >(LogTimeHelper ${graph} ${benchmark} ${variant}) \ added a comment On 2012/11/15 21:00:43, jvoung (chromium) wrote: > Does --output need an argument? http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:199: measure-validator-speed() { I added a comment before: readonly CANNED_NEXE_REV=1001 On 2012/11/15 21:00:43, jvoung (chromium) wrote: > Perhaps add a cross reference to the canned-nexe tool to indicate how to > regenerate the nexes if validation rules change. http://codereview.chromium.org/11377144/diff/11002/buildbot/buildbot_spec2k.s... buildbot/buildbot_spec2k.sh:257: run-tests SetupPnaclTranslatorX8632Opt "${TRYBOT_TRANSLATOR_TESTS}" 0 1 On 2012/11/15 21:02:04, jvoung (chromium) wrote: > Perhaps add it to the trybot runs too, so that we can see what the output would > have been. It's safe in that it won't create superfluous graphs or tracked data > points, since the trybots don't have the tracking enabled. Done. for x86
lgtm
CQ is trying da patch. Follow status at https://nativeclient-status.appspot.com/cq/robertm@chromium.org/11377144/27007
Try job failure for 11377144-27007 (retry) on nacl-lucid_64-newlib-x86_64-pnacl-spec for steps "annotate, validator speed test [x86-64], summary". It's a second try, previously, steps "annotate, validator speed test [x86-64], summary" failed. http://build.chromium.org/p/tryserver.nacl/buildstatus?builder=nacl-lucid_64-...
CQ is trying da patch. Follow status at https://nativeclient-status.appspot.com/cq/robertm@chromium.org/11377144/35011
Change committed as 10304 |
