Skip to content
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

Create self-contained cxx_zig_toolchain #22

Closed
wants to merge 169 commits into from
Closed

Conversation

aherrmann
Copy link
Contributor

Closes #20

Adds prelude//toolchains/cxx/zig with a C/C++ toolchain based on zig cc.
The doc-string of the added module contains a description and examples
configurations.

Also adds an example to the repository that builds a C++ library and a binary
depending on that library using this toolchain.

Note, for that example to work in the open source buck2 tree the following additional patch needs to be applied:

diff --git a/.buckconfig b/.buckconfig
deleted file mode 100644
index c32af92..0000000
--- a/.buckconfig
+++ /dev/null
@@ -1,19 +0,0 @@
-[repositories]
-root = .
-prelude = prelude
-ovr_config = prelude
-shim = shim
-toolchains = shim
-fbcode = shim
-fbcode_macros = shim
-fbsource = shim
-buck = shim
-
-[buildfile]
-name = TARGETS
-
-[build]
-execution_platforms = ovr_config//platforms:default
-
-[parser]
-target_platform_detector_spec = target://...->ovr_config//platforms:default
diff --git a/prelude/cxx/tools/TARGETS.v2 b/prelude/cxx/tools/TARGETS.v2
index 2030d2f..5db1689 100644
--- a/prelude/cxx/tools/TARGETS.v2
+++ b/prelude/cxx/tools/TARGETS.v2
@@ -1,4 +1,3 @@
-load("@fbcode_macros//build_defs/lib:python_common.bzl", "get_ldflags", "get_strip_mode")
 load(":defs.bzl", "cxx_hacks", "omnibus_environment")
 
 prelude = native
@@ -538,7 +537,7 @@ omnibus_environment(
     # We override the binary-level ldflags with library level ldfalgs for
     # shared roots. We include 2 important things: stripping binaries, and not
     # discarding GPU code.
-    shared_root_ld_flags = get_ldflags("", "", "omnibus_root", get_strip_mode("", ""), "") + [
+    shared_root_ld_flags = [
         "-Wl,--no-discard-section=.nv_fatbin",
         "-Wl,--no-discard-section=.nvFatBinSegment",
         # Reorder nv_fatbin after the bss section to avoid overflow

facebook-github-bot and others added 30 commits October 1, 2022 09:33
fbshipit-source-id: 9b5b2afb7ddc43bc922046453ffbf9bae0ad791e
Summary: Prohibit path `c:\\foo\..` as we do on Unix.

Reviewed By: ndmitchell

Differential Revision: D39951419

fbshipit-source-id: 49c3ece23d76c280634b6b02ef32f15b591869ea
Reviewed By: ndmitchell

Differential Revision: D39966539

fbshipit-source-id: 285e7a1d5c40696c402d7f8ade5c22f0bb9c4987
Summary: The genrule is much simpler with less weird bits, so we should use it where possible. Closer to what Buck itself knows about.

Reviewed By: stepancheg

Differential Revision: D39998148

fbshipit-source-id: f3eda76afbe9273e90e78e11f87f1c408e9c9016
Summary:
From https://fb.workplace.com/groups/930797200910874/posts/1086039248720001/?comment_id=1088687688455157&reply_comment_id=1089185148405411

Created from CodeHub with https://fburl.com/edit-in-codehub

Reviewed By: jwmcglynn

Differential Revision: D40000640

fbshipit-source-id: 50d543bcbaab10d591ab9c9add12d3c94647058d
Summary:
It prevents making `StarlarkValue` `Sized` which is needed to be able to add associated constants to `StarlarkValue`.

Associated constants are helpful because they can be accessed at compile time, unlike functions.

This can be helpful in changes like D36305002 for example, where short type name is placed in associated constant.

Also `str` is not really a `StarlarkValue` (it cannot be heap allocated).

Also with another implementation it is harder to verify that `str` implementation is consistent with `StarlarkStr` (because `StarlarkStr` caches the hash, and `str` does not).

If we need to use certain operations of `StarlarkStr` on `str`, we extract them into a separate functions.

Reviewed By: ndmitchell

Differential Revision: D36579300

fbshipit-source-id: 7f06c0a672d6d35d460729c8a6cf81be53230dd4
Summary: Previously we continued with the types until later in the process, then cleared them when compiling the def at runtime. Now we use the flag to wipe them out at expr_for_type. Slightly less to do at runtime, but mostly simpler.

Reviewed By: stepancheg

Differential Revision: D39978884

fbshipit-source-id: c2adddc363e45d3e8be4cb81ce10245cbeb504c0
Reviewed By: stepancheg

Differential Revision: D39978883

fbshipit-source-id: 1da82f144e968750445653a326aa407172d38268
Summary: Follow the same rules as to where types can go based on Python3.

Reviewed By: stepancheg

Differential Revision: D39978882

fbshipit-source-id: a9c264cef82d83e5b5b7ba66caf8dd2e91fd0a0f
Summary: Makes defining macros that repoint it easier. Also, consistency is good.

Reviewed By: stepancheg

Differential Revision: D40000513

fbshipit-source-id: 9f7af000bd743fc5d38cfbbddaf4fbfc04c9d162
Summary: If we want to swap out macros, they had better point into a cell, so do that with all our load statements out the test suite. Not harmful, and slightly more explicit, which is good anyway.

Reviewed By: stepancheg

Differential Revision: D40000514

fbshipit-source-id: 5337fef7015376d902617e15cf138321b83fc678
Summary: If we want to self-host Buck2, we should ignore buck-out in this directory.

Reviewed By: stepancheg

Differential Revision: D40000512

fbshipit-source-id: 3e2fa015d0b0108bf85765df939a52f11bb84327
Summary: Following diff D39973562 adds dice metrics to snapshot.

Reviewed By: ndmitchell

Differential Revision: D39973564

fbshipit-source-id: 828c48aa38833d252e9f2032a251d56349fa864e
Summary: Context: investigating why `buck2` hangs. Metrics might help.

Reviewed By: shonaganuma

Differential Revision: D39973562

fbshipit-source-id: 862c242e2629d7a7764863c27af61c3b4a5febe5
Reviewed By: ndmitchell

Differential Revision: D36579299

fbshipit-source-id: c5509e685dc00c4e49549e0ac50fda3440345edc
Summary: `get_type` function to `TYPE` associated constants.

Reviewed By: ndmitchell

Differential Revision: D36579848

fbshipit-source-id: 00402e310dccd5408270e3ea8b42a0b6abc550b5
Summary:
This stack enables unconditional compilation on Windows and removes `eden_materializer` feature.

The issue with Windows is:
* hard to do correctly conditional compilation markers like `#[cfg(all(feature = "..."), not(unix), fbcode_build)]`
* compilation errors on windows-only or on non-windows because of conditional compilation
* not being able to fully typecheck code locally when working on non-windows or on windows

So this stack enables compilation of Eden client on Windows keeping it disabled at runtime.

There are issues with features:
* it's hard to configure them correctly, e.g. it's easy to configure equally in `TARGETS` and in `Cargo.toml` (following diff D39915795 has an example)
* we don't check if code is compiled internally without features
* features do not work in opensource version (because of dependencies on Meta internal crates)

So this diff removes most features replacing them with `oss-enable` markers.

Reviewed By: ndmitchell

Differential Revision: D39916185

fbshipit-source-id: fb2bc6d0f0f43a05054d0c36a96612e86819c36d
Summary: Our coding standards suggest explicit return. Also clean up some error messages slightly.

Reviewed By: stepancheg

Differential Revision: D40002722

fbshipit-source-id: e25fd20c5ba943268b8cdd9829c6122135a919b7
Summary: Gives better errors than std::fs. Can also simplify things.

Reviewed By: stepancheg

Differential Revision: D40002723

fbshipit-source-id: 16f85cde216dfaa98ece2e9db1547d315a19cb15
Summary: Better style.

Reviewed By: stepancheg

Differential Revision: D40002725

fbshipit-source-id: 2d41bea9ce20b356fc412061b4514fd03f06e61d
Summary: We don't use a leading @ anywhere else, so don't do it here. One day it would be great to get rid of the @ entirely since it's just noise.

Reviewed By: stepancheg

Differential Revision: D40006192

fbshipit-source-id: f656ee96d691db3afde33e27562c7af160dc8fa1
Summary: Following diff D40006138 uses new function in place where the file is written

Reviewed By: ndmitchell

Differential Revision: D40006137

fbshipit-source-id: 210068cd18420813d4cfd1c5c69ae3875f1ca90c
Reviewed By: ndmitchell

Differential Revision: D40006138

fbshipit-source-id: 3e3f8f1e5929d0509e7180b4bbcaa42285272483
Summary: Split long function for readability.

Reviewed By: ndmitchell

Differential Revision: D40006318

fbshipit-source-id: 86f0e4ed975683efa52dc44e82699e14a9afffe1
Reviewed By: ndmitchell

Differential Revision: D40006334

fbshipit-source-id: ce8abce0f7af1641d446af67cd20175ba2d14832
Reviewed By: ndmitchell

Differential Revision: D40006548

fbshipit-source-id: aa7c18f75e253512950330553a4e7d43e959801f
Reviewed By: ndmitchell

Differential Revision: D40006549

fbshipit-source-id: dfa5b6f2ea73a521417d6adeff40da087b58a8fc
Summary: Take `PathBuf` instead of `String` for consistency with `AbsPathBuf::new`.

Reviewed By: ndmitchell

Differential Revision: D39966802

fbshipit-source-id: 3cc82dd4f99d422a9613545309bddbd540a57d41
Reviewed By: ndmitchell

Differential Revision: D39914657

fbshipit-source-id: 7a53159d53bd28ed31798931d0d0d084716de1c8
Reviewed By: ndmitchell

Differential Revision: D39914730

fbshipit-source-id: a67e9989540407ee29502b1f47b6a256aed0f384
themarwhal and others added 18 commits October 6, 2022 05:50
…esolved

Summary:
This change causes an 1GB increase in allocated memory during analysis for target `fb4a`. I was not expecting this, so revert it back until I understand what's happening here.

Repro command: buck2 kill && buck2 audit providers fb4a > /tmp/a && buck2 debug allocator-stats | jq . | grep allocated

Reviewed By: IanChilds

Differential Revision: D40140074

fbshipit-source-id: f192133e513a8ae6ff6f6df6c61dc30836515d3d
Summary: This bumps allocated memory during analysis, revert it back until I understand why.

Reviewed By: IanChilds

Differential Revision: D40140093

fbshipit-source-id: 05aac306e1c30ff6530cd98125577c12dac6c233
Summary:
Adds support for `tar.xz` archives to `http_archive`. The user needs to set the attribute `type = "tar.xz"` to make use of this.

X-link: facebook/buck2-prelude#3

Reviewed By: arlyon

Differential Revision: D40138919

Pulled By: arlyon

fbshipit-source-id: 3cbecdbde522ec5fc56acee4c75cb6caf62c8e27
Summary:
`enum DiceTaskStateForDebugging` describes possible states of dice currently running task.

Following diff D40129237 prints it in CSV.

Reviewed By: ndmitchell

Differential Revision: D40129240

fbshipit-source-id: f15319a2e90c0d9ba8f2b3c000e20ed773bd9ee4
Summary: Following diff D40129237 outputs it in TSV.

Reviewed By: ndmitchell

Differential Revision: D40129239

fbshipit-source-id: 55bacba13138fbac488f8819a7c24370991cdea3
Summary:
Output looks like this:

```
6832    3       AsyncDropped    InterpreterResultsKey   InterpreterResults(ovr_config//platform/linux)
2187    3       AsyncDropped    InterpreterResultsKey   InterpreterResults(ovr_config//cpu/x86/constraints)
2597    3       AsyncDropped    InterpreterResultsKey   InterpreterResults(fbsource//xplat/toolchains/bumps/facebook-dt-13.x)
5568    3       AsyncDropped    InterpreterResultsKey   InterpreterResults(ovr_config//platform/windows)
9673    3       AsyncDropped    InterpreterResultsKey   InterpreterResults(fbsource//xplat/buck2/platform/cache_mode)
6154    3       AsyncDropped    InterpreterResultsKey   InterpreterResults(ovr_config//cuda/constraints)
8343    3       AsyncDropped    InterpreterResultsKey   InterpreterResults(ovr_config//toolchain/xcode/constraints)
9198    3       AsyncDropped    InterpreterResultsKey   InterpreterResults(ovr_config//toolchain/wasm/constraints)
```

May be helpful investigating why buck2 hangs.

Reviewed By: ndmitchell

Differential Revision: D40129237

fbshipit-source-id: be010dc19323bc0bb2e07a416eb538a43883afa3
Summary:
We have separate key type information, e.g. when printing TSV:

```
27412   3       AsyncDropped    ReadDirKey      ReadDir(fbsource//third-party/rust/vendor/webpki-roots-0.22.2/src)
```

So including key type in `Display` is redundant.

Also this is not what `Display` usually does.

Reviewed By: ndmitchell

Differential Revision: D40130116

fbshipit-source-id: 8fd917d10d73344288e9543de15779ba8654c499
Summary: Was not easy to find.

Reviewed By: IanChilds

Differential Revision: D40130593

fbshipit-source-id: e6906de9b6bb451f129c404cbbaa96bf697195a6
Summary:
add other 2 libs to the remove list
found when testing with local omnibus build, but it may not be omnibus specific.

Reviewed By: apolloww

Differential Revision: D40130827

fbshipit-source-id: 8fbbb040a0d045156379e475af6213d2f71c57d5
Summary: Rename all instances of `is_lto` to `is_bc` in *dist_lto.bzl* and *tools/dist_lto_planner.py*

Reviewed By: christycylee

Differential Revision: D40143397

fbshipit-source-id: c1358748c4f3834cea1782a35da356e56e7da5b1
Summary: These aren't things we use open source, and don't exist on crates.io.

Reviewed By: lmvasquezg

Differential Revision: D40147451

fbshipit-source-id: 3042be4c003c19ed4d3f42e9b3be3d0dc43820eb
… information - fbcode_targets_part9065

Summary:
This diff adds oncall to Buck TARGETS files in fbcode repository based on their cont_build configs.

To know more details about this effort, please check this post: https://fb.workplace.com/groups/devx.build.bffs/posts/5268257346542728

This diff was generated by this command:
    python3 bulk_add_oncall_to_fb_targets.py -o /tmp/cont_build_folder_oncall.txt  -s ~/fbsource/ -f fbcode_targets_part9065

drop_conflicts

Differential Revision: D39717045

fbshipit-source-id: 39987637df6dce2b644e4175ac56b1e3b2aaacde
Summary: it's been bothering me :)

Reviewed By: lmvasquezg

Differential Revision: D40148026

fbshipit-source-id: 24404a67758ecbd5af9ee088bfaf8a72b0d54b3a
Summary:
Seems sometime the input archive can be symlink, resolve it before actually check its type.

Found during omnibus test, but it may not be omnibus specific

Reviewed By: apolloww

Differential Revision: D40130875

fbshipit-source-id: 7d1d8504bdd0db5c3c94e0e46e5180fae4003714
Summary: Required to bootstrap Rust

Reviewed By: lmvasquezg

Differential Revision: D40148132

fbshipit-source-id: 0c5f97cb590d3be245fdc514931b85f34f417903
Summary:
This is the appropriate Cargo.toml and stuff to generate a reindeer Rust vendored project. With these changes, I can run reindeer and then construct a build graph.

Much of this code is lightly modified from https://github.com/facebookincubator/reindeer

Reviewed By: lmvasquezg

Differential Revision: D40149639

fbshipit-source-id: 5b11127cf9f69479cd8172daec93878a8ac0e744
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 7, 2022
@aherrmann aherrmann requested review from arlyon and ndmitchell October 7, 2022 13:02
@arlyon
Copy link
Contributor

arlyon commented Oct 7, 2022

Exciting! Looking at this now

@facebook-github-bot
Copy link
Contributor

@ndmitchell has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot pushed a commit to facebook/buck2-prelude that referenced this pull request Oct 10, 2022
Summary:
Closes facebook/buck2#20

Adds `prelude//toolchains/cxx/zig` with a C/C++ toolchain based on `zig cc`.
The doc-string of the added module contains a description and examples
configurations.

Also adds an example to the repository that builds a C++ library and a binary
depending on that library using this toolchain.

Note, for that example to work in the open source buck2 tree the following additional patch needs to be applied:
```diff
 diff --git a/.buckconfig b/.buckconfig
deleted file mode 100644
index c32af92..0000000
 --- a/.buckconfig
+++ /dev/null
@@ -1,19 +0,0 @@
-[repositories]
-root = .
-prelude = prelude
-ovr_config = prelude
-shim = shim
-toolchains = shim
-fbcode = shim
-fbcode_macros = shim
-fbsource = shim
-buck = shim
-
-[buildfile]
-name = TARGETS
-
-[build]
-execution_platforms = ovr_config//platforms:default
-
-[parser]
-target_platform_detector_spec = target://...->ovr_config//platforms:default
 diff --git a/prelude/cxx/tools/TARGETS.v2 b/prelude/cxx/tools/TARGETS.v2
index 2030d2f..5db1689 100644
 --- a/prelude/cxx/tools/TARGETS.v2
+++ b/prelude/cxx/tools/TARGETS.v2
@@ -1,4 +1,3 @@
-load("fbcode_macros//build_defs/lib:python_common.bzl", "get_ldflags", "get_strip_mode")
 load(":defs.bzl", "cxx_hacks", "omnibus_environment")

 prelude = native
@@ -538,7 +537,7 @@ omnibus_environment(
     # We override the binary-level ldflags with library level ldfalgs for
     # shared roots. We include 2 important things: stripping binaries, and not
     # discarding GPU code.
-    shared_root_ld_flags = get_ldflags("", "", "omnibus_root", get_strip_mode("", ""), "") + [
+    shared_root_ld_flags = [
         "-Wl,--no-discard-section=.nv_fatbin",
         "-Wl,--no-discard-section=.nvFatBinSegment",
         # Reorder nv_fatbin after the bss section to avoid overflow
```

X-link: facebook/buck2#22

Reviewed By: arlyon

Differential Revision: D40179209

Pulled By: arlyon

fbshipit-source-id: d334f2ad4563281f78ba3f479b8defd09616cabf
facebook-github-bot pushed a commit that referenced this pull request Oct 10, 2022
Summary:
Closes #20

Adds `prelude//toolchains/cxx/zig` with a C/C++ toolchain based on `zig cc`.
The doc-string of the added module contains a description and examples
configurations.

Also adds an example to the repository that builds a C++ library and a binary
depending on that library using this toolchain.

Note, for that example to work in the open source buck2 tree the following additional patch needs to be applied:
```diff
 diff --git a/.buckconfig b/.buckconfig
deleted file mode 100644
index c32af92..0000000
 --- a/.buckconfig
+++ /dev/null
@@ -1,19 +0,0 @@
-[repositories]
-root = .
-prelude = prelude
-ovr_config = prelude
-shim = shim
-toolchains = shim
-fbcode = shim
-fbcode_macros = shim
-fbsource = shim
-buck = shim
-
-[buildfile]
-name = TARGETS
-
-[build]
-execution_platforms = ovr_config//platforms:default
-
-[parser]
-target_platform_detector_spec = target://...->ovr_config//platforms:default
 diff --git a/prelude/cxx/tools/TARGETS.v2 b/prelude/cxx/tools/TARGETS.v2
index 2030d2f..5db1689 100644
 --- a/prelude/cxx/tools/TARGETS.v2
+++ b/prelude/cxx/tools/TARGETS.v2
@@ -1,4 +1,3 @@
-load("fbcode_macros//build_defs/lib:python_common.bzl", "get_ldflags", "get_strip_mode")
 load(":defs.bzl", "cxx_hacks", "omnibus_environment")

 prelude = native
@@ -538,7 +537,7 @@ omnibus_environment(
     # We override the binary-level ldflags with library level ldfalgs for
     # shared roots. We include 2 important things: stripping binaries, and not
     # discarding GPU code.
-    shared_root_ld_flags = get_ldflags("", "", "omnibus_root", get_strip_mode("", ""), "") + [
+    shared_root_ld_flags = [
         "-Wl,--no-discard-section=.nv_fatbin",
         "-Wl,--no-discard-section=.nvFatBinSegment",
         # Reorder nv_fatbin after the bss section to avoid overflow
```

Pull Request resolved: #22

Reviewed By: arlyon

Differential Revision: D40179209

Pulled By: arlyon

fbshipit-source-id: d334f2ad4563281f78ba3f479b8defd09616cabf
facebook-github-bot pushed a commit that referenced this pull request Feb 15, 2024
Summary:
The stack around D42099161 introduced the ability to dynamically set the log level via a debug-command. This requires using `tracing_subscriber::reload::Layer`. The implementation of that machinery is backed by a `RwLock` [here](https://fburl.com/code/4xv0ihpn). This `RwLock` is read locked once on every single `#[instrumentation]` call to check whether the instrumentation is active or not.

On top of that, something in our fbsource third-party config enables the `parking_lot` feature of `tracing_subscriber`. This means that this is a `parking_lot::RwLock`, not a `std::sync::RwLock`, which is bad because it's well known that parking lot has problems with excessive spinning.

What all that adds up to is that when you put `#[instrument]` on a super hot function in dice, you get dozens of threads that are all simultaneously doing this:

```
  thread #23, name = 'buck2-rt', stop reason = signal SIGSTOP
    frame #0: 0x000000000c94a1fa buck2`<parking_lot::raw_rwlock::RawRwLock>::lock_shared_slow + 122
    frame #1: 0x0000000007a80b54 buck2`<tracing_subscriber::layer::layered::Layered<tracing_subscriber::reload::Layer<tracing_subscriber::filter::layer_filters::Filtered<tracing_subscriber::fmt::fmt_layer::Layer<tracing_subscriber::registry::sharded::Registry, tracing_subscriber::fmt::format::DefaultFields, tracing_subscriber::fmt::format::Format, std::io::stdio::stderr>, tracing_subscriber::filter::env::EnvFilter, tracing_subscriber::registry::sharded::Registry>, tracing_subscriber::registry::sharded::Registry>, tracing_subscriber::registry::sharded::Registry> as tracing_core::subscriber::Subscriber>::enabled + 292
    frame #2: 0x000000000a172d77 buck2`<dice::legacy::incremental::dep_trackers::internals::Dep<dice::legacy::key::StoragePropertiesForKey<buck2_build_api::actions::calculation::BuildKey>> as dice::legacy::incremental::graph::dependencies::Dependency>::recompute::{closure#0} + 183
    frame #3: 0x000000000a03606c buck2`<futures_util::stream::futures_unordered::FuturesUnordered<core::pin::Pin<alloc::boxed::Box<dyn core::future::future::Future<Output = core::result::Result<(alloc::boxed::Box<dyn dice::legacy::incremental::graph::dependencies::ComputedDependency>, alloc::sync::Arc<dyn dice::legacy::incremental::graph::GraphNodeDyn>), dice::api::error::DiceError>> + core::marker::Send>>> as futures_util::stream::stream::StreamExt>::poll_next_unpin + 444
    frame #4: 0x0000000009fc4755 buck2`<dice::legacy::incremental::IncrementalEngine<dice::legacy::key::StoragePropertiesForKey<buck2_build_api::actions::calculation::BuildKey>>>::compute_whether_dependencies_changed::{closure#0} (.llvm.4229350879637282184) + 1733
    frame #5: 0x0000000009ef30d4 buck2`<dice::legacy::incremental::IncrementalEngine<dice::legacy::key::StoragePropertiesForKey<buck2_build_api::actions::calculation::BuildKey>>>::compute_whether_versioned_dependencies_changed::{closure#0}::{closure#0} + 228
    frame #6: 0x0000000009f194ec buck2`<buck2_futures::cancellable_future::CancellableFuture<<dice::legacy::incremental::IncrementalEngine<dice::legacy::key::StoragePropertiesForKey<buck2_build_api::actions::calculation::BuildKey>>>::new_dice_task::{closure#0}> as core::future::future::Future>::poll (.llvm.11181184606289051452) + 3500
    frame #7: 0x0000000009f04bbf buck2`<futures_util::future::future::map::Map<buck2_futures::cancellable_future::CancellableFuture<<dice::legacy::incremental::IncrementalEngine<dice::legacy::key::StoragePropertiesForKey<buck2_build_api::actions::calculation::BuildKey>>>::new_dice_task::{closure#0}>, buck2_futures::spawn::spawn_inner<<dice::legacy::incremental::IncrementalEngine<dice::legacy::key::StoragePropertiesForKey<buck2_build_api::actions::calculation::BuildKey>>>::new_dice_task::{closure#0}, dice::api::user_data::UserComputationData, futures_util::future::ready::Ready<()>>::{closure#0}> as core::future::future::Future>::poll + 31
    frame #8: 0x0000000009ff0339 buck2`<futures_util::future::future::flatten::Flatten<futures_util::future::future::Map<futures_util::future::ready::Ready<()>, buck2_futures::spawn::spawn_inner<<dice::legacy::incremental::IncrementalEngine<dice::legacy::key::StoragePropertiesForKey<buck2_build_api::actions::calculation::BuildKey>>>::new_dice_task::{closure#0}, dice::api::user_data::UserComputationData, futures_util::future::ready::Ready<()>>::{closure#1}>, <futures_util::future::future::Map<futures_util::future::ready::Ready<()>, buck2_futures::spawn::spawn_inner<<dice::legacy::incremental::IncrementalEngine<dice::legacy::key::StoragePropertiesForKey<buck2_build_api::actions::calculation::BuildKey>>>::new_dice_task::{closure#0}, dice::api::user_data::UserComputationData, futures_util::future::ready::Ready<()>>::{closure#1}> as core::future::future::Future>::Output> as core::future::future::Future>::poll + 201
    frame #9: 0x00000000093f9f22 buck2`<tokio::task::task_local::TaskLocalFuture<buck2_events::dispatch::EventDispatcher, core::pin::Pin<alloc::boxed::Box<dyn core::future::future::Future<Output = alloc::boxed::Box<dyn core::any::Any + core::marker::Send>> + core::marker::Send>>> as core::future::future::Future>::poll + 130
    frame #10: 0x000000000939fdcb buck2`<tracing::instrument::Instrumented<<buck2_build_api::spawner::BuckSpawner as buck2_futures::spawner::Spawner<dice::api::user_data::UserComputationData>>::spawn::{closure#0}> as core::future::future::Future>::poll + 139
    frame #11: 0x00000000091ca5a9 buck2`<tokio::runtime::task::core::Core<tracing::instrument::Instrumented<<buck2_build_api::spawner::BuckSpawner as buck2_futures::spawner::Spawner<dice::api::user_data::UserComputationData>>::spawn::{closure#0}>, alloc::sync::Arc<tokio::runtime::scheduler::multi_thread::handle::Handle>>>::poll + 169
    frame #12: 0x00000000091c1b22 buck2`<tokio::runtime::task::harness::Harness<tracing::instrument::Instrumented<<buck2_build_api::spawner::BuckSpawner as buck2_futures::spawner::Spawner<dice::api::user_data::UserComputationData>>::spawn::{closure#0}>, alloc::sync::Arc<tokio::runtime::scheduler::multi_thread::handle::Handle>>>::poll + 146
    frame #13: 0x000000000c920f6f buck2`<tokio::runtime::scheduler::multi_thread::worker::Context>::run_task + 895
    frame #14: 0x000000000c91f847 buck2`<tokio::runtime::scheduler::multi_thread::worker::Context>::run + 2103
    frame #15: 0x000000000c932264 buck2`<tokio::runtime::context::scoped::Scoped<tokio::runtime::scheduler::Context>>::set::<tokio::runtime::scheduler::multi_thread::worker::run::{closure#0}::{closure#0}, ()> + 52
    frame #16: 0x000000000c932169 buck2`tokio::runtime::context::runtime::enter_runtime::<tokio::runtime::scheduler::multi_thread::worker::run::{closure#0}, ()> + 441
    frame #17: 0x000000000c91efa6 buck2`tokio::runtime::scheduler::multi_thread::worker::run + 70
    frame #18: 0x000000000c906a50 buck2`<tracing::instrument::Instrumented<tokio::runtime::blocking::task::BlockingTask<<tokio::runtime::scheduler::multi_thread::worker::Launch>::launch::{closure#0}>> as core::future::future::Future>::poll + 160
    frame #19: 0x000000000c8f8af9 buck2`<tokio::loom::std::unsafe_cell::UnsafeCell<tokio::runtime::task::core::Stage<tracing::instrument::Instrumented<tokio::runtime::blocking::task::BlockingTask<<tokio::runtime::scheduler::multi_thread::worker::Launch>::launch::{closure#0}>>>>>::with_mut::<core::task::poll::Poll<()>, <tokio::runtime::task::core::Core<tracing::instrument::Instrumented<tokio::runtime::blocking::task::BlockingTask<<tokio::runtime::scheduler::multi_thread::worker::Launch>::launch::{closure#0}>>, tokio::runtime::blocking::schedule::BlockingSchedule>>::poll::{closure#0}> + 153
    frame #20: 0x000000000c90166b buck2`<tokio::runtime::task::core::Core<tracing::instrument::Instrumented<tokio::runtime::blocking::task::BlockingTask<<tokio::runtime::scheduler::multi_thread::worker::Launch>::launch::{closure#0}>>, tokio::runtime::blocking::schedule::BlockingSchedule>>::poll + 43
    frame #21: 0x000000000c90d9b8 buck2`<tokio::runtime::task::harness::Harness<tracing::instrument::Instrumented<tokio::runtime::blocking::task::BlockingTask<<tokio::runtime::scheduler::multi_thread::worker::Launch>::launch::{closure#0}>>, tokio::runtime::blocking::schedule::BlockingSchedule>>::poll + 152
    frame #22: 0x000000000c90b848 buck2`<tokio::runtime::blocking::pool::Inner>::run + 216
    frame #23: 0x000000000c9002ab buck2`std::sys_common::backtrace::__rust_begin_short_backtrace::<<tokio::runtime::blocking::pool::Spawner>::spawn_thread::{closure#0}, ()> + 187
    frame #24: 0x000000000c90042e buck2`<<std::thread::Builder>::spawn_unchecked_<<tokio::runtime::blocking::pool::Spawner>::spawn_thread::{closure#0}, ()>::{closure#1} as core::ops::function::FnOnce<()>>::call_once::{shim:vtable#0} + 158
    frame #25: 0x000000000757e4b5 buck2`std::sys::unix::thread::Thread::new::thread_start::h618b0b8e7bda0b2b + 37
    frame #26: 0x00007f2ba1e9abaf
```

This hit an open source user super hard over in #555 .

This diff approaches this issue by putting all the `#[instrument]`s in dice behind `#[cfg(debug_assertions)]`. This allows them to still be used in debug mode, but keeps them out of any hot paths. I do not have numbers to show that most of these matter. The only one I know matters for sure is `recompute`.

Alternatives are to either try and tailor this to the callsites we know are hot (that sounds error prone) or to revert the stack that inadvertently introduced the RwLock. Even if we did that though, it's probably still safer to keep `#[instrument]` out of super hot paths like this.

Reviewed By: stepancheg

Differential Revision: D53744041

fbshipit-source-id: 85bce9af2fec8ad1d50adc241d3b8e2bfc5cec87
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Providing a hermetic C/C++ toolchain