Avoid duplicating process handles on the browser UI thread
On Windows, ::DuplicateHandle() is not a cheap user-mode operation.
Handle operations serialize on the per-process handle table lock and,
for process objects, the system-wide process table lock, so duplication
contends with file and process creation machine-wide and is prone to
priority inversion. Third-party security software compounds this by
registering ObRegisterCallbacks() callbacks for PsProcessType covering
OB_OPERATION_HANDLE_DUPLICATE, which run inline on the calling thread.
Browser UI thread stalls of ~1.4s, and in older reports 12-20s, have
been attributed to these calls.
- ChildProcessLauncher::SetProcessPriorityImpl() duplicated the handle
on every priority change, which for renderers happens on every tab
foreground/background switch, and is also the code that puts children
into the background. ChildProcessLauncherHelper now caches its own
copy on the launcher thread at launch and applies priority changes to
that, releasing it once the child has exited.
DumpProcessStack() and, on Android, SetRenderProcessPriority() also
duplicated the handle just to hand it to the launcher thread. They now
read that same cached copy, so ChildProcessLauncher no longer
duplicates on the UI thread at all.
- ModuleEventSinkImpl::OnModuleEvents() duplicated the handle once per
module load address. Since this is called repeatedly with batches of
addresses while a child process starts up, it put a large number of
handle operations on the UI thread. The process is now held by a
refcounted pointer and shared with the background tasks.
Also add Windows.DuplicateProcess.Duration.{Thread} to measure the cost
of the remaining callers, so that any left on latency-sensitive threads
can be found and removals verified in the field. Threads running a UI
message pump are reported separately, because that is where the
resulting stall is user visible as jank or a hang.
Bug: 40716800, 41439736
Change-Id: I37de4ce39e5e4ee5d140fee2d9e277881d059751
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8341090
Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: Chris Davis <chrdavis@microsoft.com>
Reviewed-by: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Bo Liu <boliu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1703889}
NOKEYCHECK=True
GitOrigin-RevId: 03788be1271edbb39d96dc72d770c56a83e0178e
Contains a written down set of principles and other information on //base. Please add to it!
Chromium is a very mature project. Most things that are generally useful are already here and things not here aren't generally useful.
The bar for adding stuff to base is that it must have demonstrated wide applicability. Prefer to add things closer to where they're used (i.e. “not base”), and pull into base only when needed. In a project our size, sometimes even duplication is OK and inevitable.
Adding a new logging macro DPVELOG_NE is not more clear than just writing the stuff you want to log in a regular logging statement, even if it makes your calling code longer. Just add it to your own code.
If the code in question does not need to be used inside base, but will have multiple consumers across the codebase, consider placing it in a new directory under components/ instead.
base is written for the Chromium project and is not intended to be used outside it. Using base outside of src.git is explicitly not supported, and base makes no guarantees about API (or even ABI) stability (like all other code in Chromium). New code that depends on base/ must be in src.git. Code that's not in src.git but pulled in through DEPS (for example, v8) cannot use base.
Owners are added when a contributor has shown the above qualifications and when they express interest. There isn't an upper bound on the number of OWNERS.
Since the primitives provided by //base are used very widely, it is important to ensure they scale to the necessary workloads and perform well under all supported platforms. The base_perftests target is a suite of synthetic microbenchmarks that measure performance in various scenarios:
thread_local, the implementation in //base, the POSIX/WinAPI directly)Regressions in these benchmarks can generally by caused by 1) operating system changes, 2) compiler version or flag changes or 3) changes in //base code itself.
Rust code in base should be organized into very small crates, split up by function. Merging crates is sometimes unavoidable (due to dependency cycles or the orphaning rule).
Rust files should live near the equivalent C++ files (if any), and use the same naming scheme (for example, run_loop.rs, not run_loop_rust.rs).
When adding FFI shims, prefer separate _shim.h files rather than adding code to existing C++ files. This helps avoid circular dependencies with the //base target. It also avoids adding code to commonly-used headers, which can increase compile size by a lot.
Crates which you expect to be widely used should be added to the public_deps of the //base:base_rust target, so that developers can simply depend on //base:base_rust the same way they do with //base.