New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TextureMapper] Missing implementation of backingStoreMemoryEstimate #14286
[TextureMapper] Missing implementation of backingStoreMemoryEstimate #14286
Conversation
EWS run on previous version of this PR (hash e8cf0b9) |
return 0; | ||
|
||
return *estimatedBackingStoreMemory; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just return estimatedBackingStoreMemory.value_or(0) to avoid checking whether there value is defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pgorszkowski-igalia @magomez : is it normal to return 0 here for CoordinatedGraphicsLayer::backingStoreMemoryEstimate()
?
It seems that intermediate steps of the PR returned estimatedBackingStoreMemory
.
cc @hwti
@@ -253,6 +255,8 @@ class WEBCORE_EXPORT CoordinatedGraphicsLayer : public GraphicsLayer { | |||
|
|||
RefPtr<AnimatedBackingStoreHost> m_animatedBackingStoreHost; | |||
RefPtr<CoordinatedGraphicsLayer> m_backdropLayer; | |||
|
|||
std::optional<double> estimatedBackingStoreMemory; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why you use an optional here. A double initialized to 0 should be enough, as 0 is what you're returning when the optional is not initialized. I mean, you can use the optional, but it's an overkill in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I will use just double for that.
#if ENABLE(RESOURCE_USAGE) | ||
// We save the total memory size of layers to show in the memory timeline of Inspector. | ||
WebCore::ResourceUsageThread::setTotalLayerBackingStoreBytes(m_obligatoryBackingStoreBytes + m_secondaryBackingStoreBytes); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with this is that it's only executed if LOG is enabled, which is odd. So in order to get a valid value we need to enable both LOG and RESOURCE_USAGE. We should be able to get this value without having to enable LOG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that m_obligatoryBackingStoreBytes and m_secondaryBackingStoreBytes are set and calculated also only in case when LOG(Compositing) is enabled.
RESOURCE_USAGE I will remove, it is not needed here.
e8cf0b9
to
00345a6
Compare
EWS run on previous version of this PR (hash 00345a6) |
00345a6
to
d30d6f5
Compare
EWS run on previous version of this PR (hash d30d6f5) |
d30d6f5
to
6f51038
Compare
EWS run on previous version of this PR (hash 6f51038) |
6f51038
to
25dad28
Compare
EWS run on previous version of this PR (hash 25dad28) |
25dad28
to
31f3708
Compare
EWS run on previous version of this PR (hash 31f3708) |
31f3708
to
0d0c1f5
Compare
EWS run on previous version of this PR (hash 0d0c1f5) |
// Memory usage of "Layers" can be "0" | ||
memoryCategories.push({type: WI.MemoryCategory.Type.Layers, size: layersSize}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the motivation/goal of this change?
also, i think this means that every WI.MemoryTimelineRecord
will now have a WI.MemoryCategory.Type.Layers
since layersSize
defaults to 0
at the top, which is probably not what we want. IMO if a WI.MemoryTimelineRecord
has a WI.MemoryCategory.Type.Layers
with a size
of 0
then we probably dont want/need to show it since there's nothing to show as it is 0 after all (i.e. having an empty graph is arguably worse than not having a graph as it could draw attention where none is deserved)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it is possible that in some cases the memory usage could be 0 but not for all time. And not having graph because memory usage is 0 at the beginning is not ideal solution. Probably there should be something else which will indicate that the graph should not be shown because there will be always 0 value. Maybe some additional field in MemoryCategoryInfo to indicate is it valid or not for specific category? WDYT?
Another solution could be removing categories for which there is no data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im pretty sure that's how it works today. it should only show the graph(s) if there's something to show
there's no way for us to know if something in that category will happen in the future. we certainly could do a better job of differentiating "there was no work in this category" and "the work in this category was not substantial enough to actually be above 0
", but IMO that's something unrelated to this PR so i'd kindly ask that you undo your changes here and open a new bug/PR where we can directly discuss/address that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will undo it and create a separate bug report about that. I completely agree with you that this is not related to this PR, but this change will depend on the one about described issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0d0c1f5
to
bf52b73
Compare
EWS run on previous version of this PR (hash bf52b73) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, fix the commit message to explain why this is implemented that way it is, because it's not straightforward to get what's been done here and why.
@@ -105,6 +105,9 @@ void ResourceUsageThread::platformSaveStateBeforeStarting() | |||
m_samplingProfilerThreadID = thread->id(); | |||
} | |||
#endif | |||
// Reset max of reported memory usage of layers. | |||
Locker locker { m_layersMemoryUsageLock }; | |||
m_maxLayersMemoryUsage = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be reset to m_currentLayersMemoryUsage and not 0.
Locker locker { m_layersMemoryUsageLock }; | ||
data.categories[MemoryCategory::Layers].dirtySize = m_maxLayersMemoryUsage; | ||
// Reset the max reported of memory usage to gather new max in next 500ms. | ||
m_maxLayersMemoryUsage = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be reset to m_currentLayersMemoryUsage and not 0 as well.
bf52b73
to
6ccb09d
Compare
EWS run on previous version of this PR (hash 6ccb09d) |
6ccb09d
to
25d28d1
Compare
EWS run on previous version of this PR (hash 25d28d1) |
25d28d1
to
3701c91
Compare
EWS run on previous version of this PR (hash 3701c91) |
3701c91
to
48f6bce
Compare
EWS run on previous version of this PR (hash 48f6bce) |
48f6bce
to
a81615a
Compare
EWS run on previous version of this PR (hash a81615a) |
46cd6c0
to
674c035
Compare
EWS run on current version of this PR (hash 674c035) |
https://bugs.webkit.org/show_bug.cgi?id=254910 Reviewed by Miguel Gomez. The backingStores used by the CoordinatedGraphicsLayers are created during a layerFlush, and they are freed in the next compositor cycle, after their content is uploaded into a texture. Due to this, the life span of the Nicosia::Buffers is very short. As the ResourceUsageThread samples the used memory every 500ms, it would be quite common that the memory usage reported by the timeline would be zero. In order to avoid this, the maximum memory usage is stored until the ResourceUsageThread is able to read it. Thanks to this, the timeline is able to show the peak in the memory usage that reflects the reality. It's not perfect though, as the peak will be shown in the timeline some ms after it really happened, but it's the best approach we can do. * Source/WebCore/page/linux/ResourceUsageThreadLinux.cpp: (WebCore::ResourceUsageThread::platformSaveStateBeforeStarting): (WebCore::ResourceUsageThread::platformCollectMemoryData): * Source/WebCore/platform/graphics/nicosia/NicosiaBuffer.cpp: (Nicosia::Buffer::resetMemoryUsage): (Nicosia::Buffer::getMemoryUsage): (Nicosia::Buffer::Buffer): (Nicosia::Buffer::~Buffer): * Source/WebCore/platform/graphics/nicosia/NicosiaBuffer.h: * Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp: (WebCore::CoordinatedGraphicsLayer::backingStoreMemoryEstimate const): * Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h: Canonical link: https://commits.webkit.org/266424@main
674c035
to
3ed63da
Compare
Committed 266424@main (3ed63da): https://commits.webkit.org/266424@main Reviewed commits have been landed. Closing PR #14286 and removing active labels. |
3ed63da
674c035