[SVG] Introduce a registry for managing `AgentGroupScheduler` and `SVGDocumentResourceTracker` [chromium/src : main]
Divyansh Mangal (Gerrit)
Divyansh Mangal added 2 comments![]( "Open in Gerrit")
`SVGDocumentResourcTracker`Divyansh Mangalnit: `SVGDocumentResourceTracker`
Done
3) Cleanup of `SVGDocumentResourcTracker` is now the responsibilityDivyansh Mangalnit: `SVGDocumentResourceTracker`
Done
Related details
- Vinay Singh
- Virali Purbey
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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 8 comments![]( "Open in Gerrit")
// TODO(dmangal): Below constructor with cache identifier can be removed when
// feature `SvgPartitionSVGDocumentResourcesInMemoryCache` is fully launched.Divyansh MangalThe feature flag is marked as stable and still we mention we will remove this once feature is fully launched. Can you please add more details about this in the description?
Apologies for the confusion, I meant it would be safe to remove the constructor during feature flag removal (after two releases). There's a similar TODO at
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/svg/svg_document_resource_tracker.h;drc=0047c18af00c7f8cb2e93d6084158e70720b5ed5;l=71
(I don't think this is needed to be mentioned in the description for this CL, but I have rephrased the comment to better reflect this understanding)
String GenerateGUID() {Divyansh MangalCan we rename it as `GenerateCacheIdentifier()`?
Sounds good
void SVGDocumentResourceTracker::ClearTracker() {Divyansh MangalCan we add a DCHECK to see if `entries_` are empty?
`entries_` member will eventually be removed (during feature flag removal), and it will definitely be empty when feature flag is on, so I think this check will be redundant, but let me know if you think otherwise.
if (!registry->HasTrackerForScheduler(agent_group_scheduler)) {Divyansh MangalWe always have to check if the scheduler has tracker, return it otherwise create it. Can we move this logic to registry itself to avoid duplicate code and call it `GetOrCreateTrackerForScheduler()`?
This was a little tricky due to the dependency problem discussed in https://chromium-review.googlesource.com/c/chromium/src/+/7239586/comment/b75d9fdf_5a3d4509/
but a template function came handy here, let me know what you think of this?
*SVGResourceSchedulerRegistry::Get()->GetTrackerForScheduler(Divyansh MangalSince this is a singleton class, can we have `ClearForTesting()` to clear scheduler_tracker_map_ (guarded under test build flags) or provide a way to destroy/recreate the singleton in tests.
`SVGResourceSchedulerRegistry` is garbage collected so I believe it should clear the memory during destruction,
The main reason I don't want to create explicit `ClearForTesting` was to make sure that we are clearing/unregistring the tracker at the correct place. Currently the unregistration is done at `AgentGroupScheduler::Dispose()`, if the tests require such clearing explicitly they can be added but for now i believe it should be ok to have this as-is. Let me know if you want to discuss on this further.
SVGResourceSchedulerRegistry* SVGResourceSchedulerRegistry::Get() {Divyansh MangalCan we add a `DCHECK(IsMainThread())` in these functions (`Get()`, `RegisterSchedulerTracker()`, `UnregisterScheduler()`, `GetTrackerForScheduler()`) as we will be calling these from main thread?
Done
void SVGResourceSchedulerRegistry::RegisterSchedulerTracker(Divyansh MangalI know we are checking if `scheduler_tracker_map_` contains has any tracker for scheduler but can we add a DCHECK ` DCHECK(!scheduler_tracker_map_.Contains(scheduler))` to make sure that we are never overridding the value?
Done
class PLATFORM_EXPORT SVGDocumentResourceTrackerBaseDivyansh MangalWas there a specific need to create a base class here?
`svg_dcoument_resource_tracker` is in `core` directory whereas our new class `svg_resource_scheduler_registry` is at `platform` directory, by design
no class of `core` can be used in `platform` (it ends up created cyclic dependencies and linking errors). But vice-versa, that is classes in `platform` can be freely used in `core`.
As such to go around this problem, I created this base class, and made the already exisiting `svg_document_resource_tracker` inherited from it, so that
`AgentGroupScheduler` and `SVGDocumentResourceTrackerBase` can instead be used as key-value pair in registry without any linking errors.
This was one way to go around the dependency issue that I could think of, let me know if you want to discuss on this.
Related details
- Vinay Singh
- Virali Purbey
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. |
Vinay Singh (Gerrit)
Vinay Singh added 5 comments![]( "Open in Gerrit")
registry->GetOrCreateTrackerForScheduler(
agent_group_scheduler, [&page]() {
return MakeGarbageCollected<SVGDocumentResourceTracker>(
page->GetPageScheduler()
->GetAgentGroupScheduler()
.DefaultTaskRunner());```
auto task_runner = page->GetPageScheduler()
->GetAgentGroupScheduler()
.DefaultTaskRunner();
registry->GetOrCreateTrackerForScheduler(
agent_group_scheduler,
[task_runner]() {
return MakeGarbageCollected<SVGDocumentResourceTracker>(task_runner);
});
```
Looks slightly cleaner.
Also, `GetOrCreateTrackerForScheduler` is no longer susceptible to hold a lamba with page reference param.
auto& cache = *tracker;Let's keep the `tracker` only. No need of additional `cache` variable.
if (SVGDocumentResourceTrackerBase* tracker =
GetTrackerForScheduler(scheduler)) {
return tracker;
}
SVGDocumentResourceTrackerBase* new_tracker = create_tracker();
RegisterSchedulerTracker(scheduler, new_tracker);
return new_tracker;```suggestion
SVGDocumentResourceTrackerBase* tracker = GetTrackerForScheduler(scheduler)
if (!tracker) {
tracker = create_tracker();
RegisterSchedulerTracker(scheduler, tracker );
}
return tracker;
```
// SVGDocumentResourceTracker instances. This enables coordinated resource
// loading and scheduling for SVG documents within each agent group.Consider "This ties the SVG resource lifecycle with the AgentGroupScheduler."
Feel free to use your judgement though.
return it != scheduler_tracker_map_.end() ? it->value : nullptr;Make sure the code has enough defensive checks in case this returns `nullptr`
Related details
- Divyansh Mangal
- Virali Purbey
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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 5 comments![]( "Open in Gerrit")
registry->GetOrCreateTrackerForScheduler(
agent_group_scheduler, [&page]() {
return MakeGarbageCollected<SVGDocumentResourceTracker>(
page->GetPageScheduler()
->GetAgentGroupScheduler()
.DefaultTaskRunner());```
auto task_runner = page->GetPageScheduler()
->GetAgentGroupScheduler()
.DefaultTaskRunner();
registry->GetOrCreateTrackerForScheduler(
agent_group_scheduler,
[task_runner]() {
return MakeGarbageCollected<SVGDocumentResourceTracker>(task_runner);
});
```Looks slightly cleaner.
Also, `GetOrCreateTrackerForScheduler` is no longer susceptible to hold a lamba with page reference param.
Done
Let's keep the `tracker` only. No need of additional `cache` variable.
Done
if (SVGDocumentResourceTrackerBase* tracker =
GetTrackerForScheduler(scheduler)) {
return tracker;
}
SVGDocumentResourceTrackerBase* new_tracker = create_tracker();
RegisterSchedulerTracker(scheduler, new_tracker);
return new_tracker;```suggestion
SVGDocumentResourceTrackerBase* tracker = GetTrackerForScheduler(scheduler)
if (!tracker) {
tracker = create_tracker();
RegisterSchedulerTracker(scheduler, tracker );
}
return tracker;
```
Done
// SVGDocumentResourceTracker instances. This enables coordinated resource
// loading and scheduling for SVG documents within each agent group.Consider "This ties the SVG resource lifecycle with the AgentGroupScheduler."
Feel free to use your judgement though.
Done
return it != scheduler_tracker_map_.end() ? it->value : nullptr;Make sure the code has enough defensive checks in case this returns `nullptr`
- Vinay Singh
- Virali Purbey
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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal voted Commit-Queue+1![]( "Open in Gerrit")
- Fredrik Söderquist
- Vinay Singh
- Virali Purbey
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. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 4 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::
SvgPartitionSVGDocumentResourcesInMemoryCacheEnabled()) {
AgentGroupScheduler* agent_group_scheduler =
&page->GetAgentGroupScheduler();
auto* registry = SVGResourceSchedulerRegistry::Get();
auto task_runner =
page->GetPageScheduler()->GetAgentGroupScheduler().DefaultTaskRunner();
tracker = static_cast<SVGDocumentResourceTracker*>(
registry->GetOrCreateTrackerForScheduler(
agent_group_scheduler, [task_runner]() {
return MakeGarbageCollected<SVGDocumentResourceTracker>(
task_runner);
}));
} else {
tracker = &page->GetSVGDocumentResourceTracker();
}You could use a technique similar to what you do in the test here as well. To allow keeping it a reference.
We shouldn't have anything "SVG" in `platform/`. Prefer weak coupling instead. It might also be possible to just observe garbage collection as well.
static Persistent<SVGResourceSchedulerRegistry>*
g_svg_resource_scheduler_registry;Use `DEFINE_STATIC_LOCAL` in function scope instead if you need a global like this.
if (!HasTrackerForScheduler(scheduler)) {
return;
}
GetTrackerForScheduler(scheduler)->ClearTracker();
scheduler_tracker_map_.erase(scheduler);This has three hash-lookups when it could have one.
Related details
- Divyansh Mangal
- Vinay Singh
- Virali Purbey
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1755067b710000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12b9a960f10000
Related details
- Divyansh Mangal
- Vinay Singh
- Virali Purbey
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. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/11c4a3ad710000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/12909d64f10000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/173c99ad710000
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/speedometer3 complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/17f48e54f10000
Divyansh Mangal (Gerrit)
Divyansh Mangal added 3 comments![]( "Open in Gerrit")
if (RuntimeEnabledFeatures::
SvgPartitionSVGDocumentResourcesInMemoryCacheEnabled()) {
AgentGroupScheduler* agent_group_scheduler =
&page->GetAgentGroupScheduler();
auto* registry = SVGResourceSchedulerRegistry::Get();
auto task_runner =
page->GetPageScheduler()->GetAgentGroupScheduler().DefaultTaskRunner();
tracker = static_cast<SVGDocumentResourceTracker*>(
registry->GetOrCreateTrackerForScheduler(
agent_group_scheduler, [task_runner]() {
return MakeGarbageCollected<SVGDocumentResourceTracker>(
task_runner);
}));
} else {
tracker = &page->GetSVGDocumentResourceTracker();
}You could use a technique similar to what you do in the test here as well. To allow keeping it a reference.
Done
static Persistent<SVGResourceSchedulerRegistry>*
g_svg_resource_scheduler_registry;Use `DEFINE_STATIC_LOCAL` in function scope instead if you need a global like this.
Done
if (!HasTrackerForScheduler(scheduler)) {
return;
}
GetTrackerForScheduler(scheduler)->ClearTracker();
scheduler_tracker_map_.erase(scheduler);This has three hash-lookups when it could have one.
- Fredrik Söderquist
- Vinay Singh
- Virali Purbey
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. |