Closed Bug 1422475 Opened 7 years ago Closed 7 years ago

FilterNode creation is slow with Direct2D and OMTP

Categories

(Core :: Graphics, defect, P3)

Product:
Core
Component:
Graphics
Version:
40 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: dvander, Assigned: bas.schouten)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

rasterflood_svg.html
5.03 KB, text/html
Details
Bug 1422475: Create FilterNodes on the paint thread when using Direct2D.
59 bytes, text/x-review-board-request
dvander
: review+
Details
Attached file rasterflood_svg.html — 
FilterNode creation appears to be quite slow, as it has to wait on the D2D global lock taken implicitly on the paint thread. I've attached a benchmark here. I've also got an older profile [1] where the problem can be seen in Content/PaintThread #4.

[1] https://perfht.ml/2jteAU7
Blocks: omtp
Summary: FilterNode creation is slow with Direct2D → FilterNode creation is slow with Direct2D and OMTP
Whiteboard: [gfx-noted]
Comment on attachment 8934827 [details]
Bug 1422475: Create FilterNodes on the paint thread when using Direct2D.

https://reviewboard.mozilla.org/r/205742/#review211282


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/2d/DrawTargetCapture.cpp:427
(Diff revision 1)
> +already_AddRefed<FilterNode>
> +DrawTargetCaptureImpl::CreateFilter(FilterType aType)
> +{
> +  if (mRefDT->GetBackendType() == BackendType::DIRECT2D1_1) {
> +    return MakeRefPtr<FilterNodeCapture>(aType).forget();
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
(In reply to Static Analysis Bot [:clangbot] from comment #2)
> Comment on attachment 8934827 [details]
> Bug 1422475: Create FilterNodes on the paint thread when using Direct2D.
> 
> https://reviewboard.mozilla.org/r/205742/#review211282
> 
> 
> C/C++ static analysis found 1 defect in this patch.
> 
> You can run this analysis locally with: `./mach static-analysis check
> path/to/file.cpp`
> 
> 
> ::: gfx/2d/DrawTargetCapture.cpp:427
> (Diff revision 1)
> > +already_AddRefed<FilterNode>
> > +DrawTargetCaptureImpl::CreateFilter(FilterType aType)
> > +{
> > +  if (mRefDT->GetBackendType() == BackendType::DIRECT2D1_1) {
> > +    return MakeRefPtr<FilterNodeCapture>(aType).forget();
> > +  } else {
> 
> Warning: Do not use 'else' after 'return' [clang-tidy:
> readability-else-after-return]

You are - yet again - wrong, :clangbot.
Assignee: nobody → bas
Status: NEW → ASSIGNED
Comment on attachment 8934827 [details]
Bug 1422475: Create FilterNodes on the paint thread when using Direct2D.

https://reviewboard.mozilla.org/r/205742/#review211526


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/2d/DrawTargetCapture.cpp:427
(Diff revision 2)
> +already_AddRefed<FilterNode>
> +DrawTargetCaptureImpl::CreateFilter(FilterType aType)
> +{
> +  if (mRefDT->GetBackendType() == BackendType::DIRECT2D1_1) {
> +    return MakeRefPtr<FilterNodeCapture>(aType).forget();
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8934827 [details]
Bug 1422475: Create FilterNodes on the paint thread when using Direct2D.

https://reviewboard.mozilla.org/r/205742/#review211528


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/2d/DrawTargetCapture.cpp:427
(Diff revision 3)
> +already_AddRefed<FilterNode>
> +DrawTargetCaptureImpl::CreateFilter(FilterType aType)
> +{
> +  if (mRefDT->GetBackendType() == BackendType::DIRECT2D1_1) {
> +    return MakeRefPtr<FilterNodeCapture>(aType).forget();
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8934827 [details]
Bug 1422475: Create FilterNodes on the paint thread when using Direct2D.

https://reviewboard.mozilla.org/r/205742/#review211570


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/2d/DrawTargetCapture.cpp:427
(Diff revision 4)
> +already_AddRefed<FilterNode>
> +DrawTargetCaptureImpl::CreateFilter(FilterType aType)
> +{
> +  if (mRefDT->GetBackendType() == BackendType::DIRECT2D1_1) {
> +    return MakeRefPtr<FilterNodeCapture>(aType).forget();
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8934827 [details]
Bug 1422475: Create FilterNodes on the paint thread when using Direct2D.

https://reviewboard.mozilla.org/r/205742/#review211582

::: gfx/2d/FilterNodeCapture.h:32
(Diff revision 4)
> +  {
> +  }
> +
> +  virtual FilterBackend GetBackendType() override { return FILTER_BACKEND_CAPTURE; }
> +
> +  FilterNode* Validate(DrawTarget* aDT);

For future-proofing this I think it should return a RefPtr.
Attachment #8934827 - Flags: review?(dvander) → review+
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/441bb5af46ac
Create FilterNodes on the paint thread when using Direct2D. r=dvander
https://hg.mozilla.org/mozilla-central/rev/441bb5af46ac
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: