- 22 Jul, 2020 1 commit
-
-
Pranav Thulasiram Bhat authored
Summary: The shallow copied child RC is often used to make small changes to the RC for a limited scope of the request. The tracepoint however reports the raw address of the RC as a way to identify the request itself - Therefore we end up reporting different identifiers for what is 'logically' the same request. This diff adds two additional arguments (the rootIds or pointers to the parent RC) to allow bpf programs to pick up this subtlety. Reviewed By: yfeldblum Differential Revision: D22424318 fbshipit-source-id: f669cabca10fa50b8a81fb4b3585d30654207e4c
-
- 21 Jul, 2020 7 commits
-
-
Nathan Bronson authored
Summary: This diff adds heterogeneous lookup support and eraseInto to the fallback (non-SIMD) version of F14 sets, which is used on mobile. std::is_invocable<hasher, const_iterator const&> is no longer evaluated during erase. It also restructures F14MapTest and F14SetTest so that as many of the tests as possible run in the fallback mode. (Note: this ignores all push blocking failures!) Reviewed By: yfeldblum, ot Differential Revision: D21489440 fbshipit-source-id: 872a250f8a6ec3c9efcd19edb7d9beca196fa865
-
Nathan Bronson authored
Summary: This diff implements efficient heterogeneous lookup, insert, and erase for the fallback (non-SIMD) mode of F14. It relies on internal implementation details of the underlying std::unordered_map implementation that happen to be true for libstdc++, libc++, and MSVC. Extending this technique to F14 sets would be straightforward, but has not been done here. (Note: this ignores all push blocking failures!) Differential Revision: D21408628 fbshipit-source-id: 72e9ddd82a50c02ec762d4b64dc3303848bd0218
-
Lee Howes authored
Summary: getTry reads like a blocking operation but is actually non-blocking. Remove to avoid confusion. (Note: this ignores all push blocking failures!) Reviewed By: yfeldblum Differential Revision: D22494732 fbshipit-source-id: d884d60cc14e2a53cd0657926a885a12b994c1ee
-
Robin Cheng authored
Summary: This is a pretty cryptic race condition, but it works like this: Each thread local instance (i.e. one ThreadLocalPtr) is assigned a unique ID by the static meta. This ID is freed up when the thread local is destroyed, and may then be reused by another newly created thread local. For each thread we keep an array of elements (ElementWrapper), indexed by the thread local ID. Each element has some metadata such as the pointer to the object being cached for that thread and that thread local. There are a few cases where we access a ElementWrapper object: - We write to it under a global lock, when destroying a thread local object (only the i-th element is written to for each thread, if the ID is 'i' for the thread local). - We write to it under a global lock, when reallocating the array of elements for the thread if a new thread local ID exceeds the capacity of the array; this can happen when we are reading the thread local for a thread. - We write to it under a global lock when the corresponding thread is destroyed. - We read from it, *without* any locks, when reading a thread local's value from a thread and the above reallocation case does not occur. The race condition happens in this case: - Thread locals A and B are created - Thread local A is accessed by thread Z, and gets, say ID = 5, the corresponding element E is initialized - Thread local A is destroyed by thread X, and under the global lock, the element E is written to (setting element->ptr = nullptr); ID 5 is released to the list of free IDs. - Thread local B is accessed for the first time by thread Y, and therefore needs to grab an ID; under the global lock, it happens to get ID = 5, and stores that ID into the thread local instance as an atomic int; the store here uses std::memory_order_seq_cst. - Thread local B is then accessed again from thread Z; this time the atomic int ID is loaded with std::memory_order_relaxed, with the resulting value of 5; we then proceed to read element E's ptr value *without any locks*. TSAN reports the last read of element E to be racy with the previous write of element E during thread X's destruction of thread local A. Even though this happened with a mutex operation in between, the final std::memory_order_relaxed causes TSAN to think that the read access was not properly synchronized after the earlier write. This conjecture is confirmed by the following small reproducible test case: ``` TEST(MyTest, Test) { std::mutex mutex; int x = 0; bool written = false; std::atomic<bool> seen{false}; std::thread t0([&] { while (!seen.load(std::memory_order_relaxed)) { } std::cout << x; }); std::thread t1([&] { while (true) { std::lock_guard<std::mutex> guard(mutex); if (written) { seen.store(true, std::memory_order_release); break; } } }); std::thread t2([&] { std::lock_guard<std::mutex> guard(mutex); x = 1; written = true; }); t0.join(); t1.join(); t2.join(); } ``` (This example, when run under TSAN, reports a race condition between "x = 1" and "std::cout << x".) This change makes the load use std::memory_order_acquire so it synchronizes with the store. Should have no performance implications on x86 but I'm not totally confident about that. While we're at it, also changing the allocate() and destroy() functions to use more relaxed memory orders; sequential consistency is unnecessary in these places. Reviewed By: yfeldblum Differential Revision: D22561685 fbshipit-source-id: 0d53925486ef78139ffe71681827b0c0959dd29e
-
Nathan Bronson authored
Summary: libstdc++ versions that don't include N4387 ("perfect initialization" for pairs and tuples) can't use the two-arg pair constructor as an emplace target when the emplace args don't exactly match the destination types. (libstdc++.so.6.0.22 is the first one that contains the improvement, which corresponds to gcc 6.1.0). This diff guards the optimization added in D21419822 (https://github.com/facebook/folly/commit/7d618e4fde39b44e0952880653e10ec97400583f) with an enable_if test to avoid the problem, and includes the template parameter changes necessary to get the target types to the test site. (Note: this ignores all push blocking failures!) Differential Revision: D21766560 fbshipit-source-id: f8b2a0230504237f976e14b0740bb31bc7a57b77
-
Yedidya Feldblum authored
Summary: [Folly] A test for the `atomic_ref` deduction guide, which permits removal of the preprocessor guard in the definition of `make_atomic_ref`. Some parts of folly still support legacy versions of C++ but we do not necessarily run the tests on legacy platforms. Reviewed By: akrieger Differential Revision: D22635366 fbshipit-source-id: 8961d3465b63875a660d9e83d094ebf0cd64d1df
-
Mark Santaniello authored
Summary: Add a `clear()` API to render the Arena like new, except that previously-allocated blocks are preserved to be reused in the future. (Caveat: we do not reuse "large blocks") Reviewed By: yfeldblum, davidtgoldblatt Differential Revision: D22209898 fbshipit-source-id: c2f27d70ee09855df8c8c6fc6d162fca92b9eac8
-
- 18 Jul, 2020 4 commits
-
-
Yedidya Feldblum authored
Summary: [Folly] Mark invoker `operator()` as `[[maybe_unused]]` when defined in macros. If the macro is used in a `.cpp` but the generated `operator()` is not invoked, the compiler might warn. This can easily happen in the following situation: ``` FOLLY_CREATE_MEMBER_INVOKER(invoke_foo_fn, foo); FOLLY_CREATE_MEMBER_INVOKER(invoke_bar_fn, bar); using invoke_one_fn = std::conditional_t< /* cond */, invoke_foo_fn, invoke_bar_fn>; constexpr invoke_one_fn invoke_on; template <typename T> void go(T& t) { invoke_one(t); } ``` In such a situation, either `invoke_foo_fn::operator()` is seen at a call-site or `invoke_bar_fn::operator()` is seen at the call-site, but not both - one of them is unused, and if the right warnings are enabled, will be warned as unused. Reviewed By: luciang, markisaa Differential Revision: D22570047 fbshipit-source-id: a4caa7a95ab74ea075cf41c26525935f3d9186c0
-
Andrii Grynenko authored
Reviewed By: yfeldblum Differential Revision: D22604684 fbshipit-source-id: 4dedb9c831d0a2137d16bbe0514e00a1c8f77f4a
-
Stanislau Hlebik authored
fbshipit-source-id: 798decc90db4f13770e97cdce3c0df7d5421b2a3
-
Stanislau Hlebik authored
fbshipit-source-id: 5113fe0c527595e4227ff827253b7414abbdf7ac
-
- 17 Jul, 2020 2 commits
-
-
Yedidya Feldblum authored
Summary: [Folly] `FOLLY_KEEP` for keeping function definitions when using function-sections even if the linker would remove them. Reviewed By: alexshap Differential Revision: D22575832 fbshipit-source-id: 08410b3c318e361b1f3efcbb8f168091faed86e8
-
Zhengxu Chen authored
Summary: For program running in different general user contexts, e.g. services, multithreading, it's very useful to extend log prefixes to contain the domain specific context strings in debugging. This diff adds one interface function to push one callback to the logger. The callbacks will be executed when the log message data is constructed, and the strings returned by callbacks will be appended to the tail of log entry prefixes. Reviewed By: simpkins Differential Revision: D21291284 fbshipit-source-id: e141bf1c907f6730ea741d0dc06975add93d489c
-
- 16 Jul, 2020 11 commits
-
-
Eric Niebler authored
Summary: `folly::coro::Materialize` uses a `union` of `ManualLifetime` objects to store either the result (value) or the error of an async operation. However, it accesses these fields without ever activating them by in-place constructing the `ManualLifetime` objects. Likewise, when changing the active field, the `ManualLifetime` objects must by destroyed. It doesn't matter that these operations are no-ops. The must be there to avoid UB, which can manifest at higher optimization levels. I fix this by providing two functions: `activate` and `deactivate`, for use constructing and destructing values in `ManualLifetime` objects when those objects are fields in a `union`. Reviewed By: yfeldblum, kirkshoop Differential Revision: D22527895 fbshipit-source-id: 8cc1c3fae75672387d977dddeff61c82093abb9a
-
Lukasz Piatkowski authored
Summary: This diff adds a minimal workflow for running integrations tests for Mononoke. Currently only one test is run and it fails. This also splits the regular Mononoke CI into separate files for Linux and Mac to match the current style in Eden repo. There are the "scopeguard::defer" fixes here that somehow escaped the CI tests. Some tweaks have been made to "integration_runner_real.py" to make it runnable outside FB context. Lastly the change from using "[[ -v ... ]" to "[[ -n "${...:-}" ]]; in "library.sh" was made because the former is not supported by the default Bash version preinstalled on modern MacOS. Pull Request resolved: https://github.com/facebookexperimental/eden/pull/26 Reviewed By: krallin Differential Revision: D22541344 Pulled By: lukaspiatkowski fbshipit-source-id: 5023d147823166a8754be852c29b1e7b0e6d9f5f
-
Kirk Shoop authored
Summary: remove pushmi from folly Reviewed By: yfeldblum Differential Revision: D22531090 fbshipit-source-id: 3e6561f220cc3fd227dec321112ec848c4f17c6f
-
Maged Michael authored
Summary: Add warning about using the default inline executor for asynchronous reclamation, which may lead to deadlock if there are unintended circular dependencies. The use of inline executor is likely due to not calling folly::enable_hazptr_thread_pool_executor at some earlier point (e.g., due to not calling folly::init()). Also changed some function names for easy identification and fixed typos. Reviewed By: yfeldblum Differential Revision: D22536210 fbshipit-source-id: 89e74411a909ec97b03e8ff90f7039ab233c087b
-
Lukas Piatkowski authored
Summary: Pull Request resolved: https://github.com/facebookexperimental/eden/pull/27 Pull Request resolved: https://github.com/facebookexperimental/rust-shed/pull/9 Original diffs: D22417488 (https://github.com/facebook/folly/commit/43d80f110b181dd7f72f0f935322a58f9cbb5147), D22528869 (https://github.com/facebook/folly/commit/3930a6ef4437f161c50ac27141cd397eda999fa7) Reviewed By: markbt Differential Revision: D22571972 fbshipit-source-id: c6f013565680a757b642dd79e647207fce3351ec
-
Matt Galloway authored
Summary: Fix building folly sysuio portability when targeting the iOS simulator, because the current Xcode 12 SDK does not contain `preadv` and `pwritev` for the iOS simulator on Catalina, even though it declares it's there, so we get link errors. Reviewed By: yfeldblum Differential Revision: D22569172 fbshipit-source-id: 870b47063cb37a24364d1de292aba1f966edfb7c
-
Brandon Schlinker authored
Summary: This diff adds a constructor that takes an `AsyncSocket*`, so that lifecycle observers are informed via `move` when a socket is transformed. If one `AsyncSocket` is constructed from another using the `AsyncSocket(AsyncSocket* oldAsyncSocket)` constructor, then `move(oldSocket, newSocket)` will be triggered for attached observers, allowing the observers to detach from the old socket and attach to the new socket. However, we currently transform between `AsyncSocket` by detaching the fd and event base, and passing them to the new constructor: ``` # from FizzAcceptorHandshakeHelper.cpp auto evb = transport_->getEventBase(); auto fd = transport_->getUnderlyingTransport<folly::AsyncSocket>() ->detachNetworkSocket() .toFd(); transport_.reset(); sslSocket_ = createSSLSocket(sslContext_, evb, fd); ``` When this happens, any `AsyncSocket::LifecycleObserver` that were attached on accept to become separated from the fd/socket that they're attempting to follow. With this change, we can do the following instead (next diff): ``` sslSocket_ = createSSLSocket( sslContext_, transport_->getUnderlyingTransport<folly::AsyncSocket>()); transport_.reset(); ``` Because `createSSLSocket` uses the `AsyncSocket*` constructor, the `move` observer event will be triggered. Differential Revision: D21614835 fbshipit-source-id: 6c995f879fe41935850247a28ff8af2b33349445
-
Brandon Schlinker authored
Summary: Adds `AsyncTransport::LifecycleCallback`, an observer that is notified when a transport is closed and destroyed. Currently only supported for `AsyncSocket` and derived types (e.g., `AsyncSSLSocket`). `AsyncSocket::LifecycleCallback` is derived from `AsyncTransport::LifecycleCallback` and adds support for additional lifecycle events relevant to `AsyncSocket`. Details: - Can be used by instrumentation that ties its lifetime to that of the transport. - Intentionally separate from all existing callbacks that may be added / used by application logic because it is designed to be used by instrumentation that is generic, and thus separate / unaware of application logic. - Multiple observer can be registered, but a `folly::small_vector` is used to minimize alloc overhead in the common case of 0 - 2 being registered. - The observer implementation is responsible for calling `removeLifecycleObserver` to remove itself if it is destroyed before the socket is destroyed. Differential Revision: D21613750 fbshipit-source-id: 92bb5de30bc8bab56fa29e62800bf58e47486f1e
-
Brandon Schlinker authored
Summary: We have traditionally used `WriteFlags::EOR` to communicate that ACK timestamping should occur. This is confusing and mostly for legacy reasons when we had a custom kernel patch that would send ACK timestamps if the EoR flag was set. Creating proper flags for ACK and SCHED timestamps. I will integrate the logic that uses these flags into `AsyncSocket` in a subsequent diff. Differential Revision: D22039799 fbshipit-source-id: 23f534365475bb67f91d86657919cce02439f0c8
-
Brandon Schlinker authored
Summary: Socket timestamps (ACK / TX) and EoR tracking currently break for `AsyncSSLSocket` if SSL renegotiation occurs while a timestamped write / EoR write is in progress. - If EoR tracking is enabled, the EoR flag and any timestamp flags are not included until `AsyncSSLSocket` writes a buffer containing the final byte to the socket. This is to avoid these flags from being set on a partial write of the passed in buffer. - If a write occurs while an SSL renegotiation is in progress, OpenSSL will return `SSL_ERROR_WANT_WRITE`. When this happens, we need to call write again, passing back in the same buffer. - The current logic for deciding whether to include the EoR and timestamping flags (`eorAwareSSLWrite`) adds the number of bytes pending to the value returned by `AsyncSSLSocket::getRawBytesWritten` to determine the minimum byte offset when the flags should be added. - However, when a write fails due to SSL renegotiation, `getRawBytesWritten` may include some of the bytes that were passed in the last call, despite how they have not actually been written to the transport yet. This is because `getRawBytesWritten` is calculated based on the BIO chain length. - As a result, the current logic for calculating the offset on which to add the flags overshoots -- it returns an offset that will never be written. This causes the flags to not be added, and timestamps to timeout. - This results in one of two things: - Timestamp timeouts, where the timestamps are never received - If a subsequent write is timestamped, the timestamps from that write may be used instead. This will cause the timestamps to be inflated, and leads to higher TX / ACK times at upper percentiles. Fix is as follows: - Change the logic that determines whether the EoR is included in the buffer to no longer rely on `getRawBytesWritten`. In addition, simplify logic so that it is no longer a separate function and easier to make sense of. - Even if EoR tracking is enabled, always write timestamp flags (TX, ACK, etc.) on every write. This reduces the amount of coordination required between different components. The socket error message handler will end up with more cases of timestamps being delivered for earlier bytes than the last body byte, but they already need to deal with that today due to partial writes. I considered just outright removing support for EoR tracking (EoR was previously used for timestamping) but decided against this as there's still some value in setting the EoR flag for debugging; see notes in code. Reviewed By: yfeldblum Differential Revision: D21969420 fbshipit-source-id: db8e7e5fbd70d627f88f2c43199387f5112b5f9e
-
Orvid King authored
Summary: MSVC upstream decided not to add these builtins in the end, so drop the guards as these are still needed. Fixes: https://github.com/facebook/folly/issues/1412 Reviewed By: yfeldblum Differential Revision: D22559891 fbshipit-source-id: 7652da1299d8be7fd64a24f9cffd11b721071d68
-
- 15 Jul, 2020 4 commits
-
-
Kirk Shoop authored
Summary: explicitly move exception_ptr arg to fix error Reviewed By: Orvid Differential Revision: D22528984 fbshipit-source-id: e72e6008e6a899663970c6dff43c8790d044dd69
-
Matt Diffenderfer authored
Summary: By using `list::splice`, we can avoid making a call to create and delete nodes. By doing so, only internal pointers are reassigned. `list::splice` documentation: https://www.boost.org/doc/libs/1_67_0/doc/html/boost/intrusive/list.html#idp55504432-bb Reviewed By: yfeldblum Differential Revision: D22369091 fbshipit-source-id: efde0f520f81506b96272b320ae9d65ce1cabcb2
-
Zeyi (Rice) Fan authored
Summary: When we build boost 1.69.0 with newer version of Xcode, it will fail with: ``` clang: error: unknown argument: '-fcoalesce-templates' ``` This commit fixes this build failure by telling boost's build system that we are building with clang on macOS. Reviewed By: chadaustin Differential Revision: D22417488 fbshipit-source-id: 0b3d22835abbba6d06812c656acb0311a60d8c67
-
Zeyi (Rice) Fan authored
Reviewed By: wez Differential Revision: D22528869 fbshipit-source-id: 66c394b6fafcc45503b593f9f6b0605b5578ce56
-
- 14 Jul, 2020 7 commits
-
-
Brandon Kieft authored
Summary: Fix the documentation errors in Folly when building with Xcode 12. Xcode supports validating doxygen comments by enabling the `-Wdocumentation` flag. In Clang 12, a new feature was added to warn when an inline Doxygen comment has no argument (https://reviews.llvm.org/rL367809). `\a` and `\b` are special commands in doxygen and were being flagged as errors. I enclosed the example escape sequences within a code block to fix these errors. You can read about all the special doxygen commands here: https://www.doxygen.nl/manual/commands.html Reviewed By: yfeldblum Differential Revision: D22488410 fbshipit-source-id: bc4acf31b3df2a860202d0bd1ee356ce8f8fe49f
-
Eric Niebler authored
Summary: Moveable types should have no-throw move assignment operators. `Executor::KeepAlive<>`'s move assignment operator is not marked `noexcept`. It calls the virtual `Executor::keepAliveRelease`, which is also not marked `noexcept`, despite the fact that virtually (haha) all overrides of that function do nothing more than decrement an atomic and free some resources. This diff makes the following `noexcept`: * `Executor::keepAliveAcquire` * `Executor::keepAliveRelease` * `KeepAlive::reset()` * `KeepAlive::operator=(KeepAlive&&)` --- Reviewed By: yfeldblum Differential Revision: D22505764 fbshipit-source-id: 8e0a04e057c971673cf75da974c1abca2bdf87e8
-
Chad Austin authored
Summary: If the caller wants a shared_ptr, UniquePtr will implicitly promote. Reviewed By: yfeldblum Differential Revision: D22046594 fbshipit-source-id: 2e7d527d3ca33dae974c62db909df605c532ea44
-
Lee Howes authored
Summary: Remove the Future-returning form of collectAny completely. (Note: this ignores all push blocking failures!) Reviewed By: yfeldblum Differential Revision: D22345361 fbshipit-source-id: 180bb74c8f64052de372f5d982ad4e77cbff0119
-
Giuseppe Ottaviano authored
Summary: `std::function` has a footprint of 32 bytes and (almost) always allocates the interrupt handler. By using an intrusively reference-counted atomic pointer the footprint is just 8 bytes, and we save further 8 bytes by eliminating `interruptHandlerSet_` (it's a `bool`, but poorly aligned). This also allows to share the handler along the continuation chain, instead of copying for every core. In addition, the `getInterruptHandler()`/`setInterruptHandlerNoLock()` API was replaced by a single `initializeInterruptHandlerFrom()`, so we don't need to expose the internal storage details anymore. Reviewed By: yfeldblum Differential Revision: D22474230 fbshipit-source-id: 059828de3b89c25684465baf8e94bc1b68dac0da
-
Andres Suarez authored
Reviewed By: mzlee Differential Revision: D22495160 fbshipit-source-id: 3d6240906dd086ccac6668d907074ec7ca86ebce
-
Ian Petersen authored
Summary: The build with Clang 10 on iOS broke with the following errors: ``` folly/detail/MemoryIdler.h:86:68: error: implicit conversion from 'uint64_t' (aka 'unsigned long long') to 'float' may lose precision [-Werror,-Wimplicit-int-float-conversion] static_cast<float>(std::numeric_limits<uint64_t>::max()) * h; ~ ^ ``` ``` folly/detail/MemoryIdler.h:87:38: error: implicit conversion from 'std::__1::chrono::duration<long long, std::__1::ratio<1, 1000000000> >::rep' (aka 'long long') to 'float' may lose precision [-Werror,-Wimplicit-int-float-conversion] auto tics = uint64_t(idleTimeout.count() * (1 + extraFrac)); ~~~~~~~~~~~~^~~~~~~ ~ ``` This diff fixes the problem by making the existing implicit casts explicit. Reviewed By: Orvid Differential Revision: D22490364 fbshipit-source-id: 90bd116290de1d8906140d514f1d4880c3b3b085
-
- 13 Jul, 2020 2 commits
-
-
Lewis Baker authored
Summary: Mark the `await_resume()` method of the `final_suspend()` awaiter as `[[noreturn]]`. This helps the compiler to dead-code eliminate this code, and in particular helps the compiler to determine that code cannot continue execution after a `co_yield co_error(ex)` expression (which calls `final_suspend()`). This enables us to write code that has a `co_yield co_error()` statement as the last line in a non-void `Task` coroutine without the compiler emitting a warning about control running off the end of the coroutine. Reviewed By: yfeldblum Differential Revision: D22229020 fbshipit-source-id: b7a63b030cb42653198731d542ffa9bbf90daa83
-
Eric Niebler authored
Summary: `boost::variant` is an expensive template, and `boost/variant.hpp` is an expensive header. In the one place it is used in futures/detail/Core.h (a commonly-included header), it can be trivially replaced with a `union`, so do so. As a drive-by, I mark as `noexcept` those members of `KeepAliveOrDeferred` that can be done so unconditionally. Reviewed By: yfeldblum, kirkshoop, ot Differential Revision: D22460453 fbshipit-source-id: 5a01f873058273d1a20265507d87796450cc008b
-
- 12 Jul, 2020 1 commit
-
-
Lee Howes authored
Summary: Adds a strongly discouraged discard helper for cases where we know there is no deferred work. Reviewed By: yfeldblum Differential Revision: D22248947 fbshipit-source-id: 26265c33d9cd10848ab730f9c5f0356eb843025b
-
- 10 Jul, 2020 1 commit
-
-
Mark Santaniello authored
Summary: Use sized-deallocation (`sdallocx`) if possible in `folly::SysAllocator` and `folly::Arena`. `Arena` has always allocated two types of blocks: 1. Normal (fixed-sized): size is the "goodSize adjusted" `minBlockSize` 2. Large (variable-sized): when #1 is too small Type #2 makes sized-deallocation tricky -- we need somewhere to remember the allocated sizes. The old code used a single type `Block` and kept a single list. Here I change to have two types and two lists. The `LargeBlock` has an additional `allocSize` data member. This makes the Arena object itself 16B larger, but seems better than adding a 4B `allocSize` to each and every block, regardless of type. Note that, prior to this change, it was possible to `merge()` arenas with different `minBlockSize`. This is no longer possible. Reviewed By: davidtgoldblatt Differential Revision: D22466906 fbshipit-source-id: 719f357a0a1f6cfcda3208391837195c3859ab69
-