[GC] Use context size in external memory [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 1 comment![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Commit-Queue | +1 |
1 comment
Related details
- Colin Blundell
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 5 comments![]( "Open in Gerrit")
return RenderingContext() ? RenderingContext()->DrawingBufferSize()
: gfx::Size();I think that this condition is misleading and unexpected. Calling this method before or after creating the context would return different values. In practice, it means that if you create an OffscreenCanvas with a very large size as in [drawingbuffer-static-canvas-test.html](https://crsrc.org/c/third_party/webgl/src/sdk/tests/conformance/canvas/drawingbuffer-static-canvas-test.html), `UpdateMemoryUsage` would calculate a very large size on OffscreenCanvas creation because it doesn't yet have a context. (You might want to add test coverage for this.)
I think that the issue you have here is that [`OffscreenCanvas::OffscreenCanvas` calls `UpdateMemoryUsage`](https://crsrc.org/c/third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc;l=92;drc=cf0ed76712edaa26ec78f209f42c1a6169ccb159), before a context is created. Is this expected? Could you remove this call and instead calculate the memory later when we have a context and we actually allocate resources, like we do for HtmlCanvasElement?
UpdateMemoryUsage();Is this needed for WebGL? I don't think we'd want it for 2d contexts. I think that for 2D contexts, a resize should release memory and the resources will only be re-created if/when the canvas is drawn to later. In fact, we should probably not even reallocate resources on draws because 2D draw APIs just add ops to the recording. We could one day decide to only allocate the backing buffer on flushes.
auto canvas_size = GetDrawingBufferSize();Don't use `auto` “merely to avoid the inconvenience of writing an explicit type”.
https://google.github.io/styleguide/cppguide.html#Type_deduction
auto canvas_size = GetDrawingBufferSize();Same (don't use `auto`).
auto canvas_size = GetDrawingBufferSize();This will crash for "bitmaprenderer" and "webgpu" contexts. It looks like we are missing test coverage?
`HTMLCanvasElement::UpdateMemoryUsage()` only accounts for memory used by `2d` or `webgl` contexts, but `OffscreenCanvas::UpdateMemoryUsage()` accounts for all context types. Which should it be? Both should behave the same.
Related details
- Colin Blundell
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Thanks! I'll hold off pending working through JP's comments.
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
gfx::Size GetDrawingBufferSize() const;nit: Per the Blink Style Guide, prefer bare words for getters unless the name conflicts with a type. Please consider renaming `GetDrawingBufferSize` to `DrawingBufferSize`. (Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reason
This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 6 comments![]( "Open in Gerrit")
return RenderingContext() ? RenderingContext()->DrawingBufferSize()
: gfx::Size();I think that this condition is misleading and unexpected. Calling this method before or after creating the context would return different values. In practice, it means that if you create an OffscreenCanvas with a very large size as in [drawingbuffer-static-canvas-test.html](https://crsrc.org/c/third_party/webgl/src/sdk/tests/conformance/canvas/drawingbuffer-static-canvas-test.html), `UpdateMemoryUsage` would calculate a very large size on OffscreenCanvas creation because it doesn't yet have a context. (You might want to add test coverage for this.)
I think that the issue you have here is that [`OffscreenCanvas::OffscreenCanvas` calls `UpdateMemoryUsage`](https://crsrc.org/c/third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc;l=92;drc=cf0ed76712edaa26ec78f209f42c1a6169ccb159), before a context is created. Is this expected? Could you remove this call and instead calculate the memory later when we have a context and we actually allocate resources, like we do for HtmlCanvasElement?
it means that if you create an OffscreenCanvas with a very large size as in drawingbuffer-static-canvas-test.html, UpdateMemoryUsage would calculate a very large size on OffscreenCanvas
When RenderingContext() == nullptr, we return an empty size, thus the compute size is 0. This might not be the most accurate, but at least it's consistent with the fact we don't reserve memory without a context AFAIU.
I think that the issue you have here is that OffscreenCanvas::OffscreenCanvas calls UpdateMemoryUsage
I can remove that one call, but beyond that, I think UpdateMemoryUsage should be idempotent: there's too many call to it to convince myself it's called at the right point, so the impl should compute a size 0 when there's no context (and it's impossible to have memory reserved).
UpdateMemoryUsage();Is this needed for WebGL? I don't think we'd want it for 2d contexts. I think that for 2D contexts, a resize should release memory and the resources will only be re-created if/when the canvas is drawn to later. In fact, we should probably not even reallocate resources on draws because 2D draw APIs just add ops to the recording. We could one day decide to only allocate the backing buffer on flushes.
I moved UpdateMemoryUsage() into the condition guarding Reshape().
In fact, we should probably not even reallocate resources on draws because 2D draw APIs just add ops to the recording.
The function simply updates the compute size as a delta over the previous size; You're saying this isn't the right time to update the size?
auto canvas_size = GetDrawingBufferSize();Don't use `auto` “merely to avoid the inconvenience of writing an explicit type”.
https://google.github.io/styleguide/cppguide.html#Type_deduction
Done
auto canvas_size = GetDrawingBufferSize();This will crash for "bitmaprenderer" and "webgpu" contexts. It looks like we are missing test coverage?
`HTMLCanvasElement::UpdateMemoryUsage()` only accounts for memory used by `2d` or `webgl` contexts, but `OffscreenCanvas::UpdateMemoryUsage()` accounts for all context types. Which should it be? Both should behave the same.
This will crash for "bitmaprenderer" and "webgpu" contexts
How so? I implemented the GetDrawingBufferSize() for all contexts in CanvasRenderingContext.
Which should it be? Both should behave the same.
I'm guessing we can do it for all contexts; OffscreenCanvas::UpdateMemoryUsage() was added later and it's not clear why HTMLCanvasElement only reports for certain contexts.
auto canvas_size = GetDrawingBufferSize();Same (don't use `auto`).
- Jean-Philippe Gravel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 11 comments![]( "Open in Gerrit")
virtual gfx::Size DrawingBufferSize() const { return Host()->Size(); }`Host()` can be nullptr after `Dispose()` is called. You shouldn't dereference it without checking it first. The current caller does check beforehand, but you never know what new callers you'll get in the future... Maybe return `gfx::Size(0, 0)`, like `WebGLRenderingContextBase::DrawingBufferSize()` does?
virtual intptr_t AllocatedBufferSize() const;Why is `AllocatedBufferSize` virtual? I don't see who's overriding it.
if (!Host() || isContextLost()) {There should definitely be unit tests validating this stuff. Checking this here is fine, but the rest of the code also has to be wired-up correctly to actually update the memory usage when the context is lost or disposed. These edge cases, context loss in particular, are rarely encountered in normal use (or rather, on high end development machines like ours). Consequently, the context lost logic did suffer from code rot in the past and it has been very complex to fix years down the line. Unit test would ensure that these conditions all around the code remain valid. It would also help knowing whether these conditions are still useful in the future (removing them would cause a test to fail).
if (!Host() || isContextLost()) {This doesn't account for the case where the a 2D canvas still doesn't have a resource provider and thus doesn't have any allocated buffer.
checked_usage *= std::min(kMaximumCanvasSize, canvas_size.width());I thought that your original intent was to remove this fake limit. Why is it being reintroduced?
It is a very strange limit to use. Why is it so incredibly large? A canvas of size [kMaximumCanvasSize, kMaximumCanvasSize] would be terabytes in size. Is such limit even useful to use?
UpdateMemoryUsage();Etienne Pierre-DorayIs this needed for WebGL? I don't think we'd want it for 2d contexts. I think that for 2D contexts, a resize should release memory and the resources will only be re-created if/when the canvas is drawn to later. In fact, we should probably not even reallocate resources on draws because 2D draw APIs just add ops to the recording. We could one day decide to only allocate the backing buffer on flushes.
I moved UpdateMemoryUsage() into the condition guarding Reshape().
In fact, we should probably not even reallocate resources on draws because 2D draw APIs just add ops to the recording.
The function simply updates the compute size as a delta over the previous size; You're saying this isn't the right time to update the size?
You're saying this isn't the right time to update the size?
Nah. I was describing a potential future where we could decide the allocate the 2D canvas bitmap lazyly, only at the very last minute when we are about to flush the recording. If we ever do that, the canvas wouldn't hold external memory until then, and maybe never if we never flush.
if (disposing_ || !context_) {From the comment below: the `disposing_` check was referring to a call to `AdjustAmountOfExternalAllocatedMemory` which has been removed. Is the `disposing_` check still relevant? If it was left there by mistake, it might be worth getting rid of id (especially since this is the only usage of this attribute). As far as calculating the memory usage goes, it shouldn't make a difference whether we are disposing or not. If we are, the host will be detached, the resource provider will be cleared, etc. These could be enough to know that the canvas no longer uses memory.
external_memory_accounter_.Decrease(v8::Isolate::GetCurrent(),
externally_allocated_memory_);
externally_allocated_memory_ = 0;
return;Instead of special-casing the `external_memory_accounter_` management here, could we not simply set `externally_allocated_memory` to 0 if we don't have a context and let the rest of the function do the job?
// AdjustAmountOfExternalAllocatedMemory is not an allocation but itAre the comment and `DCHECK` still relevant? The call to `AdjustAmountOfExternalAllocatedMemory` was [removed in this cl](https://chromium-review.googlesource.com/c/chromium/src/+/5856486/16/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc#1807).
void OffscreenCanvas::UpdateMemoryUsage() {It looks like the two `UpdateMemoryUsage` implementation are now identical. Or am I missing something? Could we move the whole thing to `CanvasRenderingContextHost`? (Could be done in a follow up, it's preferable to avoid mixing code move and code changes.)
auto canvas_size = GetDrawingBufferSize();Etienne Pierre-DorayThis will crash for "bitmaprenderer" and "webgpu" contexts. It looks like we are missing test coverage?
`HTMLCanvasElement::UpdateMemoryUsage()` only accounts for memory used by `2d` or `webgl` contexts, but `OffscreenCanvas::UpdateMemoryUsage()` accounts for all context types. Which should it be? Both should behave the same.
This will crash for "bitmaprenderer" and "webgpu" contexts
How so? I implemented the GetDrawingBufferSize() for all contexts in CanvasRenderingContext.
Which should it be? Both should behave the same.
I'm guessing we can do it for all contexts; OffscreenCanvas::UpdateMemoryUsage() was added later and it's not clear why HTMLCanvasElement only reports for certain contexts.
How so? I implemented the GetDrawingBufferSize() for all contexts in CanvasRenderingContext.
Because `GetDrawingBufferSize()` was calling `RenderingContext()->DrawingBufferSize()`, which wasn't defined for all contexts. The latest patch set looks fine though.
I'm guessing we can do it for all contexts; OffscreenCanvas::UpdateMemoryUsage() was added later and it's not clear why HTMLCanvasElement only reports for certain contexts.
I wouldn't assume OffscreenCanvas is correct because it's more recent. In fact, when OffscreenCanvas and HtmlCanvasElement disagree, HtmlCanvasElement is usually right. It's the more used and tested of the two. (And as you found out, engineers tend to forget about OffscreenCanvas!)
In any case, when `OffscreenCanvas::UpdateMemoryUsage` was implemented (in https://crrev.com/c/1297067) it somehow got a very different version than `HtmlCanvasElement` and the context type check wasn't included. So, it's not like their implementation diverged because someone forgot to update one or the other. So, why were they implemented differently from the start? I don't know...
It does seem to me that `UpdateMemoryUsage` should work for WebGPU. I could easily imagine that they forgot to update `UpdateMemoryUsage` when they implemented WebGPU.
For `bitmaprenderer` contexts, it does seem like we should be accounting memory, but I think that we are missing a few building blocks. The `ImageBitmap` class does track its memory via [`ImageBitmap::UpdateImageBitmapMemoryUsage()`](https://crsrc.org/c/third_party/blink/renderer/core/imagebitmap/image_bitmap.cc;l=541;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18). When we do `context.transferFromImageBitmap(image_bitmap)`, [we transfer that bitmap over](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/imagebitmap/image_bitmap_rendering_context.cc;l=93;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18), and [clear the memory usage tracked by `ImageBitmap`](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/imagebitmap/image_bitmap_rendering_context.cc;l=101;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18). But it doesn't look like we update the memory used by the canvas at this point.
So, yes, it does look like `UpdateMemoryUsage` should work for `bitmaprenderer` contexts, but maybe you'll need follow-up CLs to make it work correctly.
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted and added 11 comments![]( "Open in Gerrit")
Votes added by Etienne Pierre-Doray
| Commit-Queue | +1 |
11 comments
virtual gfx::Size DrawingBufferSize() const { return Host()->Size(); }`Host()` can be nullptr after `Dispose()` is called. You shouldn't dereference it without checking it first. The current caller does check beforehand, but you never know what new callers you'll get in the future... Maybe return `gfx::Size(0, 0)`, like `WebGLRenderingContextBase::DrawingBufferSize()` does?
Done
Nit: gfx::Size() == gfx::Size(0,0)
Why is `AllocatedBufferSize` virtual? I don't see who's overriding it.
No need for virtual; I was going to have overwrite but ended up only having this one impl.
if (!Host() || isContextLost()) {There should definitely be unit tests validating this stuff. Checking this here is fine, but the rest of the code also has to be wired-up correctly to actually update the memory usage when the context is lost or disposed. These edge cases, context loss in particular, are rarely encountered in normal use (or rather, on high end development machines like ours). Consequently, the context lost logic did suffer from code rot in the past and it has been very complex to fix years down the line. Unit test would ensure that these conditions all around the code remain valid. It would also help knowing whether these conditions are still useful in the future (removing them would cause a test to fail).
I added unittests in HTMLCanvasElementTest. What I'm testing:
- only 2d context: this mostly tests the common CanvasRenderingContext::AllocatedBufferSize
- When there's no context, memory is 0
- When creating a canvas unreasonably big, memory is 0
What I'm not testing:
- Size computation for every CanvasRenderingContext type.
- OffscreenCanvas: since I'll merge the impl in a follow-up, this mostly seems like idle work.
This doesn't account for the case where the a 2D canvas still doesn't have a resource provider and thus doesn't have any allocated buffer.
We don't have knowledge of provider here, but this is done through AllocatedBufferCountPerPixel():
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.h;l=195?q=AllocatedBufferCountPerPixel&ss=chromium
checked_usage *= std::min(kMaximumCanvasSize, canvas_size.width());I thought that your original intent was to remove this fake limit. Why is it being reintroduced?
It is a very strange limit to use. Why is it so incredibly large? A canvas of size [kMaximumCanvasSize, kMaximumCanvasSize] would be terabytes in size. Is such limit even useful to use?
I thought that your original intent was to remove this fake limit.
That's not my end goal: I want to fix the conformance/canvas/drawingbuffer-static-canvas-test.html crash, and this is done by using DrawingBufferSize();
For contexts other than webgl, there's still nothing that caps the buffer size, other than this kMaximumCanvasSize. On the flip side, I don't have a good reason of removing it, other than "it might not not be needed anymore".
if (disposing_ || !context_) {From the comment below: the `disposing_` check was referring to a call to `AdjustAmountOfExternalAllocatedMemory` which has been removed. Is the `disposing_` check still relevant? If it was left there by mistake, it might be worth getting rid of id (especially since this is the only usage of this attribute). As far as calculating the memory usage goes, it shouldn't make a difference whether we are disposing or not. If we are, the host will be detached, the resource provider will be cleared, etc. These could be enough to know that the canvas no longer uses memory.
There's a few things to consider:
- We'll consider context_ being null => externally_allocated_memory = 0
- if disposing_, ThreadState::Current() might crash in unittests (crbug.com/438132028); this appeared to only happen in OffscreenCanvas so far.
- if disposing_, we can't CHECK IsAllocationAllowed() because we might be inside a GC -> that's ok because delta_bytes <= 0 and we won't allocate.
An alternative impl is to use `externally_allocated_memory == 0` (see latest patchset). Personally I feel like using `disposing_ || !context_` is more explicit but I don't feel strongly.
external_memory_accounter_.Decrease(v8::Isolate::GetCurrent(),
externally_allocated_memory_);
externally_allocated_memory_ = 0;
return;Instead of special-casing the `external_memory_accounter_` management here, could we not simply set `externally_allocated_memory` to 0 if we don't have a context and let the rest of the function do the job?
See above.
// AdjustAmountOfExternalAllocatedMemory is not an allocation but itAre the comment and `DCHECK` still relevant? The call to `AdjustAmountOfExternalAllocatedMemory` was [removed in this cl](https://chromium-review.googlesource.com/c/chromium/src/+/5856486/16/third_party/blink/renderer/core/html/canvas/html_canvas_element.cc#1807).
Yes, although renamed to ExternalMemoryAccounter::Update; I updated the comment.
It looks like the two `UpdateMemoryUsage` implementation are now identical. Or am I missing something? Could we move the whole thing to `CanvasRenderingContextHost`? (Could be done in a follow up, it's preferable to avoid mixing code move and code changes.)
It looks like the two UpdateMemoryUsage implementation are now identical.
Yes, I did that on purpose.
Could we move the whole thing to CanvasRenderingContextHost?
Yes, as a follow-up: `context` lives in derived classes so merging implies more change than I'd like to do in this CL.
auto canvas_size = GetDrawingBufferSize();Etienne Pierre-DorayThis will crash for "bitmaprenderer" and "webgpu" contexts. It looks like we are missing test coverage?
`HTMLCanvasElement::UpdateMemoryUsage()` only accounts for memory used by `2d` or `webgl` contexts, but `OffscreenCanvas::UpdateMemoryUsage()` accounts for all context types. Which should it be? Both should behave the same.
Jean-Philippe GravelThis will crash for "bitmaprenderer" and "webgpu" contexts
How so? I implemented the GetDrawingBufferSize() for all contexts in CanvasRenderingContext.
Which should it be? Both should behave the same.
I'm guessing we can do it for all contexts; OffscreenCanvas::UpdateMemoryUsage() was added later and it's not clear why HTMLCanvasElement only reports for certain contexts.
How so? I implemented the GetDrawingBufferSize() for all contexts in CanvasRenderingContext.
Because `GetDrawingBufferSize()` was calling `RenderingContext()->DrawingBufferSize()`, which wasn't defined for all contexts. The latest patch set looks fine though.
I'm guessing we can do it for all contexts; OffscreenCanvas::UpdateMemoryUsage() was added later and it's not clear why HTMLCanvasElement only reports for certain contexts.
I wouldn't assume OffscreenCanvas is correct because it's more recent. In fact, when OffscreenCanvas and HtmlCanvasElement disagree, HtmlCanvasElement is usually right. It's the more used and tested of the two. (And as you found out, engineers tend to forget about OffscreenCanvas!)
In any case, when `OffscreenCanvas::UpdateMemoryUsage` was implemented (in https://crrev.com/c/1297067) it somehow got a very different version than `HtmlCanvasElement` and the context type check wasn't included. So, it's not like their implementation diverged because someone forgot to update one or the other. So, why were they implemented differently from the start? I don't know...
It does seem to me that `UpdateMemoryUsage` should work for WebGPU. I could easily imagine that they forgot to update `UpdateMemoryUsage` when they implemented WebGPU.
For `bitmaprenderer` contexts, it does seem like we should be accounting memory, but I think that we are missing a few building blocks. The `ImageBitmap` class does track its memory via [`ImageBitmap::UpdateImageBitmapMemoryUsage()`](https://crsrc.org/c/third_party/blink/renderer/core/imagebitmap/image_bitmap.cc;l=541;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18). When we do `context.transferFromImageBitmap(image_bitmap)`, [we transfer that bitmap over](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/imagebitmap/image_bitmap_rendering_context.cc;l=93;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18), and [clear the memory usage tracked by `ImageBitmap`](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/imagebitmap/image_bitmap_rendering_context.cc;l=101;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18). But it doesn't look like we update the memory used by the canvas at this point.
So, yes, it does look like `UpdateMemoryUsage` should work for `bitmaprenderer` contexts, but maybe you'll need follow-up CLs to make it work correctly.
It does seem to me that UpdateMemoryUsage should work for WebGPU. I could easily imagine that they forgot to update UpdateMemoryUsage when they implemented WebGPU.
Yes that's kinda what I meant, the early HTMLCanvasElement::UpdateMemoryUsage [1] had per context check:
```
if (Is2d() && canvas2d_bridge_) {
...
}
if (Is3d()) {
...
}
if (gpu_buffer_count && !gpu_memory_usage_) {
...
}
```
So I feel like that just evolved into what we have today that ignores context that didn't exist back then.
But it doesn't look like we update the memory used by the canvas at this point.
Interesting; presumably canvas gets a size update at some point when doing transferFromImageBitmap?
Related details
- Jean-Philippe Gravel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 8 comments![]( "Open in Gerrit")
if (disposing_ || !context_) {Etienne Pierre-DorayFrom the comment below: the `disposing_` check was referring to a call to `AdjustAmountOfExternalAllocatedMemory` which has been removed. Is the `disposing_` check still relevant? If it was left there by mistake, it might be worth getting rid of id (especially since this is the only usage of this attribute). As far as calculating the memory usage goes, it shouldn't make a difference whether we are disposing or not. If we are, the host will be detached, the resource provider will be cleared, etc. These could be enough to know that the canvas no longer uses memory.
There's a few things to consider:
- We'll consider context_ being null => externally_allocated_memory = 0
- if disposing_, ThreadState::Current() might crash in unittests (crbug.com/438132028); this appeared to only happen in OffscreenCanvas so far.
- if disposing_, we can't CHECK IsAllocationAllowed() because we might be inside a GC -> that's ok because delta_bytes <= 0 and we won't allocate.
An alternative impl is to use `externally_allocated_memory == 0` (see latest patchset). Personally I feel like using `disposing_ || !context_` is more explicit but I don't feel strongly.
If it's simpler to check for `disposing_` and allows removing the `externally_allocated_memory == 0` check, we can do that. See other comments below.
Something to note however. The original code was using `disposing_` in a `DCHECK`. In other words, the whole `disposing_` code is just for debug and never used in production. Can we settle on a solution that works on release build?
external_memory_accounter_.Decrease(v8::Isolate::GetCurrent(),
externally_allocated_memory_);
externally_allocated_memory_ = 0;I meant that the original code should have done this automatically, without needing a special case for `externally_allocated_memory == 0`. The original case would have done:
```
intptr_t externally_allocated_memory = 0;
// Sets `delta_bytes` to `-externally_allocated_memory_`.
intptr_t delta_bytes = externally_allocated_memory - externally_allocated_memory_;
// Substracts `external_memory_accounter_` down to 0.
external_memory_accounter_.Update(v8::Isolate::GetCurrent(), delta_bytes);
// Sets to 0.
externally_allocated_memory_ = externally_allocated_memory;
```
EXPECT_EQ(10u * 10u * /* Buffer Count */ 2u * /* Bytes per pixel */ 2u,While you are here, you should add a version of this test that uses a GPU accelerated canvas. Just add this at the beginning the test test body:
```
auto raster_context_provider = viz::TestContextProvider::CreateRaster();
raster_context_provider->UnboundTestRasterInterface()->set_gpu_rasterization(
true);
InitializeSharedGpuContextRaster(raster_context_provider.get());
ScopedTestingPlatformSupport<GpuMemoryBufferTestPlatform>
accelerated_platform;
GetDocument().GetSettings()->SetAcceleratedCompositingEnabled(true);
```
EXPECT_EQ(10u * 10u * /* Buffer Count */ 2u * /* Bytes per pixel */ 2u,This can't be right. There should be 4 bytes per pixels. This canvas isn't accelerated, so you should have `/* Buffer Count */ 1u * /* Bytes per pixel */ 4u`?
EXPECT_EQ(10u * 10u * /* Buffer Count */ 2u * /* Bytes per pixel */ 2u,Just a thought while were here. Maybe `GetMemoryUsage()` shouldn't return `size_t`.
This is what the style guide has to say about unsigned integers:
“Unsigned integers are good for representing bitfields and modular arithmetic. Because of historical accident, the C++ standard also uses unsigned integers to represent the size of containers - many members of the standards body believe this to be a mistake, but it is effectively impossible to fix at this point. The fact that unsigned arithmetic doesn't model the behavior of a simple integer, but is instead defined by the standard to model modular arithmetic (wrapping around on overflow/underflow), means that a significant class of bugs cannot be diagnosed by the compiler. In other cases, the defined behavior impedes optimization.
That said, mixing signedness of integer types is responsible for an equally large class of problems. The best advice we can provide: try to use iterators and containers rather than pointers and sizes, try not to mix signedness, and try to avoid unsigned types (except for representing bitfields or modular arithmetic). Do not use an unsigned type merely to assert that a variable is non-negative.”
https://google.github.io/styleguide/cppguide.html#Integer_Types
canvas.width = 1000000;This test is interesting I suppose, but it's not loosing an existing context, the GPU context just never existed because the size was invalid in the first place.
You should add another test that does:
```
EXPECT_EQ(10u * 10u * /* Buffer Count */ 1u * /* Bytes per pixel */ 4u,
canvas->GetMemoryUsage());
canvas->NotifyGpuContextLost();
EXPECT_EQ(0u, canvas->GetMemoryUsage());
```
Interesting; presumably canvas gets a size update at some point when doing transferFromImageBitmap?
The size isn't changed. The bitmap is just rendered stretched to the size of the canvas. See for instance:
https://jsfiddle.net/qtonjusx/
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 12 comments![]( "Open in Gerrit")
if (disposing_ || !context_) {Etienne Pierre-DorayFrom the comment below: the `disposing_` check was referring to a call to `AdjustAmountOfExternalAllocatedMemory` which has been removed. Is the `disposing_` check still relevant? If it was left there by mistake, it might be worth getting rid of id (especially since this is the only usage of this attribute). As far as calculating the memory usage goes, it shouldn't make a difference whether we are disposing or not. If we are, the host will be detached, the resource provider will be cleared, etc. These could be enough to know that the canvas no longer uses memory.
Jean-Philippe GravelThere's a few things to consider:
- We'll consider context_ being null => externally_allocated_memory = 0
- if disposing_, ThreadState::Current() might crash in unittests (crbug.com/438132028); this appeared to only happen in OffscreenCanvas so far.
- if disposing_, we can't CHECK IsAllocationAllowed() because we might be inside a GC -> that's ok because delta_bytes <= 0 and we won't allocate.
An alternative impl is to use `externally_allocated_memory == 0` (see latest patchset). Personally I feel like using `disposing_ || !context_` is more explicit but I don't feel strongly.
If it's simpler to check for `disposing_` and allows removing the `externally_allocated_memory == 0` check, we can do that. See other comments below.
Something to note however. The original code was using `disposing_` in a `DCHECK`. In other words, the whole `disposing_` code is just for debug and never used in production. Can we settle on a solution that works on release build?
See below.
external_memory_accounter_.Decrease(v8::Isolate::GetCurrent(),
externally_allocated_memory_);
externally_allocated_memory_ = 0;I meant that the original code should have done this automatically, without needing a special case for `externally_allocated_memory == 0`. The original case would have done:
```
intptr_t externally_allocated_memory = 0;// Sets `delta_bytes` to `-externally_allocated_memory_`.
intptr_t delta_bytes = externally_allocated_memory - externally_allocated_memory_;// Substracts `external_memory_accounter_` down to 0.
external_memory_accounter_.Update(v8::Isolate::GetCurrent(), delta_bytes);// Sets to 0.
externally_allocated_memory_ = externally_allocated_memory;
```
Discussed offline: we want to keep the CHECK on IsAllocationAllowed; ultimately what matters is whether the delta is positive.
EXPECT_EQ(10u * 10u * /* Buffer Count */ 2u * /* Bytes per pixel */ 2u,Just a thought while were here. Maybe `GetMemoryUsage()` shouldn't return `size_t`.
This is what the style guide has to say about unsigned integers:
“Unsigned integers are good for representing bitfields and modular arithmetic. Because of historical accident, the C++ standard also uses unsigned integers to represent the size of containers - many members of the standards body believe this to be a mistake, but it is effectively impossible to fix at this point. The fact that unsigned arithmetic doesn't model the behavior of a simple integer, but is instead defined by the standard to model modular arithmetic (wrapping around on overflow/underflow), means that a significant class of bugs cannot be diagnosed by the compiler. In other cases, the defined behavior impedes optimization.
That said, mixing signedness of integer types is responsible for an equally large class of problems. The best advice we can provide: try to use iterators and containers rather than pointers and sizes, try not to mix signedness, and try to avoid unsigned types (except for representing bitfields or modular arithmetic). Do not use an unsigned type merely to assert that a variable is non-negative.”
https://google.github.io/styleguide/cppguide.html#Integer_Types
I'll punt this to https://chromium-review.googlesource.com/c/chromium/src/+/6960916 since there'll be only one place to update.
EXPECT_EQ(10u * 10u * /* Buffer Count */ 2u * /* Bytes per pixel */ 2u,This can't be right. There should be 4 bytes per pixels. This canvas isn't accelerated, so you should have `/* Buffer Count */ 1u * /* Bytes per pixel */ 4u`?
Done
EXPECT_EQ(10u * 10u * /* Buffer Count */ 2u * /* Bytes per pixel */ 2u,This can't be right. There should be 4 bytes per pixels. This canvas isn't accelerated, so you should have `/* Buffer Count */ 1u * /* Bytes per pixel */ 4u`?
Very true.
EXPECT_EQ(10u * 10u * /* Buffer Count */ 2u * /* Bytes per pixel */ 2u,While you are here, you should add a version of this test that uses a GPU accelerated canvas. Just add this at the beginning the test test body:
```
auto raster_context_provider = viz::TestContextProvider::CreateRaster();
raster_context_provider->UnboundTestRasterInterface()->set_gpu_rasterization(
true);
InitializeSharedGpuContextRaster(raster_context_provider.get());
ScopedTestingPlatformSupport<GpuMemoryBufferTestPlatform>
accelerated_platform;
GetDocument().GetSettings()->SetAcceleratedCompositingEnabled(true);```
Done
EXPECT_EQ(10u * 10u * /* Buffer Count */ 2u * /* Bytes per pixel */ 2u,While you are here, you should add a version of this test that uses a GPU accelerated canvas. Just add this at the beginning the test test body:
```
auto raster_context_provider = viz::TestContextProvider::CreateRaster();
raster_context_provider->UnboundTestRasterInterface()->set_gpu_rasterization(
true);
InitializeSharedGpuContextRaster(raster_context_provider.get());
ScopedTestingPlatformSupport<GpuMemoryBufferTestPlatform>
accelerated_platform;
GetDocument().GetSettings()->SetAcceleratedCompositingEnabled(true);```
Done
This test is interesting I suppose, but it's not loosing an existing context, the GPU context just never existed because the size was invalid in the first place.
You should add another test that does:
```
EXPECT_EQ(10u * 10u * /* Buffer Count */ 1u * /* Bytes per pixel */ 4u,
canvas->GetMemoryUsage());canvas->NotifyGpuContextLost();
EXPECT_EQ(0u, canvas->GetMemoryUsage());```
Done
This test is interesting I suppose, but it's not loosing an existing context, the GPU context just never existed because the size was invalid in the first place.
You should add another test that does:
```
EXPECT_EQ(10u * 10u * /* Buffer Count */ 1u * /* Bytes per pixel */ 4u,
canvas->GetMemoryUsage());canvas->NotifyGpuContextLost();
EXPECT_EQ(0u, canvas->GetMemoryUsage());```
Done
canvas.width = 1000000;Etienne Pierre-Dorayheight
Done
canvas.width = 1000000;Etienne Pierre-Dorayheight
Done
Related details
- Jean-Philippe Gravel
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jean-Philippe Gravel
| Code-Review | +1 |
1 comment
void HTMLCanvasElement::UpdateMemoryUsage() {
intptr_t externally_allocated_memory =
context_ ? context_->AllocatedBufferSize() : 0;
// Subtracting two intptr_t that are known to be positive will never
// underflow.
intptr_t delta_bytes =
externally_allocated_memory - externally_allocated_memory_;
// TODO(junov): We assume that it is impossible to be inside a FastAPICall
// from a host interface other than the rendering context. This assumption
// may need to be revisited in the future depending on how the usage of
// [NoAllocDirectCall] evolves.
// ExternalMemoryAccounter::Update() with a positive delta can trigger a GC,
// which is not allowed when `IsAllocationAllowed() == false`.
CHECK(delta_bytes < 0 || ThreadState::Current()->IsAllocationAllowed());
external_memory_accounter_.Update(v8::Isolate::GetCurrent(), delta_bytes);
externally_allocated_memory_ = externally_allocated_memory;
}
Nice! This is a lot simpler now.
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 3 comments![]( "Open in Gerrit")
if (!Host() || isContextLost()) {Etienne Pierre-DorayThere should definitely be unit tests validating this stuff. Checking this here is fine, but the rest of the code also has to be wired-up correctly to actually update the memory usage when the context is lost or disposed. These edge cases, context loss in particular, are rarely encountered in normal use (or rather, on high end development machines like ours). Consequently, the context lost logic did suffer from code rot in the past and it has been very complex to fix years down the line. Unit test would ensure that these conditions all around the code remain valid. It would also help knowing whether these conditions are still useful in the future (removing them would cause a test to fail).
I added unittests in HTMLCanvasElementTest. What I'm testing:
- only 2d context: this mostly tests the common CanvasRenderingContext::AllocatedBufferSize
- When there's no context, memory is 0
- When creating a canvas unreasonably big, memory is 0
What I'm not testing:
- Size computation for every CanvasRenderingContext type.
- OffscreenCanvas: since I'll merge the impl in a follow-up, this mostly seems like idle work.
Resolving.
checked_usage *= std::min(kMaximumCanvasSize, canvas_size.width());I thought that your original intent was to remove this fake limit. Why is it being reintroduced?
It is a very strange limit to use. Why is it so incredibly large? A canvas of size [kMaximumCanvasSize, kMaximumCanvasSize] would be terabytes in size. Is such limit even useful to use?
I thought that your original intent was to remove this fake limit.
That's not my end goal: I want to fix the conformance/canvas/drawingbuffer-static-canvas-test.html crash, and this is done by using DrawingBufferSize();
For contexts other than webgl, there's still nothing that caps the buffer size, other than this kMaximumCanvasSize. On the flip side, I don't have a good reason of removing it, other than "it might not not be needed anymore".
Resolving.
auto canvas_size = GetDrawingBufferSize();This will crash for "bitmaprenderer" and "webgpu" contexts. It looks like we are missing test coverage?
Acknowledged
Follow-up: I'll add a call to UpdateMemoryUsage from transferFromImageBitmap.
I'm still unclear on how bitmaprenderer should handle its resource provider w.r.t. buffer count.
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jean-Philippe Gravel
| Code-Review | +1 |
1 comment
if (!Host() || isContextLost()) {Etienne Pierre-DorayThere should definitely be unit tests validating this stuff. Checking this here is fine, but the rest of the code also has to be wired-up correctly to actually update the memory usage when the context is lost or disposed. These edge cases, context loss in particular, are rarely encountered in normal use (or rather, on high end development machines like ours). Consequently, the context lost logic did suffer from code rot in the past and it has been very complex to fix years down the line. Unit test would ensure that these conditions all around the code remain valid. It would also help knowing whether these conditions are still useful in the future (removing them would cause a test to fail).
Etienne Pierre-DorayI added unittests in HTMLCanvasElementTest. What I'm testing:
- only 2d context: this mostly tests the common CanvasRenderingContext::AllocatedBufferSize
- When there's no context, memory is 0
- When creating a canvas unreasonably big, memory is 0
What I'm not testing:
- Size computation for every CanvasRenderingContext type.
- OffscreenCanvas: since I'll merge the impl in a follow-up, this mostly seems like idle work.
Resolving.
Didn't see this reply.
When creating a canvas unreasonably big, memory is 0
Note that for 2D canvas, [setting an unreasonably big size will cause the context to enter the lost state](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc;l=1358-1364;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18), which as your unit test demonstrates, results in a memory usage of 0. For 2D context therefore, using a size limit here is redundant. It would be nice if a similar mechanism existed for other context types. Letting the context use too much memory and then lying about that memory usage doesn't sound like a good idea.
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jean-Philippe Gravel (Gerrit)
Jean-Philippe Gravel added 1 comment![]( "Open in Gerrit")
return RenderingContext() ? RenderingContext()->DrawingBufferSize()
: gfx::Size();I think that this condition is misleading and unexpected. Calling this method before or after creating the context would return different values. In practice, it means that if you create an OffscreenCanvas with a very large size as in [drawingbuffer-static-canvas-test.html](https://crsrc.org/c/third_party/webgl/src/sdk/tests/conformance/canvas/drawingbuffer-static-canvas-test.html), `UpdateMemoryUsage` would calculate a very large size on OffscreenCanvas creation because it doesn't yet have a context. (You might want to add test coverage for this.)
I think that the issue you have here is that [`OffscreenCanvas::OffscreenCanvas` calls `UpdateMemoryUsage`](https://crsrc.org/c/third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc;l=92;drc=cf0ed76712edaa26ec78f209f42c1a6169ccb159), before a context is created. Is this expected? Could you remove this call and instead calculate the memory later when we have a context and we actually allocate resources, like we do for HtmlCanvasElement?
it means that if you create an OffscreenCanvas with a very large size as in drawingbuffer-static-canvas-test.html, UpdateMemoryUsage would calculate a very large size on OffscreenCanvasWhen RenderingContext() == nullptr, we return an empty size, thus the compute size is 0. This might not be the most accurate, but at least it's consistent with the fact we don't reserve memory without a context AFAIU.
I think that the issue you have here is that OffscreenCanvas::OffscreenCanvas calls UpdateMemoryUsage
I can remove that one call, but beyond that, I think UpdateMemoryUsage should be idempotent: there's too many call to it to convince myself it's called at the right point, so the impl should compute a size 0 when there's no context (and it's impossible to have memory reserved).
the impl should compute a size 0 when there's no context
Totally agree.
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
if (!Host() || isContextLost()) {Etienne Pierre-DorayThere should definitely be unit tests validating this stuff. Checking this here is fine, but the rest of the code also has to be wired-up correctly to actually update the memory usage when the context is lost or disposed. These edge cases, context loss in particular, are rarely encountered in normal use (or rather, on high end development machines like ours). Consequently, the context lost logic did suffer from code rot in the past and it has been very complex to fix years down the line. Unit test would ensure that these conditions all around the code remain valid. It would also help knowing whether these conditions are still useful in the future (removing them would cause a test to fail).
Etienne Pierre-DorayI added unittests in HTMLCanvasElementTest. What I'm testing:
- only 2d context: this mostly tests the common CanvasRenderingContext::AllocatedBufferSize
- When there's no context, memory is 0
- When creating a canvas unreasonably big, memory is 0
What I'm not testing:
- Size computation for every CanvasRenderingContext type.
- OffscreenCanvas: since I'll merge the impl in a follow-up, this mostly seems like idle work.
Jean-Philippe GravelResolving.
Didn't see this reply.
When creating a canvas unreasonably big, memory is 0
Note that for 2D canvas, [setting an unreasonably big size will cause the context to enter the lost state](https://crsrc.org/c/third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.cc;l=1358-1364;drc=48d6f7175422b2c969c14258f9f8d5b196c28d18), which as your unit test demonstrates, results in a memory usage of 0. For 2D context therefore, using a size limit here is redundant. It would be nice if a similar mechanism existed for other context types. Letting the context use too much memory and then lying about that memory usage doesn't sound like a good idea.
Letting the context use too much memory and then lying about that memory usage doesn't sound like a good idea.
Very true; webgl also has a check for this.
How about I try to remove kMaximumCanvasSize in a follow up (given it might need to be reverted)?
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell voted and added 1 comment![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
Thanks!
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 1 comment![]( "Open in Gerrit")
nit: Per the Blink Style Guide, prefer bare words for getters unless the name conflicts with a type. Please consider renaming `GetDrawingBufferSize` to `DrawingBufferSize`. (Blink Style Guide: Naming - Precede setters with the word “Set”; use bare words for getters)
_To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **Won't fix**: reason | **b/<bug_id>** | **Invalid:** reasonThis comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent).AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve.[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[GC] Use context size in external memory
When adding stricter limits on external v8 memory, I hit test failure:
https://chromium-review.googlesource.com/c/v8/v8/+/6942837
(reporting 200Gb memory)
This is because HTMLCanvasElement::UpdateMemoryUsage isn't accurate
for webgl context.
This CL makes a few incremental steps to make UpdateMemoryUsage more accurate:
- Use DrawingBufferSize instead of host's size; ultimately this is what fixes the issue above for webgl.
- It also moves UpdateMemoryUsage around to makes sure it's
invoked after context_->Reshape().
- Consider memory=0 when there's no context.
- Every type of context is included in HTMLCanvasElement: this aligns with OffscreenCanvas: this uses a default impl AllocatedBufferCountPerPixel() == 1 which is a no-op in OffscreenCanvas; the caveat is that this isn't accurate for all context.
Other changes that will be done in follow-up:
- Merge OffscreenCanvas and HTMLCanvasElement impl of UpdateMemoryUsage
- Improve AllocatedBufferCountPerPixel for more context (e.g. OffscreenCanvasRenderingContext2D should get the same behavior as CanvasRenderingContext2D through their common base class)
- M third_party/blink/renderer/core/html/canvas/canvas_rendering_context.cc
- M third_party/blink/renderer/core/html/canvas/canvas_rendering_context.h
- M third_party/blink/renderer/core/html/canvas/html_canvas_element.cc
- M third_party/blink/renderer/core/html/canvas/html_canvas_element_test.cc
- M third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc
- M third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.h
- M third_party/blink/renderer/modules/canvas/canvas2d/canvas_rendering_context_2d.h
- M third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.cc
- M third_party/blink/renderer/modules/webgl/webgl_rendering_context_base.h
- M third_party/blink/renderer/modules/webgl/webgl_rendering_context_webgpu_base.cc
- M third_party/blink/renderer/modules/webgl/webgl_rendering_context_webgpu_base.h
Code-Review: +1 by Colin Blundell, +1 by Jean-Philippe Gravel
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |