Repository navigation
Conversation
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
baaad4dd.
This review, of 0629eefc
💬 Approved with comments — Nothing here is unsafe to merge. The two findings are an INFO about inconsistent cpu:2 and feature handling across the test macros, and a QUESTION about dropping the high-cpu runner property.
Findings: 1 question · 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: ❓ 1 question · 1 info
The ThinLTO scheduling cleanup reaches only some targets, and dropping runner: high-cpu has no stated reason.
Recommendation: 💬 Optional Nits
- [QUESTION] This removes
exec_properties = {"runner": "high-cpu"}andcpu:4fromwd_cc_binaryandwd_rust_binary, including the fully optimizedlinkopts_toollinks. Is it intended that these links no longer request the high-CPU runner? (build/wd_rust_binary.bzl:137) - [INFO] Some
linkopts_defaulttargets still carry thecpu:2tag the description calls counterproductive:kj_test,wd_rust_test,wd_cc_benchmark,wd_rust_proc_macroandjsg-bench. Thewd_cc_benchmarkandwd_rust_proc_macrotest targets also don't getLINKOPTS_DEFAULT_FEATURES. The three test macros now handle this inconsistently. (build/kj_test.bzl:33)
View full review
cpu:2 and the feature are applied unevenly
The description says the cpu:2 tags are counterproductive once the native thin_lto feature is used. The tag is gone from the wd_rust_binary test target, but it is still set in these places:
build/kj_test.bzl:33, right below the line that addsLINKOPTS_DEFAULT_FEATURESbuild/wd_rust_test.bzl:39build/wd_cc_benchmark.bzl:31build/wd_rust_proc_macro.bzl:49src/rust/jsg-bench/BUILD.bazel:39
The test targets of wd_cc_benchmark and wd_rust_proc_macro also link linkopts_default, but they don't get LINKOPTS_DEFAULT_FEATURES. Downstream backend tuning won't reach them. If they are left out on purpose, a comment in build/linking.bzl would make that clear.
exec_properties = {"runner": "high-cpu"} removed
wd_cc_binary and wd_rust_binary no longer set the high-CPU runner property or the cpu:4 tag. That includes the linkopts_tool links, such as the workerd binary, which are fully optimized. This changes where those links are scheduled in remote-execution setups. The description only discusses cpu:2.
✅ All other checks passed
📝 Documentation: ✅ LGTM
The new comments in build/linking.bzl and src/rust/gen-compile-cache/BUILD.bazel describe the features as they are used, and the new features argument of wd_rust_binary is documented.
Reviewed commit: 0629eefc
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8127 +/- ##
==========================================
+ Coverage 68.31% 68.40% +0.08%
==========================================
Files 726 728 +2
Lines 189670 190456 +786
Branches 29672 29790 +118
==========================================
+ Hits 129577 130280 +703
- Misses 42719 42722 +3
- Partials 17374 17454 +80 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0629eef to
baaad4d
Compare
There was a problem hiding this comment.
Superseded by the review of
4657ec80.
This review, of baaad4dd
💬 Approved with comments — Nothing here is unsafe to merge. The earlier note about uneven cpu:2 and feature handling is fixed. What's left is a QUESTION about dropping the high-CPU runner property and an INFO about how removing cpu:2 changes test packing on remote executors.
Findings: 1 question · 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: ❓ 1 question · 1 info
The earlier note about uneven cpu:2 and feature handling is fixed. The question about the high-CPU runner is still open, and there is one INFO about test scheduling.
Recommendation: 💬 Optional Nits
- [QUESTION] This removes
exec_properties = {"runner": "high-cpu"}andcpu:4fromwd_cc_binaryandwd_rust_binary. That property applied to every action of the target, including therustccompile and the fully optimizedlinkopts_toollinks such asworkerd. Is it intended that these targets no longer request the high-CPU runner? (build/wd_rust_binary.bzl:137) - [INFO] Removing
cpu:2also removes thetest.slots = "1"property thattest_exec_properties(build/bazelflare/exec_properties.bzl:17) gives to tests with acpu:tag. Medium-size C++ and Rust tests can now share a four-vCPU executor, up to four at a time. The description doesn't mention this change in scheduling. (build/kj_test.bzl:33)
View full review
🔄 Since last review: 1 finding resolved, 1 preserved, 1 new
Fixed: uneven cpu:2 and feature handling. cpu:2 is gone from kj_test, wd_rust_test, wd_cc_benchmark, wd_rust_proc_macro and the wd_rust_binary test target. All of them now add LINKOPTS_DEFAULT_FEATURES. wd_rust_binary adds the feature only when tool is false, which matches its choice between linkopts_default and linkopts_tool. The one cpu:2 left is on the smoke-test sh_test in src/rust/jsg-bench/BUILD.bazel. That target doesn't link with linkopts_default, so it needs no change.
Still open: the high-CPU runner. See build/wd_rust_binary.bzl and build/wd_cc_binary.bzl. The removed exec_properties = {"runner": "high-cpu"} applied to every action of the target, not only the link. For wd_rust_binary that includes the rustc compile. Binaries built with linkopts_tool, such as workerd, also lose the cpu:4 tag. The description only discusses cpu:2.
Test packing on remote executors. test_exec_properties in build/bazelflare/exec_properties.bzl:17 gives test.slots = "1" to any test that has a cpu: tag. Without cpu:2, medium-size kj_test, wd_rust_test, proc-macro and rust_test targets can now share a four-vCPU executor, up to four at a time. Before, each test had an executor to itself. This may be the intended effect, but the description doesn't mention it.
✅ All other checks passed
📝 Documentation: ✅ LGTM
The new comments in build/linking.bzl and src/rust/gen-compile-cache/BUILD.bazel match how the features are used.
Reviewed commit: baaad4dd
|
Re. the LLM questions – those changes are deliberate, the high-CPU runner is no longer needed with the downstream changes. |
baaad4d to
4657ec8
Compare
There was a problem hiding this comment.
Superseded by the review of
b4c6d04b.
This review, of 4657ec80
💬 Approved with comments — Nothing here is unsafe to merge. The earlier note about uneven cpu:2 and feature handling is fixed. What's left is a QUESTION about dropping the high-CPU runner property and an INFO about how removing cpu:2 changes test packing on remote executors.
Findings: 1 question · 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: ❓ 1 question · 1 info
The earlier note about uneven cpu:2 and feature handling is fixed. The question about the high-CPU runner is still open, and there is one INFO about test scheduling.
Recommendation: 💬 Optional Nits
- [QUESTION] This removes
exec_properties = {"runner": "high-cpu"}andcpu:4fromwd_cc_binaryandwd_rust_binary. That property applied to every action of the target, including therustccompile and the fully optimizedlinkopts_toollinks such asworkerd. Is it intended that these targets no longer request the high-CPU runner? (build/wd_rust_binary.bzl:137) - [INFO] Removing
cpu:2also removes thetest.slots = "1"property thattest_exec_properties(build/bazelflare/exec_properties.bzl:17) gives to tests with acpu:tag. Medium-size C++ and Rust tests can now share a four-vCPU executor, up to four at a time. The description doesn't mention this change in scheduling. (build/kj_test.bzl:33)
View full review
🔄 Since last review: 1 finding resolved, 1 preserved, 1 new
Fixed: uneven cpu:2 and feature handling. cpu:2 is gone from kj_test, wd_rust_test, wd_cc_benchmark, wd_rust_proc_macro and the wd_rust_binary test target. All of them now add LINKOPTS_DEFAULT_FEATURES. wd_rust_binary adds the feature only when tool is false, which matches its choice between linkopts_default and linkopts_tool. The one cpu:2 left is on the smoke-test sh_test in src/rust/jsg-bench/BUILD.bazel. That target doesn't link with linkopts_default, so it needs no change.
Still open: the high-CPU runner. See build/wd_rust_binary.bzl and build/wd_cc_binary.bzl. The removed exec_properties = {"runner": "high-cpu"} applied to every action of the target, not only the link. For wd_rust_binary that includes the rustc compile. Binaries built with linkopts_tool, such as workerd, also lose the cpu:4 tag. The description only discusses cpu:2.
Test packing on remote executors. test_exec_properties in build/bazelflare/exec_properties.bzl:17 gives test.slots = "1" to any test that has a cpu: tag. Without cpu:2, medium-size kj_test, wd_rust_test, proc-macro and rust_test targets can now share a four-vCPU executor, up to four at a time. Before, each test had an executor to itself. This may be the intended effect, but the description doesn't mention it.
✅ All other checks passed
📝 Documentation: ✅ LGTM
The new comments in build/linking.bzl and src/rust/gen-compile-cache/BUILD.bazel match how the features are used.
Reviewed commit: 4657ec80 — same changes as baaad4dd, so this is that commit's review (no new review was run; line numbers are as of baaad4dd)
|
We're changing how thinLTO works downstream by using the native thin_lto feature, which means that the `cpu:2` tags will actually be counterproductive. Also add a custom feature to identify tests so we can set backend flags for them.
This shortens the critical path when building workerd since we no longer need to thinLTO-optimize V8 before we can use the compile cache
4657ec8 to
b4c6d04
Compare
There was a problem hiding this comment.
💬 Approved with comments — Nothing here is unsafe to merge. The earlier note about uneven cpu:2 and feature handling is fixed. What's left is a QUESTION about dropping the high-CPU runner property and an INFO about how removing cpu:2 changes test packing on remote executors.
Findings: 1 question · 1 info (1 is an inline comment on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: ❓ 1 question · 1 info
The earlier note about uneven cpu:2 and feature handling is fixed. The question about the high-CPU runner is still open, and there is one INFO about test scheduling.
Recommendation: 💬 Optional Nits
- [QUESTION] This removes
exec_properties = {"runner": "high-cpu"}andcpu:4fromwd_cc_binaryandwd_rust_binary. That property applied to every action of the target, including therustccompile and the fully optimizedlinkopts_toollinks such asworkerd. Is it intended that these targets no longer request the high-CPU runner? (build/wd_rust_binary.bzl:137) - [INFO] Removing
cpu:2also removes thetest.slots = "1"property thattest_exec_properties(build/bazelflare/exec_properties.bzl:17) gives to tests with acpu:tag. Medium-size C++ and Rust tests can now share a four-vCPU executor, up to four at a time. The description doesn't mention this change in scheduling. (build/kj_test.bzl:33)
View full review
🔄 Since last review: 1 finding resolved, 1 preserved, 1 new
Fixed: uneven cpu:2 and feature handling. cpu:2 is gone from kj_test, wd_rust_test, wd_cc_benchmark, wd_rust_proc_macro and the wd_rust_binary test target. All of them now add LINKOPTS_DEFAULT_FEATURES. wd_rust_binary adds the feature only when tool is false, which matches its choice between linkopts_default and linkopts_tool. The one cpu:2 left is on the smoke-test sh_test in src/rust/jsg-bench/BUILD.bazel. That target doesn't link with linkopts_default, so it needs no change.
Still open: the high-CPU runner. See build/wd_rust_binary.bzl and build/wd_cc_binary.bzl. The removed exec_properties = {"runner": "high-cpu"} applied to every action of the target, not only the link. For wd_rust_binary that includes the rustc compile. Binaries built with linkopts_tool, such as workerd, also lose the cpu:4 tag. The description only discusses cpu:2.
Test packing on remote executors. test_exec_properties in build/bazelflare/exec_properties.bzl:17 gives test.slots = "1" to any test that has a cpu: tag. Without cpu:2, medium-size kj_test, wd_rust_test, proc-macro and rust_test targets can now share a four-vCPU executor, up to four at a time. Before, each test had an executor to itself. This may be the intended effect, but the description doesn't mention it.
✅ All other checks passed
📝 Documentation: ✅ LGTM
The new comments in build/linking.bzl and src/rust/gen-compile-cache/BUILD.bazel match how the features are used.
Reviewed commit: b4c6d04b — same changes as baaad4dd, so this is that commit's review (no new review was run; line numbers are as of baaad4dd)
| # Tag with cpu:2 since this target depends on linkopts_default. | ||
| tags = tags + ["cpu:2"], | ||
| tags = tags, | ||
| target_compatible_with = target_compatible_with, |
There was a problem hiding this comment.
ℹ️ INFO · Code Quality
Removing cpu:2 also removes the test.slots = "1" property that test_exec_properties (build/bazelflare/exec_properties.bzl:17) gives to tests with a cpu: tag. Medium-size C++ and Rust tests can now share a four-vCPU executor, up to four at a time. The description doesn't mention this change in scheduling.
We're changing how thinLTO works downstream by using the native thin_lto feature, which means that the
cpu:2tags will actually be counterproductive. Also add a custom feature to identify tests so we can set backend flags for them.[build] Disable thinLTO for gen-compile-cache in downstream build
See downstream PR (to follow) for more context.