Clean CSS random cache when associated element is not alive [chromium/src : main]
Munira Tursunova (Gerrit)
Munira Tursunova has uploaded the change for review![]( "Open in Gerrit")
Commit message
Clean CSS random cache when associated element is not alive
If Element was removed, clean random base value cache entries, which
keys are associated with that element.
Change diff
Change information
- M third_party/blink/renderer/core/css/css_math_expression_node.h
- M third_party/blink/renderer/core/css/random_caching_key.cc
- M third_party/blink/renderer/core/css/random_caching_key.h
- M third_party/blink/renderer/core/css/style_engine.cc
- M third_party/blink/renderer/core/css/style_engine.h
- M third_party/blink/renderer/core/css/style_engine_test.cc
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Munira Tursunova (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Munira Tursunova (Gerrit)
Munira Tursunova voted Commit-Queue+1![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
UntracedMember<const Element> element_;Why can't this be WeakMember? Is it because name_ + nullptr element_ is a different key from name_ + element_ which has been nullified as a weak reference?
Related details
- Munira Tursunova
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. |
Munira Tursunova (Gerrit)
Munira Tursunova added 1 comment![]( "Open in Gerrit")
UntracedMember<const Element> element_;Why can't this be WeakMember? Is it because name_ + nullptr element_ is a different key from name_ + element_ which has been nullified as a weak reference?
I don't know when it will be nullified if I use `WeakMember`. So what if sometimes it will be nullified before `WeakCallbackMethod` is called? Then we won't clean the entry in the cache assosiated with this key, since `IsHeapObjectAlive` will return true, it says so in the comment here: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/cppgc/liveness-broker.h;l=48?q=IsHeapObjectAlive&ss=chromium%2Fchromium%2Fsrc:
nullptr objects are considered alive ...
I don't know oilpan well, so I thought it would be safer to use `UntracedMember` here, since I assume it shouldn't be nullified.
Related details
- Rune Lillesveen
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. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
UntracedMember<const Element> element_;Munira TursunovaWhy can't this be WeakMember? Is it because name_ + nullptr element_ is a different key from name_ + element_ which has been nullified as a weak reference?
I don't know when it will be nullified if I use `WeakMember`. So what if sometimes it will be nullified before `WeakCallbackMethod` is called? Then we won't clean the entry in the cache assosiated with this key, since `IsHeapObjectAlive` will return true, it says so in the comment here: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/cppgc/liveness-broker.h;l=48?q=IsHeapObjectAlive&ss=chromium%2Fchromium%2Fsrc:
nullptr objects are considered alive ...
I don't know oilpan well, so I thought it would be safer to use `UntracedMember` here, since I assume it shouldn't be nullified.
I don't know when it will be nullified if I use `WeakMember`. So what if sometimes it will be nullified before `WeakCallbackMethod` is called? Then we won't clean the entry in the cache assosiated with this key, since `IsHeapObjectAlive` will return true, it says so in the comment here: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/cppgc/liveness-broker.h;l=48?q=IsHeapObjectAlive&ss=chromium%2Fchromium%2Fsrc:
nullptr objects are considered alive ...
I don't know oilpan well, so I thought it would be safer to use `UntracedMember` here, since I assume it shouldn't be nullified.
My fundamental problem here is that I don't know what you are trying to fix.
Related details
- Munira Tursunova
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. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
UntracedMember<const Element> element_;Munira TursunovaWhy can't this be WeakMember? Is it because name_ + nullptr element_ is a different key from name_ + element_ which has been nullified as a weak reference?
Rune LillesveenI don't know when it will be nullified if I use `WeakMember`. So what if sometimes it will be nullified before `WeakCallbackMethod` is called? Then we won't clean the entry in the cache assosiated with this key, since `IsHeapObjectAlive` will return true, it says so in the comment here: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/cppgc/liveness-broker.h;l=48?q=IsHeapObjectAlive&ss=chromium%2Fchromium%2Fsrc:
nullptr objects are considered alive ...
I don't know oilpan well, so I thought it would be safer to use `UntracedMember` here, since I assume it shouldn't be nullified.
I don't know when it will be nullified if I use `WeakMember`. So what if sometimes it will be nullified before `WeakCallbackMethod` is called? Then we won't clean the entry in the cache assosiated with this key, since `IsHeapObjectAlive` will return true, it says so in the comment here: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/cppgc/liveness-broker.h;l=48?q=IsHeapObjectAlive&ss=chromium%2Fchromium%2Fsrc:
nullptr objects are considered alive ...
I don't know oilpan well, so I thought it would be safer to use `UntracedMember` here, since I assume it shouldn't be nullified.
My fundamental problem here is that I don't know what you are trying to fix.
WeakMember are cleared on GC :) not sure what ele to say here.
I guess what you want to keep the cache dependent on the liveness of `Element`. My suggestion would be to model that explicitly somehow: Keep the cache on something related to `Element`, so it goes automatically away. E.g. keep some `Member<>` references from some rare data.
If that's not possible, this can be modelled externally (outside of `Element`) with the following:
- HeapHashMap<WeakMember<Element>, Member<HeapVector<Member<RandomCachingKey>> element_keeps_cache_key_alive;
- HeapHashMap<WeakMember<RandomCachingKey>, double> cache;
with `RandomCachingKey::element_` being `WeakMember` as well.
Memory wise the entry gets a bit more expensive (because of the `element_keeps_cache_key_alive` map) but liveness works automatically.
Only if that is not an option, we should do manual callbacks...
Related details
- Munira Tursunova
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. |
Munira Tursunova (Gerrit)
Munira Tursunova added 1 comment![]( "Open in Gerrit")
UntracedMember<const Element> element_;Munira TursunovaWhy can't this be WeakMember? Is it because name_ + nullptr element_ is a different key from name_ + element_ which has been nullified as a weak reference?
Rune LillesveenI don't know when it will be nullified if I use `WeakMember`. So what if sometimes it will be nullified before `WeakCallbackMethod` is called? Then we won't clean the entry in the cache assosiated with this key, since `IsHeapObjectAlive` will return true, it says so in the comment here: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/cppgc/liveness-broker.h;l=48?q=IsHeapObjectAlive&ss=chromium%2Fchromium%2Fsrc:
nullptr objects are considered alive ...
I don't know oilpan well, so I thought it would be safer to use `UntracedMember` here, since I assume it shouldn't be nullified.
Michael LippautzI don't know when it will be nullified if I use `WeakMember`. So what if sometimes it will be nullified before `WeakCallbackMethod` is called? Then we won't clean the entry in the cache assosiated with this key, since `IsHeapObjectAlive` will return true, it says so in the comment here: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/cppgc/liveness-broker.h;l=48?q=IsHeapObjectAlive&ss=chromium%2Fchromium%2Fsrc:
nullptr objects are considered alive ...
I don't know oilpan well, so I thought it would be safer to use `UntracedMember` here, since I assume it shouldn't be nullified.
My fundamental problem here is that I don't know what you are trying to fix.
WeakMember are cleared on GC :) not sure what ele to say here.
I guess what you want to keep the cache dependent on the liveness of `Element`. My suggestion would be to model that explicitly somehow: Keep the cache on something related to `Element`, so it goes automatically away. E.g. keep some `Member<>` references from some rare data.
If that's not possible, this can be modelled externally (outside of `Element`) with the following:
- HeapHashMap<WeakMember<Element>, Member<HeapVector<Member<RandomCachingKey>> element_keeps_cache_key_alive;
- HeapHashMap<WeakMember<RandomCachingKey>, double> cache;
with `RandomCachingKey::element_` being `WeakMember` as well.
Memory wise the entry gets a bit more expensive (because of the `element_keeps_cache_key_alive` map) but liveness works automatically.
Only if that is not an option, we should do manual callbacks...
Thank you, Michael for the suggestions.
>If that's not possible, this can be modelled externally (outside of Element) with the following:
HeapHashMap<WeakMember<Element>, Member<HeapVector<Member<RandomCachingKey>> element_keeps_cache_key_alive;
HeapHashMap<WeakMember<RandomCachingKey>, double> cache;
Some of the cache entries are not element dependent, i.e. we can have `RandomCachingKey` with `RandomCachingKey::element_` always set to nullptr. I guess if we use WeakMember<RandomCachingKey> in cache for them (how you suggested above) there would be no one keeping them alive, no?
About storing cache on Element's rare data, I'm not sure what we can/canot add there, so I was trying to avoid that, we can actually just use hash if we can store a seed there, so we won't need a cache at all, even the one on style_engine. But I went with this aporoach since it seemed easier :)
Rune, about your question:
>My fundamental problem here is that I don't know what you are trying to fix.
Currently the random_values_cache is not properly cleaned, so when we remove Element, the RandomCachingKey entry that has a pointer to the removed Element is not getting cleaned. I'm trying to fix this problem in this CL. Please have a look at added `StyleEngineTest`, without changes in this Cl, the cache is never cleared, so second `EXPECT_EQ()` check will fail.
Related details
- Michael Lippautz
- Rune Lillesveen
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. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
Thank you, Michael for the suggestions.
>If that's not possible, this can be modelled externally (outside of Element) with the following:
HeapHashMap<WeakMember<Element>, Member<HeapVector<Member<RandomCachingKey>> element_keeps_cache_key_alive;
HeapHashMap<WeakMember<RandomCachingKey>, double> cache;Some of the cache entries are not element dependent, i.e. we can have `RandomCachingKey` with `RandomCachingKey::element_` always set to nullptr. I guess if we use WeakMember<RandomCachingKey> in cache for them (how you suggested above) there would be no one keeping them alive, no?
My immediate thought is that they should be two different HashMaps, then. I think we've discussed that before and you wanted to have a single map for some reason.
About storing cache on Element's rare data, I'm not sure what we can/canot add there, so I was trying to avoid that, we can actually just use hash if we can store a seed there, so we won't need a cache at all, even the one on style_engine. But I went with this aporoach since it seemed easier :)
Rune, about your question:
>My fundamental problem here is that I don't know what you are trying to fix.
Currently the random_values_cache is not properly cleaned, so when we remove Element, the RandomCachingKey entry that has a pointer to the removed Element is not getting cleaned. I'm trying to fix this problem in this CL. Please have a look at added `StyleEngineTest`, without changes in this Cl, the cache is never cleared, so second `EXPECT_EQ()` check will fail.
I shouldn't have to decode the unit tests to figure out what the CL is trying to do. It should be described in the CL description.
Related details
- Michael Lippautz
- Munira Tursunova
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. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
UntracedMember<const Element> element_;Opening this up as separate comment: If there's no element that's guarding the entry liveness. How are we preventing the cache blowing out of proportion when there's no elements assigned?
Related details
- Munira Tursunova
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. |
Munira Tursunova (Gerrit)
Munira Tursunova added 1 comment![]( "Open in Gerrit")
UntracedMember<const Element> element_;Opening this up as separate comment: If there's no element that's guarding the entry liveness. How are we preventing the cache blowing out of proportion when there's no elements assigned?
We don't, the way I understand the spec, these kind of entries should have livetime of the document, so yes, we will keep them while document is alive.
Related details
- Michael Lippautz
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. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
UntracedMember<const Element> element_;Munira TursunovaOpening this up as separate comment: If there's no element that's guarding the entry liveness. How are we preventing the cache blowing out of proportion when there's no elements assigned?
We don't, the way I understand the spec, these kind of entries should have livetime of the document, so yes, we will keep them while document is alive.
We don't, the way I understand the spec, these kind of entries should have livetime of the document, so yes, we will keep them while document is alive.
They do have some upper bound as a function of the number of known standard css properties, registered custom properties, and max number of random() instances per a single declaration, which is a lot less likely to blow up than for element references, right?
Related details
- Michael Lippautz
- Munira Tursunova
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. |
Munira Tursunova (Gerrit)
Munira Tursunova added 1 comment![]( "Open in Gerrit")
UntracedMember<const Element> element_;Munira TursunovaOpening this up as separate comment: If there's no element that's guarding the entry liveness. How are we preventing the cache blowing out of proportion when there's no elements assigned?
Rune LillesveenWe don't, the way I understand the spec, these kind of entries should have livetime of the document, so yes, we will keep them while document is alive.
We don't, the way I understand the spec, these kind of entries should have livetime of the document, so yes, we will keep them while document is alive.
They do have some upper bound as a function of the number of known standard css properties, registered custom properties, and max number of random() instances per a single declaration, which is a lot less likely to blow up than for element references, right?
>They do have some upper bound as a function of the number of known standard css properties, registered custom properties, and max number of random() instances per a single declaration, which is a lot less likely to blow up than for element references, right?
Yes, but I'm not sure I understand this part "...and max number of random() instances per a single declaration...". We can have different random() identifiers as well as standard css properties and registered custom properties for caching. So theoretically we might have a blowed up cache, if we have many random() values with different identifiers,i.e. `random(--ident, ...)` on the page. But seems unlikely to happen.
Related details
- Michael Lippautz
- Rune Lillesveen
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. |
Munira Tursunova (Gerrit)
Munira Tursunova added 1 comment![]( "Open in Gerrit")
UntracedMember<const Element> element_;>I shouldn't have to decode the unit tests to figure out what the CL is trying to do. It should be described in the CL description.
Updated CL description hope it's clearer now.
Related details
- Michael Lippautz
- Rune Lillesveen
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. |
Munira Tursunova (Gerrit)
Also, I think I didn't explain properly why I don't want to store values on ElementRareData, since we get Element object from CSSLengthResolver, we need to make it mutable if we want to add/get random values to ElementRareData and I'm not sure if we want to allow that.