[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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
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.
We are moving towards weak coupling of members hence this base class is no longer needed.
Please see the discussion on for details:
https://chromium-review.googlesource.com/c/chromium/src/+/7239586/comment/34331345_78eef7e0/
Closing this thread but let me know if you have any doubts
Related details
- 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. |
Virali Purbey (Gerrit)
Virali Purbey added 2 comments![]( "Open in Gerrit")
void SVGDocumentResourceTracker::ClearTracker() {Divyansh MangalCan we add a DCHECK to see if `entries_` are empty?
Virali Purbey`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.
Done
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?
- Divyansh Mangal
- Fredrik Söderquist
- Vinay Singh
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
We shouldn't have anything "SVG" in `platform/`. Prefer weak coupling instead. It might also be possible to just observe garbage collection as well.
Thanks for letting me know about this, the only reason I needed the files in `platform` because a good place to clear the corresponding tracker that I could find was in `AgentGroupSchedulerImpl::Dispose` (when AgentGroupScheduler is getting disposed).
In the new patchset I have moved these new files from `platform` level to `core` and instead made the key in hashmap as a weakMember
```
HeapHashMap<WeakMember<AgentGroupScheduler>,
Member<SVGDocumentResourceTracker>> scheduler_tracker_map_;
```
and introduced a weak callback method `ProcessCustomWeakness` which calls the `ClearTracker` function during GC cycle. Let me know what you think of this and if any improvements are needed.
Related details
- Fredrik Söderquist
- Vinay Singh
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 14 comments![]( "Open in Gerrit")
// TODO(dmangal): Below constructor with cache identifier can be removed
// during feature flag removal.Couldn't we just keep using this one for both cases, and then do the folding when removing the flag? That avoids duplicating code.
void SVGDocumentResourceTracker::ClearTracker() {I think I'd call this `Dispose()` instead to better signal that the object is going away.
for (const auto& resource : tracked_resources_) {
resource->GetContent()->Dispose();
}I hope this will be fine to do during as part of the weak-callback, but this function does a lot of things, so it's difficult to say. A pre-finalizer potentially has similar issues though. So consider this a heads up if any issues arise.
auto task_runner =
page.GetPageScheduler()->GetAgentGroupScheduler().DefaultTaskRunner();
return *registry->GetOrCreateTrackerForScheduler(
&page.GetAgentGroupScheduler(), [task_runner]() {
return MakeGarbageCollected<SVGDocumentResourceTracker>(task_runner);Passing in the task-runner seems a bit unnecessary, because it's just `DefaultTaskRunner()` on the "key".
auto* registry = SVGResourceSchedulerRegistry::Get();
auto task_runner =
page.GetPageScheduler()->GetAgentGroupScheduler().DefaultTaskRunner();
return *registry->GetOrCreateTrackerForScheduler(
&page.GetAgentGroupScheduler(), [task_runner]() {
return MakeGarbageCollected<SVGDocumentResourceTracker>(task_runner);
});Why not fold this into `Page::GetSVGDocumentResourceTracker()`? Then the change becomes smaller, and we could even cache the reference there (and thus not need to do a hash-lookup everytime we need it).
SVGDocumentResourceTracker* tracker = GetTrackerForScheduler(scheduler);
if (!tracker) {
tracker = create_tracker();
RegisterSchedulerTracker(scheduler, tracker);
}Then this could be implemented using a single `insert()`.
// Returns the tracker for the given scheduler, creating one using the
// provided callback if it doesn't exist.What's the callback good for? It seems easier to just fold that into the function. The dependency is already there.
Also...
// Returns true if the given scheduler has an associated tracker.
bool HasTrackerForScheduler(AgentGroupScheduler* scheduler) const;Is this used? I see one use in a `DCHECK` - that could be replaced with checking a return value instead.
// Associates an AgentGroupScheduler with an SVGDocumentResourceTracker.
// If the scheduler is already registered, updates its tracker.
void RegisterSchedulerTracker(AgentGroupScheduler* scheduler,
SVGDocumentResourceTracker* tracker);This should be an internal detail - nothing should call this except `GetOrCreateTrackerForScheduler`.
static SVGResourceSchedulerRegistry* Get();Can return a reference. I think you could also just make `GetTrackerForScheduler()` static, and then not need to expose this here.
#include "third_party/blink/renderer/platform/wtf/static_constructors.h"You don't need this. `DEFINE_STATIC_LOCAL` is in `std_lib_extras.h`.
if (pair.value) {When would this be null?
for (const auto& scheduler : dead_schedulers) {
scheduler_tracker_map_.erase(scheduler);
}```suggestion
scheduler_tracker_map_.RemoveAll(dead_schedulers);
```
Divyansh MangalWe shouldn't have anything "SVG" in `platform/`. Prefer weak coupling instead. It might also be possible to just observe garbage collection as well.
Thanks for letting me know about this, the only reason I needed the files in `platform` because a good place to clear the corresponding tracker that I could find was in `AgentGroupSchedulerImpl::Dispose` (when AgentGroupScheduler is getting disposed).
In the new patchset I have moved these new files from `platform` level to `core` and instead made the key in hashmap as a weakMember
```
HeapHashMap<WeakMember<AgentGroupScheduler>,
Member<SVGDocumentResourceTracker>> scheduler_tracker_map_;
```
and introduced a weak callback method `ProcessCustomWeakness` which calls the `ClearTracker` function during GC cycle. Let me know what you think of this and if any improvements are needed.
How have you tested this? I _think_ thsi may need to use `UntracedMember` rather than `WeakMember`.
Related details
- Divyansh Mangal
- Vinay Singh
Divyansh Mangal (Gerrit)
Divyansh Mangal added 14 comments![]( "Open in Gerrit")
// TODO(dmangal): Below constructor with cache identifier can be removed
// during feature flag removal.Couldn't we just keep using this one for both cases, and then do the folding when removing the flag? That avoids duplicating code.
Done
I think I'd call this `Dispose()` instead to better signal that the object is going away.
Done
auto task_runner =
page.GetPageScheduler()->GetAgentGroupScheduler().DefaultTaskRunner();
return *registry->GetOrCreateTrackerForScheduler(
&page.GetAgentGroupScheduler(), [task_runner]() {
return MakeGarbageCollected<SVGDocumentResourceTracker>(task_runner);Passing in the task-runner seems a bit unnecessary, because it's just `DefaultTaskRunner()` on the "key".
Folded this into `Page::GetSVGDocumentResourceTracker()` so this no longer was needed.
auto* registry = SVGResourceSchedulerRegistry::Get();
auto task_runner =
page.GetPageScheduler()->GetAgentGroupScheduler().DefaultTaskRunner();
return *registry->GetOrCreateTrackerForScheduler(
&page.GetAgentGroupScheduler(), [task_runner]() {
return MakeGarbageCollected<SVGDocumentResourceTracker>(task_runner);
});Why not fold this into `Page::GetSVGDocumentResourceTracker()`? Then the change becomes smaller, and we could even cache the reference there (and thus not need to do a hash-lookup everytime we need it).
Done
*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,
Divyansh MangalThe 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.
Done
SVGDocumentResourceTracker* tracker = GetTrackerForScheduler(scheduler);
if (!tracker) {
tracker = create_tracker();
RegisterSchedulerTracker(scheduler, tracker);
}Then this could be implemented using a single `insert()`.
Used `Set` in the simplified code
// Returns the tracker for the given scheduler, creating one using the
// provided callback if it doesn't exist.What's the callback good for? It seems easier to just fold that into the function. The dependency is already there.
Also...
No longer needed as we are now in `core` directory
// Returns true if the given scheduler has an associated tracker.
bool HasTrackerForScheduler(AgentGroupScheduler* scheduler) const;Is this used? I see one use in a `DCHECK` - that could be replaced with checking a return value instead.
Done
// Associates an AgentGroupScheduler with an SVGDocumentResourceTracker.
// If the scheduler is already registered, updates its tracker.
void RegisterSchedulerTracker(AgentGroupScheduler* scheduler,
SVGDocumentResourceTracker* tracker);This should be an internal detail - nothing should call this except `GetOrCreateTrackerForScheduler`.
Done
static SVGResourceSchedulerRegistry* Get();Can return a reference. I think you could also just make `GetTrackerForScheduler()` static, and then not need to expose this here.
I tried making it static (as per the current Patchset `GetOrCreateTrackerForScheduler` was tried to made static) but it uses the instance variable scheduler_tracker_map_ so got compiler errors.
I have still kept the `SVGResourceSchedulerRegistry::Get()` cause making instance variable scheduler_tracker_map_ static seemed incorrect, but let me know what you think of this?
#include "third_party/blink/renderer/platform/wtf/static_constructors.h"You don't need this. `DEFINE_STATIC_LOCAL` is in `std_lib_extras.h`.
Done
When would this be null?
You are right, this shouldn't be null, probably a DCHECK make more sense here,
for (const auto& scheduler : dead_schedulers) {
scheduler_tracker_map_.erase(scheduler);
}Divyansh Mangal```suggestion
scheduler_tracker_map_.RemoveAll(dead_schedulers);
```
Done
Divyansh MangalWe shouldn't have anything "SVG" in `platform/`. Prefer weak coupling instead. It might also be possible to just observe garbage collection as well.
Fredrik SöderquistThanks for letting me know about this, the only reason I needed the files in `platform` because a good place to clear the corresponding tracker that I could find was in `AgentGroupSchedulerImpl::Dispose` (when AgentGroupScheduler is getting disposed).
In the new patchset I have moved these new files from `platform` level to `core` and instead made the key in hashmap as a weakMember
```
HeapHashMap<WeakMember<AgentGroupScheduler>,
Member<SVGDocumentResourceTracker>> scheduler_tracker_map_;
```
and introduced a weak callback method `ProcessCustomWeakness` which calls the `ClearTracker` function during GC cycle. Let me know what you think of this and if any improvements are needed.
How have you tested this? I _think_ thsi may need to use `UntracedMember` rather than `WeakMember`.
I mainly did manual testing locally, based on the examples in bugs:
https://issues.chromium.org/issues/360599258
https://issues.chromium.org/issues/459746761
(Also did testing when RenderDocument is enabled)
I have also used `UntracedMember` in the latest patchset
Related details
- 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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
for (const auto& resource : tracked_resources_) {
resource->GetContent()->Dispose();
}Divyansh MangalI hope this will be fine to do during as part of the weak-callback, but this function does a lot of things, so it's difficult to say. A pre-finalizer potentially has similar issues though. So consider this a heads up if any issues arise.
Thanks for the heads‑up, I mainly relied on the existing tests in svg_resource_document_content_test.cc along with some manual testing across a few scenarios. I agree that this may not cover every edge case, but I’ll keep a close eye on things and follow up if any issues surface after the merge of this change
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 2 comments![]( "Open in Gerrit")
static SVGResourceSchedulerRegistry* Get();Divyansh MangalCan return a reference. I think you could also just make `GetTrackerForScheduler()` static, and then not need to expose this here.
I tried making it static (as per the current Patchset `GetOrCreateTrackerForScheduler` was tried to made static) but it uses the instance variable scheduler_tracker_map_ so got compiler errors.
I have still kept the `SVGResourceSchedulerRegistry::Get()` cause making instance variable scheduler_tracker_map_ static seemed incorrect, but let me know what you think of this?
How to make this static:
.h file:
```
...
static SVGDocumentResourceTracker* GetTracker(AgentGroupScheduler&);
...
```
.cc file:
```
...
namespace {
class RegistryHolder : public GarbageCollected<RegistryHolder> {
public:
...
MapType& GetMap() { ... }
...private:
MapType map_;
};
MapType& GetTrackerMap() {
DCHECK(IsMainThread());
DEFINE_STATIC_LOCAL(Persistent<RegistryHolder>, holder,
(MakeGarbageCollected<RegistryHolder>()));
return registry->GetMap();
}} // namespace
SVGDocumentResourceTracker*
SVGResourceSchedulerRegistry::GetTracker(AgentGroupScheduler& scheduler) {
MapType& map = GetTrackerMap();
auto it = map.find(&scheduler);
...
}
...
```
Divyansh MangalWe shouldn't have anything "SVG" in `platform/`. Prefer weak coupling instead. It might also be possible to just observe garbage collection as well.
Fredrik SöderquistThanks for letting me know about this, the only reason I needed the files in `platform` because a good place to clear the corresponding tracker that I could find was in `AgentGroupSchedulerImpl::Dispose` (when AgentGroupScheduler is getting disposed).
In the new patchset I have moved these new files from `platform` level to `core` and instead made the key in hashmap as a weakMember
```
HeapHashMap<WeakMember<AgentGroupScheduler>,
Member<SVGDocumentResourceTracker>> scheduler_tracker_map_;
```
and introduced a weak callback method `ProcessCustomWeakness` which calls the `ClearTracker` function during GC cycle. Let me know what you think of this and if any improvements are needed.
Divyansh MangalHow have you tested this? I _think_ thsi may need to use `UntracedMember` rather than `WeakMember`.
I mainly did manual testing locally, based on the examples in bugs:
https://issues.chromium.org/issues/360599258
https://issues.chromium.org/issues/459746761
(Also did testing when RenderDocument is enabled)I have also used `UntracedMember` in the latest patchset
Did you verify that cleanup happened as expected? (`Dispose()` being called etc.)
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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 2 comments![]( "Open in Gerrit")
static SVGResourceSchedulerRegistry* Get();Thanks for the awesome suggestion!, I incorporated the same in the registry.
Divyansh MangalWe shouldn't have anything "SVG" in `platform/`. Prefer weak coupling instead. It might also be possible to just observe garbage collection as well.
Fredrik SöderquistThanks for letting me know about this, the only reason I needed the files in `platform` because a good place to clear the corresponding tracker that I could find was in `AgentGroupSchedulerImpl::Dispose` (when AgentGroupScheduler is getting disposed).
In the new patchset I have moved these new files from `platform` level to `core` and instead made the key in hashmap as a weakMember
```
HeapHashMap<WeakMember<AgentGroupScheduler>,
Member<SVGDocumentResourceTracker>> scheduler_tracker_map_;
```
and introduced a weak callback method `ProcessCustomWeakness` which calls the `ClearTracker` function during GC cycle. Let me know what you think of this and if any improvements are needed.
Divyansh MangalHow have you tested this? I _think_ thsi may need to use `UntracedMember` rather than `WeakMember`.
Fredrik SöderquistI mainly did manual testing locally, based on the examples in bugs:
https://issues.chromium.org/issues/360599258
https://issues.chromium.org/issues/459746761
(Also did testing when RenderDocument is enabled)I have also used `UntracedMember` in the latest patchset
Did you verify that cleanup happened as expected? (`Dispose()` being called etc.)
Yes, `Dispose()` indeed gets called correctly when I changed to `UntracedMember`, I checked by adding breakpoints for the tests in third_party\blink\renderer\core\svg\svg_resource_document_content_test.cc
Related details
- 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 voted and added 3 comments![]( "Open in Gerrit")
Votes added by Fredrik Söderquist
| Code-Review | +1 |
3 comments
: public GarbageCollected<SVGResourceSchedulerRegistry> {You don't need this now (ditto for the `Trace()`.
Divyansh MangalWe shouldn't have anything "SVG" in `platform/`. Prefer weak coupling instead. It might also be possible to just observe garbage collection as well.
Fredrik SöderquistThanks for letting me know about this, the only reason I needed the files in `platform` because a good place to clear the corresponding tracker that I could find was in `AgentGroupSchedulerImpl::Dispose` (when AgentGroupScheduler is getting disposed).
In the new patchset I have moved these new files from `platform` level to `core` and instead made the key in hashmap as a weakMember
```
HeapHashMap<WeakMember<AgentGroupScheduler>,
Member<SVGDocumentResourceTracker>> scheduler_tracker_map_;
```
and introduced a weak callback method `ProcessCustomWeakness` which calls the `ClearTracker` function during GC cycle. Let me know what you think of this and if any improvements are needed.
Divyansh MangalHow have you tested this? I _think_ thsi may need to use `UntracedMember` rather than `WeakMember`.
Fredrik SöderquistI mainly did manual testing locally, based on the examples in bugs:
https://issues.chromium.org/issues/360599258
https://issues.chromium.org/issues/459746761
(Also did testing when RenderDocument is enabled)I have also used `UntracedMember` in the latest patchset
Divyansh MangalDid you verify that cleanup happened as expected? (`Dispose()` being called etc.)
Yes, `Dispose()` indeed gets called correctly when I changed to `UntracedMember`, I checked by adding breakpoints for the tests in third_party\blink\renderer\core\svg\svg_resource_document_content_test.cc
- 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. |
Divyansh Mangal (Gerrit)
Divyansh Mangal voted and added 1 comment![]( "Open in Gerrit")
Votes added by Divyansh Mangal
| Commit-Queue | +1 |
1 comment
: public GarbageCollected<SVGResourceSchedulerRegistry> {You don't need this now (ditto for the `Trace()`.
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
31 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/svg/svg_resource_scheduler_registry.h
Insertions: 1, Deletions: 4.
@@ -18,8 +18,7 @@
// AgentGroupScheduler instances and their corresponding
// SVGDocumentResourceTracker instances. This ties the SVG resource lifecycle
// with the AgentGroupScheduler.
-class CORE_EXPORT SVGResourceSchedulerRegistry final
- : public GarbageCollected<SVGResourceSchedulerRegistry> {
+class CORE_EXPORT SVGResourceSchedulerRegistry final {
public:
SVGResourceSchedulerRegistry() = default;
~SVGResourceSchedulerRegistry() = default;
@@ -31,8 +30,6 @@
// Returns the tracker for the given scheduler, creating one if the tracker
// doesn't exist.
static SVGDocumentResourceTracker* GetTracker(AgentGroupScheduler& scheduler);
-
- void Trace(Visitor*) const {}
};
} // namespace blink
```
Change information
[SVG] Introduce a registry for managing `AgentGroupScheduler` and
`SVGDocumentResourceTracker`
In CL:6807690, we transitioned SVG resource handling from a local
cache-based architecture to leverage the existing `MemoryCache` data
structure. This resolved issues where SVG resources were not
refreshed on reload, under the assumption that reloads would not
create new `Page` objects.
However, CL:6965528 introduced the `RenderDocument` feature, which
fundamentally changed the navigation framework. With
`RenderDocument`, every cross-document navigation—including
reloads creates a new Page (along with a new LocalFrame,
RenderFrame, WebView, etc.) upon committing navigation. This
invalidated our previous architecture and introduced race conditions
during reloads due to deletion of `Page` and associated resources.
This CL re-architects SVG caching to eliminate the dependency on
`Page` for resource scheduling. Since `Page` previously acted as the
provider of `AgentGroupScheduler` to `SVGDocumentResource`, we now
introduce a dedicated registry to maintain this relationship
directly.
Key changes in this CL:
1) Introduce SVGResourceSchedulerRegistry
2) Redirect all call sites where `Page` currently provides
`AgentGroupScheduler`.
3) Cleanup of `SVGDocumentResourceTracker` is now the responsibility
of `SVGResourceSchedulerRegistry`.
4) Tests are updated to use `SVGResourceSchedulerRegistry`.
Design doc: https://docs.google.com/document/d/1BF027AlGjIocPblyxi-KGaG15YvBPCNUOW81P9BWSpE/edit?usp=sharing
- M third_party/blink/renderer/core/page/page.cc
- M third_party/blink/renderer/core/svg/build.gni
- M third_party/blink/renderer/core/svg/svg_document_resource_tracker.cc
- M third_party/blink/renderer/core/svg/svg_document_resource_tracker.h
- A third_party/blink/renderer/core/svg/svg_resource_scheduler_registry.cc
- A third_party/blink/renderer/core/svg/svg_resource_scheduler_registry.h
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
Code-Review: +1 by Fredrik Söderquist
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |