Re: [PATCH][RFC] Implement benchmark script in python

[Mike Frysinger <vapier at gentoo dot org>]

On Friday 06 December 2013 03:53:34 Siddhesh Poyarekar wrote:
> I reimplemented the benchmark code generation script in python since I
> figured it would be nicer to read and maintain.  This has the
> disadvantage of an added dependency on python - I think someone (or
> maybe me?) mentioned it as a problem in the past - but it could be an
> optional dependency only for those who want to run benchmarks.  I was
> also emboldened by Will's (really old) patch to add a python script
> that generates graphs for benchmark outputs.

i have no problem utilizing python in any place that does not impact:
	tar xf glibc.tar.bz2 && cd glibc && ./configure && make && make check

for larger scripts, python is certainly easier to maintain

that said, this is the first python we've imported, so i think we need to nail 
down some general python things before we allow it.  thus proposed:
	- use PEP8 style.  that means no tabs for indentation.
	- use PEP257 for docstrings.  here's the preferred format:
	"""One line description.

	Longer details go here on multiple lines.

	Args:
	  arg: description of it

	Returns:
	  Describe the return value.

	Raises:
	  Any random exceptions that might be raised.
	"""
	- require python-2.7, but be compatible with python-3.2+
	- use "from __future__ import print_function"
	- use printf strings rather than concatenation:
		bad: print('blah' + var + 'foo')
		good: print('blah%sfoo' % var)
	- prefer strings use single quotes rather than double quotes
	- use a main() func
	- global scope code is heavily discouraged
	- globals are heavily discouraged
	- let's add a pylintrc file and require all code to pass `pylint` with it

> #!/usr/bin/env python

personally, i'd be fine with /usr/bin/python.  are systems so unusual that they 
put it elsewhere that matters ?

> # Copyright (C) 2013 Free Software Foundation, Inc.
> # This file is part of the GNU C Library.
> 
> # The GNU C Library is free software; you can redistribute it and/or

having one continuous comment block is easier to read -- so have a # inbetween 
these sections rather than just a blank line

> import sys
> 
> all_vals = {}
> # Valid directives.
> directives = {"name": "",
> 	      "args": [],
> 	      "includes": [],
> 	      "include-sources": [],
> 	      "ret": ""}
> 
> # Generate the C source for the function from the values and directives.
> def gen_source(func):

use a docstring rather than comments above the func

>     # Print macros.  This branches out to a separate routine if
>     # the function takes arguments.
>     if not directives["args"]:
> 	print "#define CALL_BENCH_FUNC(v, i) %s ()" % func
> 	print "#define NUM_VARIANTS (1)"
> 	print "#define NUM_SAMPLES(v) (1)"
> 	print "#define VARIANT(v) FUNCNAME \"()\""

i wonder if we should pull these out into constant strings rather than 
inlining them all.  it's easier to reason/tweak large string blocks than 
multiple prints.

DEFINE_SPEW = """
#define CALL_BENCH_FUNC(v, i) %(func)s ()
#define NUM_VARIANTS (1)
#define NUM_SAMPLES(v) (1)
#define VARIANT(v) FUNCNAME "()"
"""
...
	print(DEFINE_SPEW % {'func': func})

> 	outargs = []
>     else:
> 	outargs = _print_arg_data (func)

no space before the (

>     for k in all_vals.keys():
> 	vals = all_vals[k]
> 	out = map (lambda v: "{%s}," % v, vals)

just use a list comprehension:
	out = ['{%s},' % x for x in vals]

> 	# Members for the variants structure list that we will
> 	# print later.
> 	variants.append("{\"%s(%s)\", %d, in%d}," % (func, k, len(vals), n))
> 
> 	print "struct args in%d[%d] = {" % (n, len(vals))
> 	print '\n'.join(out)
> 	print "};\n"
> 	n = n + 1

use zip() & itertools:
	for k, n in zip(all_vals.keys(), itertools.count()):

>     except (IndexError, KeyError):
> 	print >> sys.stderr, "Invalid directive:", ':'.join(d)
> 	sys.exit(1)

you use this form a couple of times.  suggest adding a helper like die() to 
write the string to stderr and call exit.

> def parse_file(func):
>     global all_vals
>     try:
> 	file_handle = open(func + "-inputs", "r")

use a with directive:
	with open('%s-inputs' % func) as f:
		for line in f:
			line = line.rstrip()
			...

>     for line in file_handle:
> 	line = line.strip()

guessing you meant .rstrip() here and not .strip() ?

> 	# Look for directives and parse it if found.
> 	if line.startswith("##"):
> 	    _parse_directive(line[2:].split(':', 1))
> 
> 	# Skip blank lines and comments.
> 	if not line or line[0] == '#':
> 	    continue

what about inline comments ?  or are those not supported ?
	line = line.split('#', 1)[0].rstrip()
	if not line:
		# it's a comment or blank

> 	# Otherwise, we're an input.  Add to the appropriate input set.
> 	cur_name = directives["name"]
> 	try:
> 	    all_vals[cur_name].append(line)
> 	except KeyError:
> 	    all_vals[cur_name] = []
> 	    all_vals[cur_name].append(line)

this would be simpler:
	all_vals.setdefault(cur_name, [])
	all_vals[cur_name].append(line)

> if __name__ == '__main__':
>     if not sys.argv[1:]:
> 	print "Usage: ", sys.argv[0], " <function>"
> 	sys.exit(1)
> 
>     parse_file(sys.argv[1])
>     gen_source(sys.argv[1])

this should be in a main func.

def main(args):
	if len(args) != 1:
		print('Usage: %s <function>' % sys.argv[0])

	parse_file(args[0])
	gen_source(args[0])


if __name__ == '__main__':
	sys.exit(main(sys.argv[1:]))
-mike

Attachment: signature.asc
Description: This is a digitally signed message part.