Protected Audience: prepare contexts during trusted signals download [chromium/src : main]
Abigail Katcoff (Gerrit)
Abigail Katcoff voted and added 1 comment![]( "Open in Gerrit")
Votes added by Abigail Katcoff
| Commit-Queue | +1 |
1 comment
Related details
- Russ Hamilton
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russ Hamilton (Gerrit)
Russ Hamilton added 3 comments![]( "Open in Gerrit")
I think we need a higher level integration test to ensure that we correctly run auctions. I'm particularly concerned about interactions with the multi-threaded bidding feature.
fresh_context_recycler = std::move(unused_context_recyclers_.back());How do we ensure that the context is associated with the current thread?
size_t thread_index = next_thread_index_;I don't understand how this works with multiple bidder threads. It looks like all of the contexts we create are tied to a single thread. That's not ideal, given we try to spread the load over all threads.
Related details
- Abigail Katcoff
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abigail Katcoff (Gerrit)
Abigail Katcoff added 3 comments![]( "Open in Gerrit")
I think we need a higher level integration test to ensure that we correctly run auctions. I'm particularly concerned about interactions with the multi-threaded bidding feature.
Good idea -- do you think it's sufficient for me to make the unittests subclass
BidderWorkletMultiThreadingTest instead BidderWorkletTest?
fresh_context_recycler = std::move(unused_context_recyclers_.back());How do we ensure that the context is associated with the current thread?
unused_context_recyclers_ is a member of V8State which we always run with the same associated v8_runner.
size_t thread_index = next_thread_index_;I don't understand how this works with multiple bidder threads. It looks like all of the contexts we create are tied to a single thread. That's not ideal, given we try to spread the load over all threads.
I increment thread_index in the for loop, so the load is spread over all the threads.
Related details
- Russ Hamilton
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russ Hamilton (Gerrit)
Russ Hamilton added 3 comments![]( "Open in Gerrit")
Abigail KatcoffI think we need a higher level integration test to ensure that we correctly run auctions. I'm particularly concerned about interactions with the multi-threaded bidding feature.
Good idea -- do you think it's sufficient for me to make the unittests subclass
BidderWorkletMultiThreadingTest instead BidderWorkletTest?
Yeah, that sounds like a good idea to me.
fresh_context_recycler = std::move(unused_context_recyclers_.back());Abigail KatcoffHow do we ensure that the context is associated with the current thread?
unused_context_recyclers_ is a member of V8State which we always run with the same associated v8_runner.
Acknowledged
size_t thread_index = next_thread_index_;Abigail KatcoffI don't understand how this works with multiple bidder threads. It looks like all of the contexts we create are tied to a single thread. That's not ideal, given we try to spread the load over all threads.
I increment thread_index in the for loop, so the load is spread over all the threads.
- Abigail Katcoff
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abigail Katcoff (Gerrit)
Abigail Katcoff voted and added 1 comment![]( "Open in Gerrit")
Votes added by Abigail Katcoff
| Commit-Queue | +1 |
1 comment
I think we need a higher level integration test to ensure that we correctly run auctions. I'm particularly concerned about interactions with the multi-threaded bidding feature.
Russ HamiltonGood idea -- do you think it's sufficient for me to make the unittests subclass
BidderWorkletMultiThreadingTest instead BidderWorkletTest?
Yeah, that sounds like a good idea to me.
- Russ Hamilton
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Russ Hamilton (Gerrit)
Russ Hamilton voted Code-Review+1![]( "Open in Gerrit")
- Abigail Katcoff
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abigail Katcoff (Gerrit)
Abigail Katcoff added 1 comment![]( "Open in Gerrit")
@dcheng PTAL at blink/features. Thanks!
Related details
- Daniel Cheng
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted Code-Review+1![]( "Open in Gerrit")
- Abigail Katcoff
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abigail Katcoff (Gerrit)
Abigail Katcoff added 1 comment![]( "Open in Gerrit")
- Orr Bernstein
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orr Bernstein (Gerrit)
Orr Bernstein added 3 comments![]( "Open in Gerrit")
Thanks!
<histogram name="Ads.InterestGroup.Auction.NumPremadeContextsScheduled"Is this supposed to be `PremadeContextsScheduledPerThread`? Could you update the description as well, perhaps just 'Recorded when the context preparation tasks are posted in each bidder thread' (if that's correct)?
The number of contexts scheduled to be prepared before bids tasks are ready.s/bids/bid/?
```suggestion
The number of contexts scheduled to be prepared before bid tasks are ready.
```
Related details
- Abigail Katcoff
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abigail Katcoff (Gerrit)
Abigail Katcoff added 2 comments![]( "Open in Gerrit")
<histogram name="Ads.InterestGroup.Auction.NumPremadeContextsScheduled"Is this supposed to be `PremadeContextsScheduledPerThread`? Could you update the description as well, perhaps just 'Recorded when the context preparation tasks are posted in each bidder thread' (if that's correct)?
Yes! Thanks, I forgot to update this when I made the name change.
The number of contexts scheduled to be prepared before bids tasks are ready.s/bids/bid/?
```suggestion
The number of contexts scheduled to be prepared before bid tasks are ready.
```
- Orr Bernstein
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Orr Bernstein (Gerrit)
Orr Bernstein voted and added 1 comment![]( "Open in Gerrit")
Votes added by Orr Bernstein
| Code-Review | +1 |
Thanks!
- Abigail Katcoff
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Abigail Katcoff (Gerrit)
Abigail Katcoff voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Protected Audience: prepare contexts during trusted signals download
In the time between downloading bidding scripts and bidding signals,
prepare some contexts for later use by making the contexts,
running the top level script, and adding bindings. The worklet is
relatively idle during this time, and doing some preparation in
advance can improve the overall latency.
This can be further improved in a follow-up CL to make the contexts
before downloading the scripts, and running the top-level scripts
once we have them.
- M content/services/auction_worklet/bidder_worklet.cc
- M content/services/auction_worklet/bidder_worklet.h
- M content/services/auction_worklet/bidder_worklet_unittest.cc
- M testing/variations/fieldtrial_testing_config.json
- M third_party/blink/common/features.cc
- M third_party/blink/public/common/features.h
- M tools/metrics/histograms/metadata/others/histograms.xml
Code-Review: +1 by Orr Bernstein, +1 by Daniel Cheng, +1 by Russ Hamilton
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |