Skip to content

add --path option #20

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

add --path option #20

wants to merge 3 commits into from

Conversation

vlebourl
Copy link
Contributor

@vlebourl vlebourl commented Nov 23, 2021

Adds a --path optional command line argument to specify a path in which to look for compilers.
Possible usage:

benchmark --path=/usr/bin
benchmark --path=/usr/local/bin:/usr/bin

This allows for the testing of several different builds of the same version of compiler, for instance, clang-13 with or without LTO.

@nordlow
Copy link
Owner

nordlow commented Nov 23, 2021

Thanks. What is args.path defaulted to?

benchmark Outdated
@@ -299,6 +297,10 @@ def main():
default=1,
help='Number of runs for each compilation')

parser.add_argument('--path', dest='path', nargs='+',
default=["/usr/local/bin"],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested "/usr/local/bin" here, but I'm open for other default option!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could actually be defaulted to $PATH?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes actually more sense to me so commited a patch.

Copy link
Owner

@nordlow nordlow Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the current behavior kept if the --path is not given?

@vlebourl
Copy link
Contributor Author

vlebourl commented Nov 24, 2021

Changed default again as per your last comment suggested.
Not providing --path yields the current default behavior.
--path should be provided as the environment variable does as it relies on os.pathsep to split the path.
eg under Unix:

benchmark --path=/usr/local/bin:/usr/bin

@vlebourl
Copy link
Contributor Author

example of a use case: compiling clang in 2 stages or not, both with LTO optimization:

| Lang-uage | Temp-lated | Check Time [us/fn] | Compile Time [us/fn] | Build Time [us/fn] | Run Time [us/fn] | Check RSS [kB/fn] | Build RSS [kB/fn] | Exec Version | Exec Path 
| :-------: | ---------- | :----------------: | :------------------: | :----------------: | :--------------: | :---------------: | :---------------: | :----------: | :-------: 
| C++       | No         |   25.9 (best)      |  148.4 (1.0x)        |  169.8 (1.0x)      |   1730 (best)    | sampling error    |   13.5 (1.1x)     | 14.0.0       | ~/Dependencies/llvm-project/llvm/cmake-build-lto/bin/clang-14 
| C++       | No         |   31.3 (1.2x)      |  215.5 (1.5x)        |  244.7 (1.5x)      |   1984 (1.1x)    |    3.4 (best)     | sampling error    | 14.0.0       | ~/Dependencies/llvm-project/llvm/build/tools/clang/stage2-bins/bin/clang-13 
| C++       | Yes        |   40.7 (1.6x)      |  146.8 (1.0x)        |  176.1 (1.1x)      |   1743 (1.0x)    | sampling error    | sampling error    | 14.0.0       | ~/Dependencies/llvm-project/llvm/cmake-build-lto/bin/clang-14 
| C++       | Yes        |   51.8 (2.0x)      |  217.8 (1.5x)        |  257.9 (1.5x)      |   2089 (1.2x)    | sampling error    | sampling error    | 14.0.0       | ~/Dependencies/llvm-project/llvm/build/tools/clang/stage2-bins/bin/clang-13 

@nordlow
Copy link
Owner

nordlow commented Nov 24, 2021

Sorry for not seeing this until now but can you please move the iteration over the paths out from each benchmark_... function into into main function and add a exe_path parameter to each benchmark_... function? That's gonna make the diff much smaller in terms of indentation.

Someting like

for exe_path in args.exe_paths:
    if 'Go' in args.languages:
        if 'Check' in args.operations:
                benchmark_Go(results=results, code_paths=code_paths, args=args, op='Check', templated=False, exe_path=exe_path)
        if 'Build' in args.operations:
                benchmark_Go(results=results, code_paths=code_paths, args=args, op='Build', templated=False, exe_path=exe_path)
    # and similarly for other languages

?

While you're at it can you refactor

        if 'Check' in args.operations:
            benchmark_Swift(results=results, code_paths=code_paths, args=args, op='Check', templated=False)
        if 'Build' in args.operations:
            benchmark_Swift(results=results, code_paths=code_paths, args=args, op='Build', templated=False)

to

        for op in args.operations:
            benchmark_Swift(results=results, code_paths=code_paths, args=args, op=op, templated=False)

and have each benchmark_LANG return None if op is not supported?

@vlebourl
Copy link
Contributor Author

good point, I'll have a look!
I also wonder whether using a checksum and os.path.realpath(path) could help avoid testing several times the same exe via symlinks.

@nordlow
Copy link
Owner

nordlow commented Nov 24, 2021

good point, I'll have a look!

Thanks!

I also wonder whether using a checksum and os.path.realpath(path) could help avoid testing several times the same exe via symlinks.

Sound good to me!

@nordlow nordlow force-pushed the master branch 2 times, most recently from 9b6431f to f3b09e1 Compare August 21, 2022 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants