[benchmark] Naming Convention #20334
Conversation
New benchmark naming convention for better readability and improved naming system that accounts for performance coverage growth going forward.
|
Please review: @eeckstein, @gottesmm, @atrick, @airspeedswift, @lorentey, @dabrahams |
Extend parser to support benchmark names that include `-` in names, as proposed in PR swiftlang#20334.
The extended `Flatten` test family is based on recently added benchmarks `FlattenLostLoop` and `FlattenListFlatMap`. They had unnecessarily large base workloads, which prevented more precise measurement. Their base workload was lowered by a factor of 20. See discussion on swiftlang#20116 (comment) Since these are recent additions to the Swift Benchmark Suite, I’m removing the originals and reintroducing them under new names `Flatten.Array.Tuple4.flatMap` and `Flatten.Array.Tuple4.for-in.Reserve`without going through the `legacyFactor`. Based on these two templates, this commit introduces thorough performance test coverage of the related space including: * method chain `map.joined` * naive for-in implementation without `reserveCapacity` * few Unsafe variants that should serve as aspirational targets for ideally optimized code * lazy variants * variants for different underlying types: 4 element Array, struct and class in addition to the original 4 element tuple * variants that flatten Sequence instead of Array The tests follow naming convention proposed in swiftlang#20334
Allow for running of test matching the naming convention proposed in swiftlang#20334.
Applying new Benchmark Naming Convention swiftlang#20334
|
I'm staying out of this at this point because I don't use the benchmarks nearly as much as the people who more frequently work in the stdlib or on optimization passes, but I still think clarity is more important than brevity and that inventing non-standard acronyms like "OCB" is just going to confuse people. |
|
I have to agree that the proposed short names here are essentially unreadable to me compared to the existing names. They may make sense to someone who reads these regularly but since many more people will be reading only the few benchmark names that are affected by their changes these would be equivalent to showing the first 40 characters of the SHA1 hash of a benchmark. |
|
@jrose-apple, @dabrahams @xwu, I'm not arguing for the return of C-style naming! Don't let the allergic reaction to sins past cloud your judgment, please...
Assuming you're comparing the Ignoring
I've argued that case before:
But I think you are all highlighting a real issue here: making the benchmark reports more actionable for someone without intimate knowledge of the Swift Benchmark Suite. Or as Xiaodi put it:
Which, to me, actually hints at a better solution… Benchmark names alone, even though they contain a pinch of semantic hints about their purpose, are not enough to take reasonable action when they show up in a report on GitHub. Lifting length restrictions on benchmark names doesn't solve that problem. Benchmark names — like the commit hashes — are just unique identifiers. You need to consult benchmark's actual source code anyway. That's what needs to be made easier! Hyperlinking is what makes SHA1 hashes usable on GitHub. I can extend Limiting benchmark names to fixed length greatly eases creation of existing and all future tooling for visualizing our performance. I have several ideas, especially for relative comparison of variants and groups inside a single family. |
|
@palimondo Thanks for doing this. I've never understood the argument for absurdly long camel case names. When you have many of these similar names scattered throughout the data set, it becomes impossible for me to visually distinguish them from each other, and I often get them mixed up. Brevity increases clarity. Using namespaces and separators also helps a lot. You're right, the purpose of these names is simply to have unique benchmark IDs. The reason we don't use numbers is that it also helps to have something memorable. They should to easy to pick out in a large data set. They need to work with all manner of data formatting tools. I often manually elide large names just so I can more easily work with the data table. People seem to be arguing that the name should indicate the purpose of the benchmark, which is just ridiulous. You know what would actually help to quickly communicate the purpose of a benchmark: benchmark descriptions and comments in the benchmark! No one says we can't output the short description in whatever web view people are arguing about. nit: "OCB" is a rather bizarre choice given that "ObjC" is an accepted abbreviation. Why does "Bridge" need to be in the name at all? It can be in the description! |
There's nothing religious about the choices I made… Considering the longest name Are there any other fundamental objections to the proposed naming convention? |
Removed the controversial mention of C-style name prefixes.
|
Also, any feedback on the idea to provide links to source code from benchmark reports? Implementation detailsWhile the idea with extending |
|
@lorentey @gottesmm I've heard specific concerns or suggestions from you. Do you have any suggestions for proceeding with this PR? I like the use of a namespace separator that this PR introduces. It doesn't force anyone to use any particular abbreviation. I think the side discussion about coding conventions was way off the mark. I think we can all agree that the current naming convention does not provide sufficient clarity as it is. I'm strongly in favor of adding free-form one line descriptions for clarity. Does anyone think that needs to be done before landing this PR? |
|
Allowing periods as separators is definitely an improvement over the status quo. Adding free-form descriptions is a good idea, too. |
|
@atrick I am fine with the namespace separator. I think having something like that makes sense and provides a good way to slice/dice results. That being said I do object to this PR in its current form since I still do not understand why the 40 character limit is the only way to solve the problem that Pavol is trying to solve. Specifically: a. The argument around HTML tables is a non-problem since we can always put the test names on the right hand side and that problem is gone. Then one will have: | DATA_COL | DATA_COL | DATA_COL | TEST_NAME | b. Visualizations. AFAIKT this is the other argument for the 40 character limit. I do not buy that the 40 character limit is the only solution for this. Why couldn't we instead limit the size of the individual namespaces (perhaps to 10-15 characters) but allow for the user to have as many namespaces as they want. If one looks at the visualization that @palimondo posted above the important thing is the shortness of the namespace names... not the total length. If that suffices then having the limit on the individual namespaces IMO would be a more flexible solution and would stop the wastage of engineering time by people trying to compress names to fit within 40 characters. Before the naming convention lands, I would at least like my concerns here answered. Why is it that the 40 character limit is the only solution here? That being said I would be ok with the test renames here landing, as long as the naming convention document and enforcement of the naming convention is removed from the PR. |
|
As an aside, I also think in a follow on commit it would be useful to:
|
|
@gottesmm I don't think this has much to do with HTML. It has to do with a variety of text-based tools, git comments, terminals, org-table (my favorite), chart labels, and with data visualization in general. I won't block a PR because the name is too long. It's just my personal opinion that 40 characters is already way too long as an ID if you also have a description string that can be used for the HTML output. A designated namespace separator allows anyone to work with the namespaces programatically. The way the benchmark author specifies that is simply by inserting the character in a string. Are you thinking it should be tied to the filename/classname somehow? |
|
Andy and I spoke about this offline, I withdraw my objections. |
|
I don't think there are any objections to the content of this PR itself. Note that the 40 character name limit was already there. It's not being introduced here. We haven't fully resolved how to print benchmark descriptions--I can see a couple a viable options and opinions on it are split--but that doesn't need to be discussed in this PR. |
Matched the part of the ISO standard used for language code to match one used on https://www.unicode.org/udhr/translations.html (I plan to use the Universal Declaration of Human Rights as corpus for string testing.)
|
@swift-ci please test |
|
Thank you @atrick and all who participated in the review to make this proposal better! 🤝 |
Renamed tests according to the naming convention proposed in PR swiftlang#20334. They now fit the 40 character limit and are structured into groups and variants. Also extracted tags into 2 constants, to simplify the [BenchmarkInfo] expression.
Extend parser to support benchmark names that include `-?!` in names, to fully support the new Naming Convention from PR swiftlang#20334.
After short discussion with @lorentey about benchmark names and my work on re-enabling
ExistentialPerformancebenchmarks and looking at how to shorten the names ofObjectiveCBridge*benchmarks in the future, I'm proposing a new benchmark naming convention for better readability and improved naming system that accounts for performance coverage growth going forward.The PR was updates following a lengthy conversation during review, see comments, to clarify use of abbreviations.
To create more cohesive and well structured system, names of newly added benchmarks should meet the following set of requirements:
UpperCamelCase. For a family of benchmarks, separate the name components with periods.UpperCase, methods arelowerCase.Technically, the benchmark's name must match the following regular expression:
[A-Z][a-zA-Z0-9\-.!?]+More details with examples are in the Naming.md document.