Delete CrossThreadCopier [chromium/src : main]
Daniel Cheng (Gerrit)
Daniel Cheng added 6 comments![]( "Open in Gerrit")
@mlip...@chromium.org and @jbr...@chromium.org for the overall review.
tl;dr I really don't think the costs of `CrossThreadCopier` justify its existence anymore. It's true that Oilpan types are still thread-hostile, but the issue has always been transitive violations, which the current traits don't really do much for anyway.
I have toyed with the idea of having some marker for thread-hostile things, and then having the clang plugin derive thread-safety transitively and enforce it for things that care (like `CrossThreadBind...`)–basically, the Rust style of "compiler derives `Send` for you where it can" but I don't think figuring that out should be blocking for this cleanup either. Please let me know you feel differently.
@ca...@chromium.org, @fma...@chromium.org I left some implementation-specific questions below–PTAL at those if you can.
params->IsolatedCopy()));If we don't call `IsolatedCopy()` here, it won't build.
Technically you can still do an unsafe thing here though if you call `std::move(*this)`...
: public CrossThreadCopierPassThrough<base::span<uint8_t>> {This is an example of a specialization that's not really defined in the correct place and could lead to ODR issues.
Actually even bindings spans is pretty dubious... but that's something I'm tackling separately.
// GarbageCollected heap but it doesn't actually have to.@ca...@chromium.org, is this really load-bearing? I see this was made GCed in https://chromium-review.googlesource.com/c/chromium/src/+/6485451 but from my perspective it's only adding complexity.
CHECK(bitmap.isImmutable() || bitmap.isNull())@fma...@chromium.org, do we really need to keep these runtime checks?
(If this were code outside of Blink, we just wouldn't have these checks at all)
"sk_sp<T> can be passed across threads only if T is SkRefCnt.");@fma...@chromium.org, is this `static_assert` really load-bearing? It seems "nice to have" but I'm not sure it's "keep hundreds of lines of C++ specializations nice to have".
Related details
- Andrey Kosyakov
- Florin Malita
- Jeremy Roman
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 11 comments![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
11 comments
bindings/ and inspector/ lgtm
CSSSyntaxDefinition IsolatedCopy() const;Looks like you've just deleted the only call site, so this can go as well.
scoped_refptr<blink::SerializedScriptValue> message;drive-by: looks like this may be destroyed on a thread different from its creation thread (both before and after this change), and `~SerializedScriptValue()` is not entirely thread safe, at least when it comes to [non-production code](https://source.chromium.org/chromium/chromium/src/+/main:v8/src/api/api.cc;l=12455;drc=db569cd7847d479358bd391a18b2c28393803333). The prod code looks thread-safe though, at least at a quick glance.
params->IsolatedCopy()));If we don't call `IsolatedCopy()` here, it won't build.
Technically you can still do an unsafe thing here though if you call `std::move(*this)`...
Looks like ModuleScriptCreationParams intentionally clears some fields though, so maybe we shouldn't get around it.
// TODO(dcheng): Unclear if this is even needed.It's needed for as the underlying V8StackTrace::clone() is non-trivial (i.e. drops some non-thread safe fields).
// GarbageCollected heap but it doesn't actually have to.@ca...@chromium.org, is this really load-bearing? I see this was made GCed in https://chromium-review.googlesource.com/c/chromium/src/+/6485451 but from my perspective it's only adding complexity.
The original plan was to start using some v8 types in SourceLocation, but for the time being further development went a slightly different path. That said, I think it may still have some merit. A SourceLocation being sent accross threads is a very infrequent case, whereas SourceLocation (proper) is rather performance-critical and we want it to be as fast as possible. Considering it's not uncommon to discard it, we primarily care for capturing source location to be fast, and from this point of view keeping v8 types and converting lazily might help.
struct CrossThreadCopier<internal::BasicCrossThreadHandle<T, WeaknessPolicy>>drive-by thought: do we have a way to accidentally prevent regular persistent handles from being posted across threads now?
Related details
- Daniel Cheng
- Florin Malita
- Jeremy Roman
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Daniel Cheng (Gerrit)
Daniel Cheng added 8 comments![]( "Open in Gerrit")
Please nuke the entire file.
This is intentional, to avoid dealing with IWYU errors. Copied the TODO here.
Please nuke the entire file.
This one I missed though. Done now.
Daniel ChengNuke the entire file?
Intentionally kept for now but copied the TODO here as well.
Daniel Chengditto.
Good call, done.
params->IsolatedCopy()));Andrey KosyakovIf we don't call `IsolatedCopy()` here, it won't build.
Technically you can still do an unsafe thing here though if you call `std::move(*this)`...
Looks like ModuleScriptCreationParams intentionally clears some fields though, so maybe we shouldn't get around it.
Yes, sorry–to be clear, calling `std::move(*this)` would be a bug, and I am highlighting this as a thing that we would no longer be able to catch.
// TODO(dcheng): Unclear if this is even needed.It's needed for as the underlying V8StackTrace::clone() is non-trivial (i.e. drops some non-thread safe fields).
I'm going to have to think about how to make this CL safe in light of this.
// GarbageCollected heap but it doesn't actually have to.Andrey Kosyakov@ca...@chromium.org, is this really load-bearing? I see this was made GCed in https://chromium-review.googlesource.com/c/chromium/src/+/6485451 but from my perspective it's only adding complexity.
The original plan was to start using some v8 types in SourceLocation, but for the time being further development went a slightly different path. That said, I think it may still have some merit. A SourceLocation being sent accross threads is a very infrequent case, whereas SourceLocation (proper) is rather performance-critical and we want it to be as fast as possible. Considering it's not uncommon to discard it, we primarily care for capturing source location to be fast, and from this point of view keeping v8 types and converting lazily might help.
OK I'm willing to buy that argument. I just worry because it's currently easy to bind this unsafely. I've reverted the changes to this file.
struct CrossThreadCopier<internal::BasicCrossThreadHandle<T, WeaknessPolicy>>drive-by thought: do we have a way to accidentally prevent regular persistent handles from being posted across threads now?
There is a static_assert in `CrossThreadBindOnce()` that theoretically should catch this.
But it doesn't catch transitive violations, e.g. see https://chromium-review.googlesource.com/7167966
Related details
- Andrey Kosyakov
- Florin Malita
- Jeremy Roman
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florin Malita (Gerrit)
Florin Malita added 2 comments![]( "Open in Gerrit")
CHECK(bitmap.isImmutable() || bitmap.isNull())@fma...@chromium.org, do we really need to keep these runtime checks?
(If this were code outside of Blink, we just wouldn't have these checks at all)
New code should use `SkImage`s instead, which are intrinsically immutable. Removing sgtm.
"sk_sp<T> can be passed across threads only if T is SkRefCnt.");@fma...@chromium.org, is this `static_assert` really load-bearing? It seems "nice to have" but I'm not sure it's "keep hundreds of lines of C++ specializations nice to have".
Agreed, I don't think it's worth keeping this machinery around just for an assert.
(it also arbitrarily excludes `SkNVRefCnt`-derived types)
Related details
- Andrey Kosyakov
- Daniel Cheng
- Jeremy Roman
- Michael Lippautz
Daniel Cheng (Gerrit)
Daniel Cheng added 3 comments![]( "Open in Gerrit")
// TODO(dcheng): Unclear if this is even needed.Daniel ChengIt's needed for as the underlying V8StackTrace::clone() is non-trivial (i.e. drops some non-thread safe fields).
I'm going to have to think about how to make this CL safe in light of this.
OK, the parent CL should address this. `CrossThreadSourceLocation` is now always inherently safe to move across threads.
CHECK(bitmap.isImmutable() || bitmap.isNull())Florin Malita@fma...@chromium.org, do we really need to keep these runtime checks?
(If this were code outside of Blink, we just wouldn't have these checks at all)
New code should use `SkImage`s instead, which are intrinsically immutable. Removing sgtm.
Acknowledged
"sk_sp<T> can be passed across threads only if T is SkRefCnt.");Florin Malita@fma...@chromium.org, is this `static_assert` really load-bearing? It seems "nice to have" but I'm not sure it's "keep hundreds of lines of C++ specializations nice to have".
Agreed, I don't think it's worth keeping this machinery around just for an assert.
(it also arbitrarily excludes `SkNVRefCnt`-derived types)
- Andrey Kosyakov
- Jeremy Roman
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng added 1 comment![]( "Open in Gerrit")
Looks like you've just deleted the only call site, so this can go as well.
Oops, fixed this too now.
Related details
- Andrey Kosyakov
- Jeremy Roman
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 3 comments![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
3 comments
// TODO(dcheng): This one might have some value, though it's not entirely clearRemove this following the removal of class it was referring to?
struct CrossThreadCopier<internal::BasicCrossThreadHandle<T, WeaknessPolicy>>Daniel Chengdrive-by thought: do we have a way to accidentally prevent regular persistent handles from being posted across threads now?
There is a static_assert in `CrossThreadBindOnce()` that theoretically should catch this.
But it doesn't catch transitive violations, e.g. see https://chromium-review.googlesource.com/7167966
- Daniel Cheng
- Jeremy Roman
- Michael Lippautz
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement