[GC] Lift UpdateMemoryUsage to CanvasRenderingContextHost [chromium/src : main]

[Etienne Pierre-Doray (Gerrit)]

Attention needed from Jean-Philippe Gravel

Etienne Pierre-Doray added 1 comment

Patchset-level comments

File-level comment, Patchset 2 (Latest):

Etienne Pierre-Doray . resolved

PTAL

Open in Gerrit

Related details

Attention is currently required from:

• Jean-Philippe Gravel

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I9bdf2da7f40c5ccd5658b08f058125a01af899e7

Gerrit-Change-Number: 6960916

Gerrit-PatchSet: 2

Gerrit-Owner: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>

Gerrit-Comment-Date: Thu, 18 Sep 2025 20:37:32 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Jean-Philippe Gravel (Gerrit)]

Attention needed from Etienne Pierre-Doray

Jean-Philippe Gravel added 6 comments

File third_party/blink/renderer/core/html/canvas/canvas_rendering_context_host.h

Line 180, Patchset 2 (Latest): NO_UNIQUE_ADDRESS V8ExternalMemoryAccounterBase external_memory_accounter_;

Jean-Philippe Gravel . unresolved

Why is this `NO_UNIQUE_ADDRESS`? Maybe a comment explaining that would be useful?

Line 179, Patchset 2 (Latest): mutable intptr_t externally_allocated_memory_;

Jean-Philippe Gravel . unresolved

It doesn't need to be `mutable` if `UpdateMemoryUsage` is not `const`. Right?

File third_party/blink/renderer/core/html/canvas/html_canvas_element.h

Line 467, Patchset 2 (Latest): // GPU Memory Management
mutable intptr_t externally_allocated_memory_;

Jean-Philippe Gravel . unresolved

Remove.

File third_party/blink/renderer/core/html/canvas/html_canvas_element.cc

Line 330, Patchset 2 (Latest): externally_allocated_memory_(0) {

Jean-Philippe Gravel . unresolved

Remove.

File third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.h

Line 117, Patchset 2 (Parent): // TODO(fserb): Merge this with HTMLCanvasElement::UpdateMemoryUsage

Jean-Philippe Gravel . resolved

And there was a TODO this whole time!
Thanks for taking care of it!

File third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc

Line 103, Patchset 2 (Latest):OffscreenCanvas::~OffscreenCanvas() = default;

Jean-Philippe Gravel . unresolved

Remove? There's no need to explicitly declare and define a default constructor, is there? Is this because it would otherwise trigger the compiler check preventing large functions to be inlined?

Open in Gerrit

Related details

Attention is currently required from:

• Etienne Pierre-Doray

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I9bdf2da7f40c5ccd5658b08f058125a01af899e7

Gerrit-Change-Number: 6960916

Gerrit-PatchSet: 2

Gerrit-Owner: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Comment-Date: Thu, 18 Sep 2025 20:58:32 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

[Etienne Pierre-Doray (Gerrit)]

Attention needed from Jean-Philippe Gravel

Etienne Pierre-Doray added 6 comments

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Etienne Pierre-Doray . resolved

PTAnL

File third_party/blink/renderer/core/html/canvas/canvas_rendering_context_host.h

Line 180, Patchset 2: NO_UNIQUE_ADDRESS V8ExternalMemoryAccounterBase external_memory_accounter_;

Jean-Philippe Gravel . resolved

Why is this `NO_UNIQUE_ADDRESS`? Maybe a comment explaining that would be useful?

Etienne Pierre-Doray

Added a comment, this is so that dcheck only things don't make the object bigger in release builds.

Line 179, Patchset 2: mutable intptr_t externally_allocated_memory_;

Jean-Philippe Gravel . resolved

It doesn't need to be `mutable` if `UpdateMemoryUsage` is not `const`. Right?

Etienne Pierre-Doray

Done

File third_party/blink/renderer/core/html/canvas/html_canvas_element.h

Line 467, Patchset 2: // GPU Memory Management
mutable intptr_t externally_allocated_memory_;

Jean-Philippe Gravel . resolved

Remove.

Etienne Pierre-Doray

Done

File third_party/blink/renderer/core/html/canvas/html_canvas_element.cc

Line 330, Patchset 2: externally_allocated_memory_(0) {

Jean-Philippe Gravel . resolved

Remove.

Etienne Pierre-Doray

Done

File third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.cc

Line 103, Patchset 2:OffscreenCanvas::~OffscreenCanvas() = default;

Jean-Philippe Gravel . resolved

Remove? There's no need to explicitly declare and define a default constructor, is there? Is this because it would otherwise trigger the compiler check preventing large functions to be inlined?

Etienne Pierre-Doray

I thought it would trip for virtual methods, but apparently not.

Open in Gerrit

Related details

Attention is currently required from:

• Jean-Philippe Gravel

Submit Requirements:

• Code-Coverage
• Code-Owners
• Code-Review

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I9bdf2da7f40c5ccd5658b08f058125a01af899e7

Gerrit-Change-Number: 6960916

Gerrit-PatchSet: 4

Gerrit-Owner: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Jean-Philippe Gravel <jpgr...@chromium.org>

Gerrit-Comment-Date: Thu, 18 Sep 2025 21:30:28 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Jean-Philippe Gravel <jpgr...@chromium.org>

[Jean-Philippe Gravel (Gerrit)]

Attention needed from Etienne Pierre-Doray

Jean-Philippe Gravel voted and added 3 comments

Votes added by Jean-Philippe Gravel

Code-Review

+1

3 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Jean-Philippe Gravel . resolved

Thanks!

File third_party/blink/renderer/core/html/canvas/canvas_rendering_context_host.h

Line 180, Patchset 2: NO_UNIQUE_ADDRESS V8ExternalMemoryAccounterBase external_memory_accounter_;

Jean-Philippe Gravel . unresolved

Why is this `NO_UNIQUE_ADDRESS`? Maybe a comment explaining that would be useful?

Etienne Pierre-Doray

Added a comment, this is so that dcheck only things don't make the object bigger in release builds.

Jean-Philippe Gravel

Ha, make sense. I code-searched wrong and thought we were using `V8ExternalMemoryAccounter`, which would consume memory even in release builds.

Which makes me think (idea for a follow-up). Why aren't we not using `V8ExternalMemoryAccounter`? Using a base class here is strange. Using the child class would hide the `NO_UNIQUE_ADDRESS` complexity and delta calculation away. The fact that users of the class need to use `NO_UNIQUE_ADDRESS` breaks encapsulation. Users need to know that the class has no members and if that ever changes, all users will need to be updated or else you'll have a bug. Then, all of the fuss around not being allowed to increase the memory size while GC is disabled is a concern all users of `V8ExternalMemoryAccounter` should have. Why not centralize all of this in one place?

File third_party/blink/renderer/core/offscreencanvas/offscreen_canvas.h

Line 51, Patchset 5 (Latest): ~OffscreenCanvas() override = default;

Jean-Philippe Gravel . unresolved

Still. This is not needed either. Isn't it? The compiler automatically generates a default constructor if you don't provide one. See for instance: https://godbolt.org/z/9aaMdTc1n

Open in Gerrit

Related details

Attention is currently required from:

• Etienne Pierre-Doray

Submit Requirements:

• Code-Coverage
• Code-Owners

• Code-Review
• No-Unresolved-Comments

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: chromium/src

Gerrit-Branch: main

Gerrit-Change-Id: I9bdf2da7f40c5ccd5658b08f058125a01af899e7

Gerrit-Change-Number: 6960916

Gerrit-PatchSet: 5

Gerrit-Owner: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Reviewer: Jean-Philippe Gravel <jpgr...@chromium.org>

Gerrit-CC: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-CC: chromium...@chromium.org

Gerrit-Attention: Etienne Pierre-Doray <etie...@chromium.org>

Gerrit-Comment-Date: Fri, 19 Sep 2025 16:31:22 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

Comment-In-Reply-To: Jean-Philippe Gravel <jpgr...@chromium.org>

Comment-In-Reply-To: Etienne Pierre-Doray <etie...@chromium.org>