[SVG] Put SVG document resources in a separate `MemoryCache` partition [chromium/src : main]
Divyansh Mangal (Gerrit)
Divyansh Mangal added 18 comments![]( "Open in Gerrit")
[SVG] Introduce Page level cache identifier for `MemoryCache` for
SVG resourcesDivyansh MangalHow about something more along the lines of:
> Put SVG document resources in a separate MemoryCache partitionand then expand on that in the non-title part?
Done
When an external SVG resource is updated, the cached version
persists. Even reloading the page does not reflect the updatedDivyansh Mangal"...no new request is made"
Done
identifier. The unique Id is genereted usingDivyansh Mangaltypo
Done
Change-Id: I6521e438713a36e491b841ac7b265b247f4b751fDivyansh MangalBug#?
Done
String(BrowsingContextGroupToken().ToString()));Divyansh MangalDoes this need to be wrapped in a `String`?
Fredrik SöderquistYes, because `BrowsingContextGroupToken().ToString()` returns `std::string` and there was no implicit conversion between `String` and `std::string`.
Divyansh MangalMaybe we could add a prefix to this identifier to make it a bit more "discoverable" - something like `"svg-resources:<token-string>"` perhaps?
I have hardcoded the string currently, but I believe we generally create a corresponding string somewhere else?
String svg_cache_identifier_;Divyansh MangalThis can be dropped (quite obvious from the class name).
Done
const String& GetSVGCacheIdentifier() const { return svg_cache_identifier_; }Divyansh MangalRedundant
Done
void AddStrongReference(SVGDocumentResource* resource);Divyansh Mangal`AddResource`
Done
HeapLinkedHashSet<Member<SVGDocumentResource>> strong_references_;Divyansh MangalDoes this need to be a `HeapLinkedHashSet` vs just a `HeapHashMap`? Looks like the only real difference is that `HeapLinkedHashSet` maintains the order of insertion, but is that necessary?
Kurt Catti-Schmidt`SVGResourceDocumentCache` will no longer be a cache, it will be a Tracker sort of thing (this will be subsequent renaming task corresponding to that) to keep track of the strong references of fetched SVG resources. So, we definitely no longer need a `HeapHashMap`. We need a data structure that can maintain a list of references effectively.
Among the data structures, I picked `HeapLinkedHashSet` because `MemoryCache` also uses the same to store strong references. Granted, in `MemoryCache` there is some logic corresponding to LRU and that's why `LinkedHashSet` makes sense there, we currently don't have any need for LRU logic here but in the future, it may arise so picking this data structure seemed ok to me. Other than this I had option for `HeapVector` but did not had any strong reason to pick that up.
Divyansh MangalIn general, you want to use the most performant data structure at any given time. So if you need LRU in the future, you can implement and test it separately in the future and know the exact cost of a given data structure.
`HeapVector` would have a higher CPU + memory cache rate (due to sequential access) and less memory overhead, but you do lookups on `strong_references_` via `Contains`, and perform `erase`, both of which are O(N) for vectors. So a `HeapHashMap` will be a better choice here, as it won't have the additional linked list to maintain that `HeapLinkedHashSet` does.
Fredrik SöderquistI think you are right in that case, since I am using `Contains` or `erase` like functions `HeapHashMap` might be better. I guess I was worried about what Key should be taken, but I can take what we were earlier using anyway, @f...@opera.com, do you also think `HeapHashMap` is better here?
Divyansh MangalI would say `HeapHashSet`, but yes, no need for a linked set that I can see. I'd also call this field something else than `strong_references_` - perhaps `tracked_resources_`?
I have used `HeapHashSet` as per this discussion and renamed the variable.
if (resource) {Divyansh MangalWe shouldn't allow nulls to be added to the set in the first place. (And this is making different assumptions than the other iterations.)
Done
for (const auto& resource : to_remove) {
strong_references_.erase(resource);
}Divyansh Mangal`HashSet` has a `RemoveAll()`.
Done
const bool all_strong_references_are_observed = std::ranges::all_of(Divyansh Mangal```suggestion
const bool all_resources_are_observed = std::ranges::all_of(
```
Done
if (strong_references_.Contains(resource)) {
return;
}
strong_references_.AppendOrMoveToLast(resource);Divyansh MangalThis could just be a single `insert()` (assuming we go with a `HashSet`).
Done
for (const auto& resource : strong_references_) {
if (resource->GetContent() == content) {
return true;
}
}
return false;Divyansh Mangal`any_of()`?
Done
if (RuntimeEnabledFeatures::SVGPageLevelCacheIdentifierEnabled() &&
notified_target_observer_) {
return;
}Divyansh MangalThis looks like a bodge. This needs to be explained much better (this isn't even mentioned in the commit message). The naming of this field is also poor.
@f...@opera.com I agree on the ambiguity of this variable, let me try to explain why it was needed:
Consider your html has two <use> element both referencing the same resource. When first <use> fetches the resource and completion happens it will call [1] and notify itself. Then when the second <use> fetches the resources and completion happens it will notify all observers that are observing this resource. So it notifies, first <use> again and itself. This resulted in the first <use> element being notified twice which was not needed (and the DCHECK() at L:665 gets hit).
This behaviour was observed during the testing of WPT [2]
I wanted to use the active status of `pending_event_` to do the check instead but was not sure if that's a good fix given the DCHECK at L:665
ResourceMap* resources = resource_maps_it->value.Get();
for (auto resource_iter = resources->begin();
resource_iter != resources->end(); resource_iter = resources->begin()) {
DCHECK(resource_iter.Get());
DCHECK(resource_iter->value.Get());
DCHECK(resource_iter->value->GetResource());
Resource* resource = resource_iter->value->GetResource();
DCHECK(resource);
RemoveInternal(resources, resource_iter);
}
resource_maps_.erase(resource_maps_it);Divyansh MangalThis is the same as in `EvictResources()`. Could we factor out that bit instead an dreuse?
Done
const String cache_identifier =
RuntimeEnabledFeatures::SVGPageLevelCacheIdentifierEnabled() &&
!params.GetSVGCacheIdentifier().empty()
? params.GetSVGCacheIdentifier()
: GetCacheIdentifier(
params.GetResourceRequest().Url(),
params.GetResourceRequest().GetSkipServiceWorker());Divyansh MangalRather than putting more "gunk" into `FetchParameters`, I think we could get this identifier via the `FetchContext` (and use `factory.GetType()` to limit those calls).
Also, I think we could put this logic in an overload of `GetCacheIdentifier()`.
I added the setter and getter in `FetchContext` and removed the corresponding from `FetchParameters`. The overloaded is also introduced in the new PatchSet.
name: "SVGPageLevelCacheIdentifier",Divyansh MangalIsn't more like "SvgDocumentResourcesInMemoryCache"(Partition)?
I have updated it to `PartitionSvgDocumentResourcesInMemoryCache`
Related details
- Fredrik Söderquist
- Kurt Catti-Schmidt
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
Data(), response.CurrentRequestUrl(), HasSuccessfulRevalidation());Divyansh MangalI think we could just skip a large part of this function if this is true - i.e there should be no need to update the content in any way.
I have already skipped some part of `UpdateDocument` already, did you mean there are other parts in `SVGDocumentResource::Finish` that we could skip?
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job mac-m1_mini_2020-perf/motionmark1.3.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/166be82a510000
Related details
- Divyansh Mangal
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 12 comments![]( "Open in Gerrit")
Data(), response.CurrentRequestUrl(), HasSuccessfulRevalidation());Divyansh MangalI think we could just skip a large part of this function if this is true - i.e there should be no need to update the content in any way.
I have already skipped some part of `UpdateDocument` already, did you mean there are other parts in `SVGDocumentResource::Finish` that we could skip?
The resource hasn't changed, so there shouldn't be a need to re-run that code - and why should we bother that code with this knowledge at all? Similarly here we don't want to potentially cause more side-effects than needed.
String(BrowsingContextGroupToken().ToString()));Divyansh MangalDoes this need to be wrapped in a `String`?
Fredrik SöderquistYes, because `BrowsingContextGroupToken().ToString()` returns `std::string` and there was no implicit conversion between `String` and `std::string`.
Divyansh MangalMaybe we could add a prefix to this identifier to make it a bit more "discoverable" - something like `"svg-resources:<token-string>"` perhaps?
I have hardcoded the string currently, but I believe we generally create a corresponding string somewhere else?
`SVGResourceDocumentCache` (later `...Tracker`) ought to be only entity that needs to care about hot the id is constructed, so in that sense it might be good to move the generation there. If it generated it internally it might've been even better.
document.Fetcher()->Context().SetSVGCacheIdentifier(
page->GetSVGResourceDocumentCache().GetCacheIdentifier());Unnecessary.
WebFeature::kPartitionSVGDocumentResourcesInMemoryCache);What is this intended to measure/be used for? The name seems like it could be chosen to not be the same as the REF.
if (RuntimeEnabledFeatures::SVGPageLevelCacheIdentifierEnabled() &&
notified_target_observer_) {
return;
}Divyansh MangalThis looks like a bodge. This needs to be explained much better (this isn't even mentioned in the commit message). The naming of this field is also poor.
@f...@opera.com I agree on the ambiguity of this variable, let me try to explain why it was needed:
Consider your html has two <use> element both referencing the same resource. When first <use> fetches the resource and completion happens it will call [1] and notify itself. Then when the second <use> fetches the resources and completion happens it will notify all observers that are observing this resource. So it notifies, first <use> again and itself. This resulted in the first <use> element being notified twice which was not needed (and the DCHECK() at L:665 gets hit).
This behaviour was observed during the testing of WPT [2]
I wanted to use the active status of `pending_event_` to do the check instead but was not sure if that's a good fix given the DCHECK at L:665
@f...@opera.com I agree on the ambiguity of this variable, let me try to explain why it was needed:
Consider your html has two <use> element both referencing the same resource. When first <use> fetches the resource and completion happens it will call [1] and notify itself. Then when the second <use> fetches the resources and completion happens it will notify all observers that are observing this resource. So it notifies, first <use> again and itself. This resulted in the first <use> element being notified twice which was not needed (and the DCHECK() at L:665 gets hit).
If that happen, then something in how the notifications are generated is broken. All observers should only be notified when the resource changes state. In the example the second `<use>` would get a notification when adding itself as an observer, but no other observers should be notified at that point.
I would suspect that this indicates something may be broken/different with how revalidations are handled. If a new request of the same resource (URL) caused a revalidation, and then all observing `<use>`s are notified, we can choose to either rebuild the shadow tree or not. This currently does the latter.
`pending_event_` can't be used for this, because it's only about the gap between a resource load completing and the corresponding event being sent, while in the above case the event can have been dispatched a long time ago. Setting and unsetting a tracking bool could be done in `UpdateDocumentContent()` (similar to how `load_event_delayer_` is updated) and then cleared again when the notification arrives. The bool needs a more meaningful name though - like `notification_pending_` or something along those lines, to indicate that we've performed an action that we expect to yield a call to `ResourceNotifyFinished()`. The early-out should then be documented so that we know why it's being performed.
DCHECK_EQ(document_content_, document_content);This should go first in the function.
// Returns the SVG cache identifier for SVG document resources.
String GetSVGCacheIdentifier() const { return svg_cache_identifier_; }
void SetSVGCacheIdentifier(const String& cache_identifier) {
svg_cache_identifier_ = cache_identifier;
}
protected:
const Vector<KURL> empty_unused_preloads_;
String svg_cache_identifier_;You should extend the frame-specific `FetchContext` so that it accesses the cache id from the `Page`/`SVGResourceDocumentCache`.
void RemoveAllResourcesFromMap(ResourceMap* resources);Can be made `static`.
GetCacheIdentifier(factory.GetType(), params.GetResourceRequest().Url(),Just use the existing `url` (as before).
// For SVG documents, use the SVG-specific cache identifier when the featureSVG resource documents
name: "SVGPageLevelCacheIdentifier",Divyansh MangalIsn't more like "SvgDocumentResourcesInMemoryCache"(Partition)?
I have updated it to `PartitionSvgDocumentResourcesInMemoryCache`
I think having a name that starts with `"Svg..."` is preferable, because then SVG-related will naturally group.
Related details
- Divyansh Mangal
Divyansh Mangal (Gerrit)
Divyansh Mangal added 14 comments![]( "Open in Gerrit")
Data(), response.CurrentRequestUrl(), HasSuccessfulRevalidation());Divyansh MangalI think we could just skip a large part of this function if this is true - i.e there should be no need to update the content in any way.
Fredrik SöderquistI have already skipped some part of `UpdateDocument` already, did you mean there are other parts in `SVGDocumentResource::Finish` that we could skip?
The resource hasn't changed, so there shouldn't be a need to re-run that code - and why should we bother that code with this knowledge at all? Similarly here we don't want to potentially cause more side-effects than needed.
I have removed the argument for `UpdateDocument` and the updated the code.
I feel like when revalidation happens few things should happen like
->`LoadingFinished()` should be called.
-> content_ status should be updated to completed.
-> observers should be notified.
I have created a new function `RevalidationFinished` to depict that and rest of the functionality from `SVGDocumentResource::Finish` is utilized as is.
Let me know what you think of the new solution.
String(BrowsingContextGroupToken().ToString()));Divyansh MangalDoes this need to be wrapped in a `String`?
Fredrik SöderquistYes, because `BrowsingContextGroupToken().ToString()` returns `std::string` and there was no implicit conversion between `String` and `std::string`.
Divyansh MangalMaybe we could add a prefix to this identifier to make it a bit more "discoverable" - something like `"svg-resources:<token-string>"` perhaps?
Fredrik SöderquistI have hardcoded the string currently, but I believe we generally create a corresponding string somewhere else?
`SVGResourceDocumentCache` (later `...Tracker`) ought to be only entity that needs to care about hot the id is constructed, so in that sense it might be good to move the generation there. If it generated it internally it might've been even better.
Done
// `has_successful_revalidation` indicates whether the resource has been
// successfully revalidated.Kurt Catti-SchmidtThis feels circular - can you give a more general description that someone not familiar with the code would understand? Something like "`has_successful_revalidation` implies that the underlying object already exists." Or perhaps rename the variable name to `bool element_already_exists`?
Divyansh Mangal`element_already_exists` -> `resource_already_exists`
Divyansh MangalI want to emphasis on the part that this is happening only cause of revalidation, so I have kept the variable name as is, but I have updated the comment as you suggested. But let me know if you want to discuss about this further.
Closing this thread as has_successful_revalidation is no longer needed
document.Fetcher()->Context().SetSVGCacheIdentifier(
page->GetSVGResourceDocumentCache().GetCacheIdentifier());Divyansh MangalUnnecessary.
Done
WebFeature::kPartitionSVGDocumentResourcesInMemoryCache);What is this intended to measure/be used for? The name seems like it could be chosen to not be the same as the REF.
I actually wanted to measure how many users are actually using this SVG caching, i thought this might be a good way to metric to have. I placed this piece of code here because I know this is the entry point when caching will actually begin.
I have updated the name to `kCachingSVGDocumentResources`.
Let me know what you think of this?
if (RuntimeEnabledFeatures::SVGPageLevelCacheIdentifierEnabled() &&
notified_target_observer_) {
return;
}Divyansh MangalThis looks like a bodge. This needs to be explained much better (this isn't even mentioned in the commit message). The naming of this field is also poor.
Fredrik Söderquist@f...@opera.com I agree on the ambiguity of this variable, let me try to explain why it was needed:
Consider your html has two <use> element both referencing the same resource. When first <use> fetches the resource and completion happens it will call [1] and notify itself. Then when the second <use> fetches the resources and completion happens it will notify all observers that are observing this resource. So it notifies, first <use> again and itself. This resulted in the first <use> element being notified twice which was not needed (and the DCHECK() at L:665 gets hit).
This behaviour was observed during the testing of WPT [2]
I wanted to use the active status of `pending_event_` to do the check instead but was not sure if that's a good fix given the DCHECK at L:665
@f...@opera.com I agree on the ambiguity of this variable, let me try to explain why it was needed:
Consider your html has two <use> element both referencing the same resource. When first <use> fetches the resource and completion happens it will call [1] and notify itself. Then when the second <use> fetches the resources and completion happens it will notify all observers that are observing this resource. So it notifies, first <use> again and itself. This resulted in the first <use> element being notified twice which was not needed (and the DCHECK() at L:665 gets hit).
If that happen, then something in how the notifications are generated is broken. All observers should only be notified when the resource changes state. In the example the second `<use>` would get a notification when adding itself as an observer, but no other observers should be notified at that point.
I would suspect that this indicates something may be broken/different with how revalidations are handled. If a new request of the same resource (URL) caused a revalidation, and then all observing `<use>`s are notified, we can choose to either rebuild the shadow tree or not. This currently does the latter.
`pending_event_` can't be used for this, because it's only about the gap between a resource load completing and the corresponding event being sent, while in the above case the event can have been dispatched a long time ago. Setting and unsetting a tracking bool could be done in `UpdateDocumentContent()` (similar to how `load_event_delayer_` is updated) and then cleared again when the notification arrives. The bool needs a more meaningful name though - like `notification_pending_` or something along those lines, to indicate that we've performed an action that we expect to yield a call to `ResourceNotifyFinished()`. The early-out should then be documented so that we know why it's being performed.
I have implemented the suggestion that you gave, as well as commented the usage of `notification_pending_` wherever necessary. Let me know if I missed something
This should go first in the function.
Done
// Returns the SVG cache identifier for SVG document resources.
String GetSVGCacheIdentifier() const { return svg_cache_identifier_; }
void SetSVGCacheIdentifier(const String& cache_identifier) {
svg_cache_identifier_ = cache_identifier;
}
protected:
const Vector<KURL> empty_unused_preloads_;
String svg_cache_identifier_;You should extend the frame-specific `FetchContext` so that it accesses the cache id from the `Page`/`SVGResourceDocumentCache`.
Done
void RemoveAllResourcesFromMap(ResourceMap* resources);Can be made `static`.
I was not able to make it `static` as we utilise member function `RemoveInternal` internally.
const String cache_identifier =
RuntimeEnabledFeatures::SVGPageLevelCacheIdentifierEnabled() &&
!params.GetSVGCacheIdentifier().empty()
? params.GetSVGCacheIdentifier()
: GetCacheIdentifier(
params.GetResourceRequest().Url(),
params.GetResourceRequest().GetSkipServiceWorker());Divyansh MangalRather than putting more "gunk" into `FetchParameters`, I think we could get this identifier via the `FetchContext` (and use `factory.GetType()` to limit those calls).
Also, I think we could put this logic in an overload of `GetCacheIdentifier()`.
I added the setter and getter in `FetchContext` and removed the corresponding from `FetchParameters`. The overloaded is also introduced in the new PatchSet.
We are extending `FrameFetchContext` so no setter is needed now.
GetCacheIdentifier(factory.GetType(), params.GetResourceRequest().Url(),Just use the existing `url` (as before).
Done
// For SVG documents, use the SVG-specific cache identifier when the featureDivyansh MangalSVG resource documents
Done
name: "SVGPageLevelCacheIdentifier",Divyansh MangalIsn't more like "SvgDocumentResourcesInMemoryCache"(Partition)?
Fredrik SöderquistI have updated it to `PartitionSvgDocumentResourcesInMemoryCache`
I think having a name that starts with `"Svg..."` is preferable, because then SVG-related will naturally group.
Agreed, Updated the flag to `SvgPartitionSVGDocumentResourcesInMemoryCache`
Related details
- Fredrik Söderquist
- Kurt Catti-Schmidt
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 2 comments![]( "Open in Gerrit")
Data(), response.CurrentRequestUrl(), HasSuccessfulRevalidation());Divyansh MangalI think we could just skip a large part of this function if this is true - i.e there should be no need to update the content in any way.
Fredrik SöderquistI have already skipped some part of `UpdateDocument` already, did you mean there are other parts in `SVGDocumentResource::Finish` that we could skip?
Divyansh MangalThe resource hasn't changed, so there shouldn't be a need to re-run that code - and why should we bother that code with this knowledge at all? Similarly here we don't want to potentially cause more side-effects than needed.
I have removed the argument for `UpdateDocument` and the updated the code.
I feel like when revalidation happens few things should happen like->`LoadingFinished()` should be called.
-> content_ status should be updated to completed.
-> observers should be notified.I have created a new function `RevalidationFinished` to depict that and rest of the functionality from `SVGDocumentResource::Finish` is utilized as is.
Let me know what you think of the new solution.
No, none of state should have changed for a successful revalidation - and again the `*Content` object should not be bothered with this. What you could do is split out the main body of this function so that we don't have to indent that code. I.e make this something like:
```
void SVGDocumentResource::Finish(...) {
bool notify_observers = ...;
if (HasSuccessfulRevalidation()) {
notify_observers = content_->IsLoaded();
} else {
notify_observers = UpdateContent();
}
TextResource::Finish(load_finish_time, task_runner);
if (notify_observers) {
...
}
}
bool SVGDocumentResource:::UpdateContent() {
...
}
``` WebFeature::kPartitionSVGDocumentResourcesInMemoryCache);Divyansh MangalWhat is this intended to measure/be used for? The name seems like it could be chosen to not be the same as the REF.
I actually wanted to measure how many users are actually using this SVG caching, i thought this might be a good way to metric to have. I placed this piece of code here because I know this is the entry point when caching will actually begin.
I have updated the name to `kCachingSVGDocumentResources`.
Let me know what you think of this?
It's not measuring caching, it's measuring all loads.
Related details
- Divyansh Mangal
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 11 comments![]( "Open in Gerrit")
3) Proper Disposal of SVG ResourcesNot really introducing this, because you're literally reusing the existing mechanism (as stated in the text here).
if (document_ && document_->GetPage()) {We should be able to assume that this is true if detached is false.
if (document_ && document_->GetPage()) {Use the common pattern of checking for a detached state. (`GetResourceFetcherProperties().IsDetached()`)
to_remove.push_back(resource);
}
}
tracked_resources_.RemoveAll(to_remove);Should probably remove the reference from the `MemoryCache` here as well.
#include "third_party/blink/renderer/platform/loader/fetch/fetch_context.h"
#include "third_party/blink/renderer/platform/loader/fetch/fetch_parameters.h"
#include "third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h"Not used
// If IsLoaded() returns true then the document load completed
// synchronously, so we can check if we have a usable document and notify
// our listeners. If not, then we need to wait for the async load completion
// callback.Undo the formatting change
// `ResourceNotifyFinished()`. Used to avoid redundant shadow tree rebuilds
// when multiple notifications arrive for the same resource state change.Used to filter out (redundant) multiple notifications for the same resource.
// when multiple notifications arrive for the same resource state change.Can drop this bit.
// Tracks whether we've performed an action that we expect to yield a call toWe know what this "action" is, so we can spell it out.
notification_pending_ = false;This should be cleared at the top (since we can return before this). It could make sense to just have a single flah-guarded block at the top.
void RemoveAllResourcesFromMap(ResourceMap* resources);Divyansh MangalCan be made `static`.
I was not able to make it `static` as we utilise member function `RemoveInternal` internally.
Acknowledged
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job mac-m1_mini_2020-perf/motionmark1.3.crossbench failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1595191a510000
Divyansh Mangal (Gerrit)
Divyansh Mangal added 12 comments![]( "Open in Gerrit")
We should be able to assume that this is true if detached is false.
Done
Use the common pattern of checking for a detached state. (`GetResourceFetcherProperties().IsDetached()`)
Done
@f...@opera.com I have updated the code as you suggested, but here's the thing when the resource gets loaded for the first time the resource's status and resource-content's status starts as `kPending` (see `SVGDocumentResource::NotifyStartLoad()`) and when loading finishes the resource's status and resource-content's status becomes `kCached`, problem is when successful revalidation happens once again resource's status and resource-content's status starts as `kPending` but when it finishes resource's status gets updated to `kCached` during `TextResource::Finish(load_finish_time, task_runner)` but with your suggested code resource-content's status remain as `kPending` and even in subsequent function no-one updates it to `kCached`.
I still think that when revalidation is happening some function has to update the status from `kPending` to `kCached` for the resource-content as well since resource status is getting updated from `kPending` to `kCached`.
For that reason, I have kept the `content->RevalidationFinished()` but let me know if you want to discuss on this further.
to_remove.push_back(resource);
}
}
tracked_resources_.RemoveAll(to_remove);Should probably remove the reference from the `MemoryCache` here as well.
Done
#include "third_party/blink/renderer/platform/loader/fetch/fetch_context.h"
#include "third_party/blink/renderer/platform/loader/fetch/fetch_parameters.h"
#include "third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h"Divyansh MangalNot used
Done
// If IsLoaded() returns true then the document load completed
// synchronously, so we can check if we have a usable document and notify
// our listeners. If not, then we need to wait for the async load completion
// callback.Divyansh MangalUndo the formatting change
Done
WebFeature::kPartitionSVGDocumentResourcesInMemoryCache);Divyansh MangalWhat is this intended to measure/be used for? The name seems like it could be chosen to not be the same as the REF.
Fredrik SöderquistI actually wanted to measure how many users are actually using this SVG caching, i thought this might be a good way to metric to have. I placed this piece of code here because I know this is the entry point when caching will actually begin.
I have updated the name to `kCachingSVGDocumentResources`.
Let me know what you think of this?
It's not measuring caching, it's measuring all loads.
yes, you are right, I have shifted the position of usecounter code a little to better reflect caching. Let me know if its alright.
// `ResourceNotifyFinished()`. Used to avoid redundant shadow tree rebuilds
// when multiple notifications arrive for the same resource state change.Used to filter out (redundant) multiple notifications for the same resource.
Done
// when multiple notifications arrive for the same resource state change.Divyansh MangalCan drop this bit.
Done
// Tracks whether we've performed an action that we expect to yield a call toWe know what this "action" is, so we can spell it out.
Done
if (RuntimeEnabledFeatures::SVGPageLevelCacheIdentifierEnabled() &&
notified_target_observer_) {
return;
}Divyansh MangalThis looks like a bodge. This needs to be explained much better (this isn't even mentioned in the commit message). The naming of this field is also poor.
Fredrik Söderquist@f...@opera.com I agree on the ambiguity of this variable, let me try to explain why it was needed:
Consider your html has two <use> element both referencing the same resource. When first <use> fetches the resource and completion happens it will call [1] and notify itself. Then when the second <use> fetches the resources and completion happens it will notify all observers that are observing this resource. So it notifies, first <use> again and itself. This resulted in the first <use> element being notified twice which was not needed (and the DCHECK() at L:665 gets hit).
This behaviour was observed during the testing of WPT [2]
I wanted to use the active status of `pending_event_` to do the check instead but was not sure if that's a good fix given the DCHECK at L:665
Divyansh Mangal@f...@opera.com I agree on the ambiguity of this variable, let me try to explain why it was needed:
Consider your html has two <use> element both referencing the same resource. When first <use> fetches the resource and completion happens it will call [1] and notify itself. Then when the second <use> fetches the resources and completion happens it will notify all observers that are observing this resource. So it notifies, first <use> again and itself. This resulted in the first <use> element being notified twice which was not needed (and the DCHECK() at L:665 gets hit).
If that happen, then something in how the notifications are generated is broken. All observers should only be notified when the resource changes state. In the example the second `<use>` would get a notification when adding itself as an observer, but no other observers should be notified at that point.
I would suspect that this indicates something may be broken/different with how revalidations are handled. If a new request of the same resource (URL) caused a revalidation, and then all observing `<use>`s are notified, we can choose to either rebuild the shadow tree or not. This currently does the latter.
`pending_event_` can't be used for this, because it's only about the gap between a resource load completing and the corresponding event being sent, while in the above case the event can have been dispatched a long time ago. Setting and unsetting a tracking bool could be done in `UpdateDocumentContent()` (similar to how `load_event_delayer_` is updated) and then cleared again when the notification arrives. The bool needs a more meaningful name though - like `notification_pending_` or something along those lines, to indicate that we've performed an action that we expect to yield a call to `ResourceNotifyFinished()`. The early-out should then be documented so that we know why it's being performed.
I have implemented the suggestion that you gave, as well as commented the usage of `notification_pending_` wherever necessary. Let me know if I missed something
Closing this discussion as we are moving with the `notification_pending_` solution.
This should be cleared at the top (since we can return before this). It could make sense to just have a single flah-guarded block at the top.
- Fredrik Söderquist
- Kurt Catti-Schmidt
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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
3) Proper Disposal of SVG ResourcesDivyansh MangalNot really introducing this, because you're literally reusing the existing mechanism (as stated in the text here).
It does sounds like I am introducing this change rather than reusing. I have updated the description and added a short note for the garbage disposal mechanism
Related details
- Fredrik Söderquist
- Kurt Catti-Schmidt
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
anne robles (Gerrit)
anne robles added 1 comment![]( "Open in Gerrit")
- Divyansh Mangal
Fredrik Söderquist (Gerrit)
Fredrik Söderquist added 7 comments![]( "Open in Gerrit")
Sounds like it could call `UpdateStatus()`. There could be a function to determine the status and then a call to `UpdateStatus()`.
static String MakeCacheIdentifier(String browser_context_group_token);`StringView`
inline constexpr char kSVGDocumentResourcePrefix[] = "svg-resources:";There's no reason for this to live in this namespace. It's only used once, so it can be local to that scope.
WebFeature::kPartitionSVGDocumentResourcesInMemoryCache);Divyansh MangalWhat is this intended to measure/be used for? The name seems like it could be chosen to not be the same as the REF.
Fredrik SöderquistI actually wanted to measure how many users are actually using this SVG caching, i thought this might be a good way to metric to have. I placed this piece of code here because I know this is the entry point when caching will actually begin.
I have updated the name to `kCachingSVGDocumentResources`.
Let me know what you think of this?
Divyansh MangalIt's not measuring caching, it's measuring all loads.
yes, you are right, I have shifted the position of usecounter code a little to better reflect caching. Let me know if its alright.
I still the "caching" bit is uninteresting - there's always caching. Maybe tracking if external SVG document resources are used is interesting to someone, in which case I'd let that ("external") replace caching in the name.
// Tracks whether we've performed an action that we expect to yield a call toDivyansh MangalWe know what this "action" is, so we can spell it out.
Done
The new version is not correct.
// Early-out if we've already been notified for this resource state change.Drop
// elements to be notified, but we only want to rebuild the shadow tree once
// per actual resource change....when this element has initiated the resource fetch.
Related details
- Divyansh Mangal
Divyansh Mangal (Gerrit)
Divyansh Mangal added 7 comments![]( "Open in Gerrit")
@f...@opera.com I tried moving the code around to have a good-looking solution, I have kept the `content->RevalidationFinished()` and based on its return status calling `UpdateStatus` in `UpdateContentStatus`.
One small thing though instead of just using `content_->IsLoaded()` to notify the observer, once I have updated the `content_`'s status then I am notifying the observers. (basically, based on the updated content's status observers should be notified)
Let me know what you think of the new approach.
static String MakeCacheIdentifier(String browser_context_group_token);`StringView`
Updated the change, just curious though is `StringView` optimised for string operations like copying and appending?
inline constexpr char kSVGDocumentResourcePrefix[] = "svg-resources:";There's no reason for this to live in this namespace. It's only used once, so it can be local to that scope.
Moved to local scope
WebFeature::kPartitionSVGDocumentResourcesInMemoryCache);Divyansh MangalWhat is this intended to measure/be used for? The name seems like it could be chosen to not be the same as the REF.
Fredrik SöderquistI actually wanted to measure how many users are actually using this SVG caching, i thought this might be a good way to metric to have. I placed this piece of code here because I know this is the entry point when caching will actually begin.
I have updated the name to `kCachingSVGDocumentResources`.
Let me know what you think of this?
Divyansh MangalIt's not measuring caching, it's measuring all loads.
Fredrik Söderquistyes, you are right, I have shifted the position of usecounter code a little to better reflect caching. Let me know if its alright.
I still the "caching" bit is uninteresting - there's always caching. Maybe tracking if external SVG document resources are used is interesting to someone, in which case I'd let that ("external") replace caching in the name.
Fair enough, updated the variable name to `kExternalSVGDocumentResources`
// Tracks whether we've performed an action that we expect to yield a call toDivyansh MangalWe know what this "action" is, so we can spell it out.
Fredrik SöderquistDone
The new version is not correct.
My bad, I focused more on the revalidation aspect rather than the action being performed, I have updated it let me know if it need some refining.
// Early-out if we've already been notified for this resource state change.Divyansh MangalDrop
Done
// elements to be notified, but we only want to rebuild the shadow tree once
// per actual resource change....when this element has initiated the resource fetch.
- Fredrik Söderquist
- Kurt Catti-Schmidt
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| 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")
`LoadingFinished()` shouldn't be called in that code-path. If you want to determine the state of the content, then add a function to do that (which needn't have any side-effects) and name it appropriately. With the current factoring `SVGDocumentResource::UpdateContent` and `UpdateContentStatus` are unnecessary (and doesn't really aid readability).
static String MakeCacheIdentifier(String browser_context_group_token);Divyansh Mangal`StringView`
Updated the change, just curious though is `StringView` optimised for string operations like copying and appending?
It's optimized for not having to create `String` objects when they are not needed (like in this case).
// Tracks whether we've performed an action that we expect to yield a call toDivyansh MangalWe know what this "action" is, so we can spell it out.
Fredrik SöderquistDone
Divyansh MangalThe new version is not correct.
My bad, I focused more on the revalidation aspect rather than the action being performed, I have updated it let me know if it need some refining.
It looks ok now.
name: "SVGPageLevelCacheIdentifier",Divyansh MangalIsn't more like "SvgDocumentResourcesInMemoryCache"(Partition)?
Fredrik SöderquistI have updated it to `PartitionSvgDocumentResourcesInMemoryCache`
Divyansh MangalI think having a name that starts with `"Svg..."` is preferable, because then SVG-related will naturally group.
Agreed, Updated the flag to `SvgPartitionSVGDocumentResourcesInMemoryCache`
- Divyansh Mangal
Divyansh Mangal (Gerrit)
Divyansh Mangal added 2 comments![]( "Open in Gerrit")
Ohh, in that case I don't think I need to introduce any new functions then a call like `content_->UpdateStatus(GetStatus());` should be sufficient right?
But let me know if I missed something
static String MakeCacheIdentifier(String browser_context_group_token);Divyansh Mangal`StringView`
Fredrik SöderquistUpdated the change, just curious though is `StringView` optimised for string operations like copying and appending?
It's optimized for not having to create `String` objects when they are not needed (like in this case).
- Fredrik Söderquist
- Kurt Catti-Schmidt
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fredrik Söderquist (Gerrit)
Fredrik Söderquist voted and added 2 comments![]( "Open in Gerrit")
Votes added by Fredrik Söderquist
| Code-Review | +1 |
2 comments
Data(), response.CurrentRequestUrl(), HasSuccessfulRevalidation());Marking resolved
WebFeature::kPartitionSVGDocumentResourcesInMemoryCache);Divyansh MangalWhat is this intended to measure/be used for? The name seems like it could be chosen to not be the same as the REF.
Fredrik SöderquistI actually wanted to measure how many users are actually using this SVG caching, i thought this might be a good way to metric to have. I placed this piece of code here because I know this is the entry point when caching will actually begin.
I have updated the name to `kCachingSVGDocumentResources`.
Let me know what you think of this?
Divyansh MangalIt's not measuring caching, it's measuring all loads.
Fredrik Söderquistyes, you are right, I have shifted the position of usecounter code a little to better reflect caching. Let me know if its alright.
Divyansh MangalI still the "caching" bit is uninteresting - there's always caching. Maybe tracking if external SVG document resources are used is interesting to someone, in which case I'd let that ("external") replace caching in the name.
Fair enough, updated the variable name to `kExternalSVGDocumentResources`
- Divyansh Mangal
- Kurt Catti-Schmidt
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
- Kurt Catti-Schmidt
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt voted and added 4 comments![]( "Open in Gerrit")
Votes added by Kurt Catti-Schmidt
| Code-Review | +1 |
4 comments
Just some small suggestions, overall lgtm.
bool HasContentForTesting(SVGResourceDocumentContent* content);This should be `const`
void EvictResourcesForCacheIdentifier(const String& cache_identifier);This needs a comment (similar to the one above)
if (resource_map_iter == resource_maps_.end()) {
return;
}Is this scenario possible? If yes, please add a comment documenting when this occurs. If not, this should this be a `DCHECK_NE(resource_map_iter, resource_maps_.end())`.
Related details
- Divyansh Mangal
Divyansh Mangal (Gerrit)
Divyansh Mangal added 3 comments![]( "Open in Gerrit")
bool HasContentForTesting(SVGResourceDocumentContent* content);Divyansh MangalThis should be `const`
Done
void EvictResourcesForCacheIdentifier(const String& cache_identifier);This needs a comment (similar to the one above)
Done
if (resource_map_iter == resource_maps_.end()) {
return;
}Is this scenario possible? If yes, please add a comment documenting when this occurs. If not, this should this be a `DCHECK_NE(resource_map_iter, resource_maps_.end())`.
Hi Kurt, yes, this scenario is possible. So, consider an svg that got added and got removed after sometime. In that case the inner map corresponding to `cache_identifier` exists but it will be empty. And when the Page is getting destroyed this function is called so If I add the DCHECK that you suggested it will give unnecessary error.
I have added a short comment mentioning this fact. Let me know if that makes sense.
Related details
- Kurt Catti-Schmidt
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kurt Catti-Schmidt
| Code-Review | +1 |
1 comment
if (resource_map_iter == resource_maps_.end()) {
return;
}Divyansh MangalIs this scenario possible? If yes, please add a comment documenting when this occurs. If not, this should this be a `DCHECK_NE(resource_map_iter, resource_maps_.end())`.
Hi Kurt, yes, this scenario is possible. So, consider an svg that got added and got removed after sometime. In that case the inner map corresponding to `cache_identifier` exists but it will be empty. And when the Page is getting destroyed this function is called so If I add the DCHECK that you suggested it will give unnecessary error.
I have added a short comment mentioning this fact. Let me know if that makes sense.
1. "...exists but.." (missing an "s")
2. This isn't checking that the map is empty, or that the entry is empty. I realize that `find` will return no results when it's empty though. Can you add a DCHECK ensuring that it's empty when this happens?
```
if (resource_map_iter == resource_maps_.end()) {
DCHECK(resource_maps_.empty());
return;
}
```
...or are there more cases where this can happen? Thinking about it more, this would be cleaner (and maybe more performant) to do an empty check before the `find`, e.g.:
```
// We need to early out in cases where the resource map exist but its empty.
if(resource_maps_.empty()) {
return;
}
const auto& resource_map_iter = resource_maps_.find(cache_identifier);
DCHECK_NE(resource_map_iter, resource_maps_.end())
```
My concern is that we don't want to have page identifiers in the cache that aren't supposed to be there, as they could consume extra memory. So having these extra DCHECK's can help avoid bugs.
Related details
- Divyansh Mangal
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
if (resource_map_iter == resource_maps_.end()) {
return;
}Divyansh MangalIs this scenario possible? If yes, please add a comment documenting when this occurs. If not, this should this be a `DCHECK_NE(resource_map_iter, resource_maps_.end())`.
Kurt Catti-SchmidtHi Kurt, yes, this scenario is possible. So, consider an svg that got added and got removed after sometime. In that case the inner map corresponding to `cache_identifier` exists but it will be empty. And when the Page is getting destroyed this function is called so If I add the DCHECK that you suggested it will give unnecessary error.
I have added a short comment mentioning this fact. Let me know if that makes sense.
1. "...exists but.." (missing an "s")
2. This isn't checking that the map is empty, or that the entry is empty. I realize that `find` will return no results when it's empty though. Can you add a DCHECK ensuring that it's empty when this happens?```
if (resource_map_iter == resource_maps_.end()) {
DCHECK(resource_maps_.empty());
return;
}
```...or are there more cases where this can happen? Thinking about it more, this would be cleaner (and maybe more performant) to do an empty check before the `find`, e.g.:
```
// We need to early out in cases where the resource map exist but its empty.
if(resource_maps_.empty()) {
return;
}
const auto& resource_map_iter = resource_maps_.find(cache_identifier);
DCHECK_NE(resource_map_iter, resource_maps_.end())
```My concern is that we don't want to have page identifiers in the cache that aren't supposed to be there, as they could consume extra memory. So having these extra DCHECK's can help avoid bugs.
Hi Kurt, I understand your concern empty case was just one example thats where the DHCECK that you suggested was hitting. Basically, the existence of svg page specific cache identifier does not guarantee there will be a corresponding entry in `ResoureMap` of `MemoryCache`.
For example, if we try to fetch a svg that was in a data:url, no new resource will be made, since no new resource is being made no new entry in ResourceMap will be added (see working in `ResourceFetcher`) but since we will follow the usual steps of fetching the svg resource, the variable `cache_identifier_` at `SVGResourceDocumentCache` will still be populated. (I have also tested your suggestions and tests like third_party/blink/web_tests/editing/pasteboard/paste-svg-use.html fails due to a similar reason)
Try to think in this way, whenever we fetch a svg it's not guaranteed to be a success (or it's not guaranteed that we will add in the `MemoryCache`), however we have still created a string at `cache_identifier_` at svg cache level whether there was success or failure. But, there will be no entry corresponding to it at the `MemoryCache` level so we are not consuming any extra memory there. Only string value at `SVGResourceDocumentCache` will persist, which I don't is that much of an issue. Let me know if you want to discuss this further.
Related details
- Kurt Catti-Schmidt
- Sejal Anand
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |