Bug 182503 - Move ImageBuffer-related functionality from HTMLCanvasElement to CanvasBase
Summary: Move ImageBuffer-related functionality from HTMLCanvasElement to CanvasBase
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on:
Blocks: 182686 183720 182685
  Show dependency treegraph
 
Reported: 2018-02-05 12:42 PST by Zan Dobersek
Modified: 2019-10-31 13:43 PDT (History)
19 users (show)

See Also:


Attachments
Patch (24.29 KB, patch)
2018-02-05 13:24 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (24.24 KB, patch)
2018-02-05 13:26 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-sierra (1.64 MB, application/zip)
2018-02-05 16:04 PST, Build Bot
no flags Details
Patch (25.71 KB, patch)
2018-02-06 00:55 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-sierra (3.50 MB, application/zip)
2018-02-06 02:29 PST, Build Bot
no flags Details
Patch (26.48 KB, patch)
2018-02-06 07:53 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (26.79 KB, patch)
2018-03-01 01:47 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (27.13 KB, patch)
2019-10-01 04:15 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (27.56 KB, patch)
2019-10-07 05:34 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (28.76 KB, patch)
2019-10-07 07:46 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (28.28 KB, patch)
2019-10-07 09:10 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (28.27 KB, patch)
2019-10-10 02:54 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (28.36 KB, patch)
2019-10-11 03:42 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (26.41 KB, patch)
2019-10-16 09:40 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (26.51 KB, patch)
2019-10-17 02:30 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (26.88 KB, patch)
2019-10-28 03:51 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (26.95 KB, patch)
2019-10-30 04:08 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (35.79 KB, patch)
2019-10-31 09:49 PDT, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2018-02-05 12:42:47 PST
Move ImageBuffer-related functionality from HTMLCanvasElement to CanvasBase
Comment 1 Zan Dobersek 2018-02-05 13:24:01 PST
Created attachment 333116 [details]
Patch
Comment 2 Zan Dobersek 2018-02-05 13:26:15 PST
Created attachment 333117 [details]
Patch
Comment 3 Build Bot 2018-02-05 16:04:11 PST
Comment on attachment 333117 [details]
Patch

Attachment 333117 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6372436

Number of test failures exceeded the failure limit.
Comment 4 Build Bot 2018-02-05 16:04:12 PST
Created attachment 333130 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 Zan Dobersek 2018-02-06 00:55:32 PST
Created attachment 333158 [details]
Patch
Comment 6 Build Bot 2018-02-06 02:29:31 PST
Comment on attachment 333158 [details]
Patch

Attachment 333158 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6379587

Number of test failures exceeded the failure limit.
Comment 7 Build Bot 2018-02-06 02:29:32 PST
Created attachment 333162 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 8 Zan Dobersek 2018-02-06 07:53:43 PST
Created attachment 333174 [details]
Patch
Comment 9 Zan Dobersek 2018-03-01 01:47:06 PST
Created attachment 334801 [details]
Patch
Comment 10 Chris Lord 2019-10-01 04:15:30 PDT
Created attachment 379889 [details]
Patch
Comment 11 Chris Lord 2019-10-07 05:34:07 PDT
Created attachment 380315 [details]
Patch
Comment 12 Chris Lord 2019-10-07 07:46:51 PDT
Created attachment 380326 [details]
Patch
Comment 13 Chris Lord 2019-10-07 09:10:21 PDT
Created attachment 380331 [details]
Patch
Comment 14 Chris Lord 2019-10-08 02:46:33 PDT
Comment on attachment 380331 [details]
Patch

I don't think the failures there have anything to do with this patch (which only touches Canvas-related things), also marking cq?
Comment 15 Chris Lord 2019-10-10 02:54:04 PDT
Created attachment 380619 [details]
Patch
Comment 16 Ryosuke Niwa 2019-10-11 02:01:04 PDT
Comment on attachment 380619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380619&action=review

> Source/WebCore/html/CanvasBase.cpp:222
> +    s_activePixelMemory -= std::min(previousMemoryCost, s_activePixelMemory);

How is this thread safe?

> Source/WebCore/html/CanvasBase.cpp:234
> +    s_activePixelMemory += currentMemoryCost;

Ditto.
Comment 17 Zan Dobersek 2019-10-11 02:38:15 PDT
Comment on attachment 380619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380619&action=review

>> Source/WebCore/html/CanvasBase.cpp:222
>> +    s_activePixelMemory -= std::min(previousMemoryCost, s_activePixelMemory);
> 
> How is this thread safe?

It's not, but in the scope of these changes alone it doesn't have to be since it will still only be called only from HTMLCanvasElement.

This can be made thread-safe in a separate patch.
Comment 18 Ryosuke Niwa 2019-10-11 02:51:03 PDT
(In reply to Zan Dobersek from comment #17)
> Comment on attachment 380619 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=380619&action=review
> 
> >> Source/WebCore/html/CanvasBase.cpp:222
> >> +    s_activePixelMemory -= std::min(previousMemoryCost, s_activePixelMemory);
> > 
> > How is this thread safe?
> 
> It's not, but in the scope of these changes alone it doesn't have to be
> since it will still only be called only from HTMLCanvasElement.
> 
> This can be made thread-safe in a separate patch.

Hm... it doesn't seem great to introduce new code that's not thread safe to use concurrency in order to add a feature that requires all relevant code to be thread safe.
Comment 19 Chris Lord 2019-10-11 03:04:17 PDT
(In reply to Ryosuke Niwa from comment #18)
> (In reply to Zan Dobersek from comment #17)
> > Comment on attachment 380619 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=380619&action=review
> > 
> > >> Source/WebCore/html/CanvasBase.cpp:222
> > >> +    s_activePixelMemory -= std::min(previousMemoryCost, s_activePixelMemory);
> > > 
> > > How is this thread safe?
> > 
> > It's not, but in the scope of these changes alone it doesn't have to be
> > since it will still only be called only from HTMLCanvasElement.
> > 
> > This can be made thread-safe in a separate patch.
> 
> Hm... it doesn't seem great to introduce new code that's not thread safe to
> use concurrency in order to add a feature that requires all relevant code to
> be thread safe.

That's fair, I guess an easy fix would be to just replace s_activePixelMemory with an atomic int(?)
Comment 20 Ryosuke Niwa 2019-10-11 03:18:14 PDT
Comment on attachment 380619 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380619&action=review

>>>>> Source/WebCore/html/CanvasBase.cpp:222
>>>>> +    s_activePixelMemory -= std::min(previousMemoryCost, s_activePixelMemory);
>>>> 
>>>> How is this thread safe?
>>> 
>>> It's not, but in the scope of these changes alone it doesn't have to be since it will still only be called only from HTMLCanvasElement.
>>> 
>>> This can be made thread-safe in a separate patch.
>> 
>> Hm... it doesn't seem great to introduce new code that's not thread safe to use concurrency in order to add a feature that requires all relevant code to be thread safe.
> 
> That's fair, I guess an easy fix would be to just replace s_activePixelMemory with an atomic int(?)

Atomic int wouldn't work because by the time you've computed min, the value of s_activePixelMemory may have been updated by some other thread.
Comment 21 Chris Lord 2019-10-11 03:28:49 PDT
(In reply to Ryosuke Niwa from comment #20)
> Comment on attachment 380619 [details]
> Atomic int wouldn't work because by the time you've computed min, the value
> of s_activePixelMemory may have been updated by some other thread.

Of course, you're right - I'd like to avoid having a lock here, it seems overkill given the common case and this function is called quite often. I'm a bit dubious of this whole thing, if it's reliable at all then it should never drop below zero. I guess what this is guarding against is if previousMemoryCost is for some reason bigger than what was previously calculated as currentMemoryCost. The alternative would be to store what was previously set, then we could use an atomic int and remove min check altogether.
Comment 22 Chris Lord 2019-10-11 03:42:44 PDT
Created attachment 380739 [details]
Patch
Comment 23 Chris Lord 2019-10-16 09:40:02 PDT
Created attachment 381075 [details]
Patch
Comment 24 Chris Lord 2019-10-17 02:30:04 PDT
Created attachment 381182 [details]
Patch
Comment 25 Darin Adler 2019-10-17 09:32:36 PDT
Comment on attachment 381182 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381182&action=review

Was looking this over casually without carefully studying everything. A few comments on the current patch.

> Source/WebCore/html/CanvasBase.h:110
> +    CanvasBase(const IntSize&);

I think it’s better to use IntSize rather than const IntSize&; more efficient the former way because of the small size of this struct.

Also, this constructor should be marked explicit since it’s not intended for the conversion.

> Source/WebCore/html/CanvasBase.h:113
> +    virtual void createImageBuffer() const { };

I think this should be private, not protected. Functions don’t need to be protected to be overridden by derived classes, only if they need to be called by derived classes.

Unnecessary semicolon here.

> Source/WebCore/html/CanvasBase.h:118
> +    static Atomic<size_t> s_activePixelMemory;

We could avoid putting this in the header and avoid needing to add the Atomics.h include by:

- Having this be a global variable private to the source file (internal linkage, using "static" or an anonymous namespace), rather than a static data member.
- Having a protected static member function that simply fetches the value; that is what’s needed in the derived class HTMLCanvasElement.

> Source/WebCore/html/CanvasBase.h:131
> +    mutable IntSize m_size;
> +
> +    mutable Lock m_imageBufferAssignmentLock;
> +
> +    // m_createdImageBuffer means we tried to malloc the buffer. We didn't necessarily get it.
> +    mutable bool m_hasCreatedImageBuffer { false };
> +    mutable bool m_didClearImageBuffer { false };
> +    mutable std::unique_ptr<ImageBuffer> m_imageBuffer;
> +    mutable size_t m_imageBufferCost { 0 };
> +    mutable std::unique_ptr<GraphicsContextStateSaver> m_contextStateSaver;

Typically protected data members are not a good long term design pattern. As many of these as possible should be made private. We can add protected functions for the derived classes to use.

> Source/WebCore/html/CustomPaintCanvas.cpp:41
>  CustomPaintCanvas::CustomPaintCanvas(ScriptExecutionContext& context, unsigned width, unsigned height)

Not new to this patch: Strange that this takes two unsigned values and then converts them to int without checking their values. Seems unsafe and non-obvious that this will change giant value into negative numbers.

> Source/WebCore/html/OffscreenCanvas.cpp:44
>  OffscreenCanvas::OffscreenCanvas(ScriptExecutionContext& context, unsigned width, unsigned height)

Same comment as above.
Comment 26 Chris Lord 2019-10-18 08:11:02 PDT
(In reply to Darin Adler from comment #25)
> Comment on attachment 381182 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=381182&action=review
> 
> Was looking this over casually without carefully studying everything. A few
> comments on the current patch.
> 
> > Source/WebCore/html/CanvasBase.h:110
> > +    CanvasBase(const IntSize&);
> 
> I think it’s better to use IntSize rather than const IntSize&; more
> efficient the former way because of the small size of this struct.
> 
> Also, this constructor should be marked explicit since it’s not intended for
> the conversion.

Sounds good to me

> > Source/WebCore/html/CanvasBase.h:113
> > +    virtual void createImageBuffer() const { };
> 
> I think this should be private, not protected. Functions don’t need to be
> protected to be overridden by derived classes, only if they need to be
> called by derived classes.
> 
> Unnecessary semicolon here.

Fair point, and good spot - will change.

> > Source/WebCore/html/CanvasBase.h:118
> > +    static Atomic<size_t> s_activePixelMemory;
> 
> We could avoid putting this in the header and avoid needing to add the
> Atomics.h include by:
> 
> - Having this be a global variable private to the source file (internal
> linkage, using "static" or an anonymous namespace), rather than a static
> data member.
> - Having a protected static member function that simply fetches the value;
> that is what’s needed in the derived class HTMLCanvasElement.

Done.

> > Source/WebCore/html/CanvasBase.h:131
> > +    mutable IntSize m_size;
> > +
> > +    mutable Lock m_imageBufferAssignmentLock;
> > +
> > +    // m_createdImageBuffer means we tried to malloc the buffer. We didn't necessarily get it.
> > +    mutable bool m_hasCreatedImageBuffer { false };
> > +    mutable bool m_didClearImageBuffer { false };
> > +    mutable std::unique_ptr<ImageBuffer> m_imageBuffer;
> > +    mutable size_t m_imageBufferCost { 0 };
> > +    mutable std::unique_ptr<GraphicsContextStateSaver> m_contextStateSaver;
> 
> Typically protected data members are not a good long term design pattern. As
> many of these as possible should be made private. We can add protected
> functions for the derived classes to use.

Will do.

> > Source/WebCore/html/CustomPaintCanvas.cpp:41
> >  CustomPaintCanvas::CustomPaintCanvas(ScriptExecutionContext& context, unsigned width, unsigned height)
> 
> Not new to this patch: Strange that this takes two unsigned values and then
> converts them to int without checking their values. Seems unsafe and
> non-obvious that this will change giant value into negative numbers.
> 
> > Source/WebCore/html/OffscreenCanvas.cpp:44
> >  OffscreenCanvas::OffscreenCanvas(ScriptExecutionContext& context, unsigned width, unsigned height)
> 
> Same comment as above.

I'm going to leave this, for now at least. Like you say, it was there before, and for what it's worth, canvases initialised with negative values is a situation that's checked in layout tests, so at the least this has test coverage and doesn't result in something really bad happening (I don't think).
Comment 27 Chris Lord 2019-10-28 03:51:50 PDT
Created attachment 382064 [details]
Patch
Comment 28 Chris Lord 2019-10-28 07:22:39 PDT
Comment on attachment 382064 [details]
Patch

Removing cq?, mac-debug-wk1 failures look related to this patch.
Comment 29 Chris Lord 2019-10-30 03:40:17 PDT
Going over this patch and the exception that is crashing Mac builds, I don't see how, given the path it's triggered on, this could happen... In the hope that this was caused by an unrelated patch, I'm going to try submitting again.
Comment 30 Chris Lord 2019-10-30 04:08:06 PDT
Created attachment 382295 [details]
Patch
Comment 31 Ryosuke Niwa 2019-10-30 15:11:45 PDT
Comment on attachment 382295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382295&action=review

> Source/WebCore/html/CanvasBase.cpp:39
> +static Atomic<size_t> s_activePixelMemory { 0 };

Use std::atomics instead?

> Source/WebCore/html/CanvasBase.cpp:100
> +size_t CanvasBase::memoryCost() const

We should rename this to memoryCostConcurrently instead of having a lengthly comment below.
Probably in a separate patch though.

> Source/WebCore/html/CanvasBase.cpp:113
> +    // externalMemoryCost() may be invoked concurrently from a GC thread, and we need to be careful

Ditto.

> Source/WebCore/html/CanvasBase.cpp:205
> +    s_activePixelMemory.exchangeAdd(m_imageBufferCost);

?? How does work? By this code runs, s_activePixelMemory's value may have changed, right?
Why can't we just do this?
s_activePixelMemory += m_imageBufferCost - previousMemoryCost

https://en.cppreference.com/w/cpp/atomic/atomic/operator_arith2

> Source/WebCore/html/CanvasBase.h:113
> +

Nit: Remove this plan line?

> Source/WebCore/html/CanvasBase.h:115
> +

Ditto.

> Source/WebCore/html/CanvasBase.h:124
> +    mutable IntSize m_size;
> +
> +    mutable Lock m_imageBufferAssignmentLock;
> +
> +    mutable std::unique_ptr<GraphicsContextStateSaver> m_contextStateSaver;

What's up with all the blank lines here? Just remove them.
Also can't we add protected setter & getter for these instead of making them protected?
Comment 32 Darin Adler 2019-10-30 15:23:06 PDT
This crash does look like it is related to the patch:

https://ews-build.webkit.org/results/macOS-High-Sierra-Debug-WK1-Tests-EWS/r382295-5143/DumpRenderTree-2196-crash-log.txt

It’s a crash inside garbage collection. When the graphics context is destroyed, it has a non-empty save/restore stack and there’s an assertion that says that’s not OK.

I think the assertion is probably incorrect. It seems that for a canvas you could save, not restore, and then let the canvas get garbage collected. Then the assertion would fire. So the assertion seems to be incorrect.

I think the patch may have to remove this assertion!
Comment 33 Chris Lord 2019-10-31 05:52:12 PDT
(In reply to Ryosuke Niwa from comment #31)
> Comment on attachment 382295 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382295&action=review
> 
> > Source/WebCore/html/CanvasBase.cpp:39
> > +static Atomic<size_t> s_activePixelMemory { 0 };
> 
> Use std::atomics instead?

Sure

> > Source/WebCore/html/CanvasBase.cpp:100
> > +size_t CanvasBase::memoryCost() const
> 
> We should rename this to memoryCostConcurrently instead of having a lengthly
> comment below.
> Probably in a separate patch though.
> 
> > Source/WebCore/html/CanvasBase.cpp:113
> > +    // externalMemoryCost() may be invoked concurrently from a GC thread, and we need to be careful
> 
> Ditto.

Right, I left these unchanged but that sounds like a nice follow-up.

> > Source/WebCore/html/CanvasBase.cpp:205
> > +    s_activePixelMemory.exchangeAdd(m_imageBufferCost);
> 
> ?? How does work? By this code runs, s_activePixelMemory's value may have
> changed, right?
> Why can't we just do this?
> s_activePixelMemory += m_imageBufferCost - previousMemoryCost
> 
> https://en.cppreference.com/w/cpp/atomic/atomic/operator_arith2

Using std::atomic we can do +=, Atomic didn't have such an operator. With regards to the change, it doesn't matter if it changes, as long as the addition and subtraction are atomic and guaranteed to happen, but your suggestion is nicer.

> > Source/WebCore/html/CanvasBase.h:113
> > +
> 
> Nit: Remove this plan line?
> 
> > Source/WebCore/html/CanvasBase.h:115
> > +
> 
> Ditto.
> 
> > Source/WebCore/html/CanvasBase.h:124
> > +    mutable IntSize m_size;
> > +
> > +    mutable Lock m_imageBufferAssignmentLock;
> > +
> > +    mutable std::unique_ptr<GraphicsContextStateSaver> m_contextStateSaver;
> 
> What's up with all the blank lines here? Just remove them.

Sure. I think this may be legacy from older versions of the patch where there were more variables, grouped somewhat logically, but some have been removed/moved(?)

> Also can't we add protected setter & getter for these instead of making them
> protected?

I don't see why not (immediately, at least), will do.
Comment 34 Chris Lord 2019-10-31 05:58:53 PDT
(In reply to Darin Adler from comment #32)
> This crash does look like it is related to the patch:
> 
> https://ews-build.webkit.org/results/macOS-High-Sierra-Debug-WK1-Tests-EWS/
> r382295-5143/DumpRenderTree-2196-crash-log.txt
> 
> It’s a crash inside garbage collection. When the graphics context is
> destroyed, it has a non-empty save/restore stack and there’s an assertion
> that says that’s not OK.
> 
> I think the assertion is probably incorrect. It seems that for a canvas you
> could save, not restore, and then let the canvas get garbage collected. Then
> the assertion would fire. So the assertion seems to be incorrect.
> 
> I think the patch may have to remove this assertion!

I don't think this is correct. CanvasRenderingContext2DBase unwinds the stack in its destructor when asserts are enabled, so I guess this would indicate that that destructor isn't running before the GraphicsContext is being destroyed.

I can't reproduce this locally, however, so I'm not sure what's going on here or what this patch has changed to cause that issue. I'll look into it further. If I can't come up with anything I'll see about using a Mac to debug this.
Comment 35 Chris Lord 2019-10-31 09:49:42 PDT
Created attachment 382471 [details]
Patch
Comment 36 Chris Lord 2019-10-31 09:50:51 PDT
I believe/hope this addresses the issues brought up and the Mac assert, but I'd like to see green EWS first rather than cause cq?/r? churn.
Comment 37 Chris Lord 2019-10-31 10:46:34 PDT
Comment on attachment 382471 [details]
Patch

Ok, looking good :)
Comment 38 WebKit Commit Bot 2019-10-31 13:42:04 PDT
Comment on attachment 382471 [details]
Patch

Clearing flags on attachment: 382471

Committed r251874: <https://trac.webkit.org/changeset/251874>
Comment 39 WebKit Commit Bot 2019-10-31 13:42:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Radar WebKit Bug Importer 2019-10-31 13:43:18 PDT
<rdar://problem/56793658>