[soft navs] Use a prefinalizer instead of UntracedMember for SoftNavigationContext [chromium/src : main]
Scott Haseley (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
// wasn't shut down by the associated `SoftNavigationHeuritics`, which happensnit: Fix typos in comments: 'SoftNavigationHeuritics' -> 'SoftNavigationHeuristics' and 'isn'st' -> 'isn't'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Michal Mocny
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. |
Scott Haseley (Gerrit)
Scott Haseley added 1 comment![]( "Open in Gerrit")
// wasn't shut down by the associated `SoftNavigationHeuritics`, which happensnit: Fix typos in comments: 'SoftNavigationHeuritics' -> 'SoftNavigationHeuristics' and 'isn'st' -> 'isn't'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
Related details
- Michal Mocny
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michal Mocny (Gerrit)
Michal Mocny added 2 comments![]( "Open in Gerrit")
}Would we report less histograms (for the failed-to-emit cases) with these guards, and the timing of Dispose now vs prior?
I think it's fine to break these but should consider what type of reporting will be useful in the future.
Perhaps just:
- #context-created (should eventually == #interactions?)
- #icp-reported
- #soft-nav-emitted
Perhaps would suffice. We could count exhausted context by comparing counts.
(We might want to report the status of active un-emitted context at document unload for the cases that are still active)
potential_soft_navigations_.erase(iter);My understanding was that this automatically happens after dispose for HeapHashSet of pure weakness members?
Related details
- Scott Haseley
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. |
Michal Mocny (Gerrit)
Michal Mocny voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michal Mocny
| Code-Review | +1 |
1 comment
( I think we could follow with removing most of the histograms for the exhausted case )
Related details
- Scott Haseley
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. |
Scott Haseley (Gerrit)
Scott Haseley added 2 comments![]( "Open in Gerrit")
Would we report less histograms (for the failed-to-emit cases) with these guards, and the timing of Dispose now vs prior?
I think it's fine to break these but should consider what type of reporting will be useful in the future.
Perhaps just:
- #context-created (should eventually == #interactions?)
- #icp-reported
- #soft-nav-emitted
Perhaps would suffice. We could count exhausted context by comparing counts.
(We might want to report the status of active un-emitted context at document unload for the cases that are still active)
It shouldn't change the counts:
- `window_ == null` --> Shutdown() was called, and the heuristics class flushes metrics for all pending contexts at that point.
- `heuristics == null` --> unit test. I don't think this can happen outside of tests if the context hasn't been shut down.
My understanding was that this automatically happens after dispose for HeapHashSet of pure weakness members?
Oh, duh, forgot I changed this to be a weak set. Thanks. Removed the erase call.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 1 comment![]( "Open in Gerrit")
Votes added by Scott Haseley
| Commit-Queue | +2 |
1 comment
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc
Insertions: 1, Deletions: 3.
The diff is too large to show. Please review the diff.
```
Change information
[soft navs] Use a prefinalizer instead of UntracedMember for SoftNavigationContext
SoftNavigationHeuristics does custom tracing of the set of contexts
using UntracedMember. This used to be how we detect the set becoming
empty, but we don't track this any longer. We do still, however, record
some metrics and end a trace event, so I think we still need detect
when soft nav contexts are disposed.
This CL changes how we do this to use a prefinalizer instead, which is
generally preferred to UntracedMember. This is being done to make set
iteration safer, since HashSet entry erasure invalidates iterators,
which would be a problem if a set interaction causes a GC, e.g. due to
an allocation. This is a prerequisite for crrev.com/c/7120640, where I
don't trust that an iteration I need to add is safe.
- M third_party/blink/renderer/core/timing/soft_navigation_context.cc
- M third_party/blink/renderer/core/timing/soft_navigation_context.h
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics.cc
- M third_party/blink/renderer/core/timing/soft_navigation_heuristics.h
Code-Review: +1 by Michal Mocny
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |