1403935 - Enable OMTP by Default on Windows nightly

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Bas Schouten (:bas.schouten)]

OMTP should be enabled by default!

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's attach blockers to this bug, if we don't want it on nightly by default until they're fixed.

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1403935: Enable OMTP by default on windows only.

Review commit: https://reviewboard.mozilla.org/r/184670/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184670/

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8913297 [details]
Bug 1403935: Enable OMTP by default on windows only.

https://reviewboard.mozilla.org/r/184670/#review189864

\o/

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/843b90c48664
Enable OMTP by default on windows only. r=dvander

Comment 5

[Mayank Bansal]

A feature as big as this deserves a blog post with motivation, basic design, basic implementation detail and perf improvements.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Mayank Bansal from comment #5)
> A feature as big as this deserves a blog post with motivation, basic design,
> basic implementation detail and perf improvements.

Yup, it's coming.

Comment 7

[Gary [:streetwolf52]]

I'm on the latest inbound with this patch and "layers.omtp.enabled" is now true.  However in about:support it says:

OMTP	
opt-in by default: OMTP is an opt-in feature
available by user: Enabled by pref

On Fx58 it's not an opt-in feature anymore.   Shouldn't the wording be different?

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/843b90c48664

Comment 9

[Daniel Holbert [:dholbert]]

I filed bug 1404532 for comment 7.

Comment 10

[Kacper Michajłow [:kasper93]]

Hey, 

You might want to take care about this crash bug 1395394 reported one month ago. I can reproduce quite easily, now that OMTP is enabled by default.

Thanks.

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/a5f92049b409
Backed out changeset 843b90c48664 for frequently asserting in mochitest gfx/tests/mochitest/test_font_whitelist.html on Windows non-stylo builds. r=backout a=backout

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for frequently asserting in mochitest gfx/tests/mochitest/test_font_whitelist.html on Windows non-stylo builds:

https://hg.mozilla.org/mozilla-central/rev/a5f92049b409adbb465586f6217416aa9b7b3157

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=843b90c486643459c1dd90d0f074c7d8f1c97286&filter-searchStr=047198b05bb06a42bb5266b8a689ba00eb43cf58
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=134213138&repo=mozilla-inbound

20:24:28     INFO -  1974 INFO None1975 INFO TEST-START | gfx/tests/mochitest/test_font_whitelist.html
20:24:28     INFO -  GECKO(6584) | ++DOMWINDOW == 10 (0000024E0890C800) [pid = 7080] [serial = 12] [outer = 0000024E08B62000]
20:24:29     INFO -  GECKO(6584) | MEMORY STAT | vsize 1475MB | vsizeMaxContiguous 131762077MB | residentFast 99MB | heapAllocated 18MB
20:24:29     INFO -  GECKO(6584) | Assertion failure: IsAcquired() && mOwningThread == PR_GetCurrentThread(), at z:/build/build/src/xpcom/threads/BlockingResourceBase.cpp:401
20:24:29     INFO -  GECKO(6584) | #01: mozilla::AssertIsMainThreadOrServoFontMetricsLocked() [layout/style/ServoBindings.cpp:2459]
20:24:29     INFO -  GECKO(6584) | #02: mozilla::WeakPtr<mozilla::gfx::UnscaledFont>::operator mozilla::gfx::UnscaledFont *() [mfbt/WeakPtr.h:276]
20:24:29     INFO -  GECKO(6584) | #03: mozilla::SupportsWeakPtr<mozilla::gfx::UnscaledFont>::~SupportsWeakPtr<mozilla::gfx::UnscaledFont>() [mfbt/WeakPtr.h:207]
20:24:29     INFO -  GECKO(6584) | #04: mozilla::gfx::UnscaledFont::~UnscaledFont() [gfx/2d/ScaledFontBase.cpp:34]
20:24:29     INFO -  GECKO(6584) | #05: mozilla::gfx::UnscaledFontDWrite::`scalar deleting destructor'(unsigned int)
20:24:29     INFO -  GECKO(6584) | #06: mozilla::detail::RefCounted<mozilla::gfx::UnscaledFont,0>::Release() [mfbt/RefCounted.h:140]
20:24:29     INFO -  GECKO(6584) | #07: mozilla::gfx::ScaledFont::~ScaledFont() [gfx/2d/ScaledFontBase.cpp:41]
20:24:29     INFO -  GECKO(6584) | #08: mozilla::gfx::ScaledFontDWrite::`scalar deleting destructor'(unsigned int)
20:24:29     INFO -  GECKO(6584) | #09: mozilla::detail::RefCounted<mozilla::gfx::ScaledFont,0>::Release() [mfbt/RefCounted.h:140]
20:24:29     INFO -  GECKO(6584) | #10: mozilla::gfx::FillGlyphsCommand::~FillGlyphsCommand()
20:24:29     INFO -  GECKO(6584) | #11: mozilla::gfx::FillGlyphsCommand::`scalar deleting destructor'(unsigned int)
20:24:29     INFO -  GECKO(6584) | #12: mozilla::gfx::DrawTargetCaptureImpl::~DrawTargetCaptureImpl() [gfx/2d/DrawTargetCapture.cpp:22]
20:24:29     INFO -  GECKO(6584) | #13: mozilla::gfx::DrawTargetCaptureImpl::`scalar deleting destructor'(unsigned int)
20:24:29     INFO -  GECKO(6584) | #14: mozilla::detail::RefCounted<mozilla::gfx::DrawTarget,0>::Release() [mfbt/RefCounted.h:140]
20:24:29     INFO -  GECKO(6584) | #15: mozilla::detail::RunnableFunction<<lambda_a08043e6d837f3855247633b89253f78> >::`scalar deleting destructor'(unsigned int)
20:24:29     INFO -  GECKO(6584) | #16: mozilla::Runnable::Release() [xpcom/threads/nsThreadUtils.cpp:47]
20:24:29     INFO -  GECKO(6584) | #17: nsThread::ProcessNextEvent(bool,bool *) [xpcom/threads/nsThread.cpp:1047]
20:24:29     INFO -  GECKO(6584) | #18: NS_ProcessNextEvent(nsIThread *,bool) [xpcom/threads/nsThreadUtils.cpp:524]
20:24:29     INFO -  GECKO(6584) | #19: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate *) [ipc/glue/MessagePump.cpp:368]
20:24:29     INFO -  GECKO(6584) | #20: MessageLoop::RunHandler() [ipc/chromium/src/base/message_loop.cc:320]
20:24:29     INFO -  GECKO(6584) | #21: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:300]
20:24:29     INFO -  GECKO(6584) | #22: nsThread::ThreadFunc(void *) [xpcom/threads/nsThread.cpp:429]
20:24:29     INFO -  GECKO(6584) | #23: PR_NativeRunThread [nsprpub/pr/src/threads/combined/pruthr.c:406]
20:24:29     INFO -  GECKO(6584) | #24: pr_root [nsprpub/pr/src/md/windows/w95thred.c:96]
20:24:29     INFO -  GECKO(6584) | #25: ucrtbase.dll + 0x20369
20:24:29     INFO -  GECKO(6584) | #26: KERNEL32.DLL + 0x12774
20:24:29     INFO -  GECKO(6584) | #27: ntdll.dll + 0x70d61

Comment 13

[Nicholas Nethercote [inactive]]

The Nightly builds that had this enabled were about 1.5x crashier than normal, due to bug 1395394, bug 1404587, and bug 1404627. If it hadn't already been backed out I would have suggested doing so. Please fix them before re-enabling OMTP! :)

Comment 14

[Bas Schouten (:bas.schouten)]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> The Nightly builds that had this enabled were about 1.5x crashier than
> normal, due to bug 1395394, bug 1404587, and bug 1404627. If it hadn't
> already been backed out I would have suggested doing so. Please fix them
> before re-enabling OMTP! :)

(What we believe to be) fixes for all those bugs are up/landed! So working on it :-).

Comment 15

[Nicholas Nethercote [inactive]]

> (What we believe to be) fixes for all those bugs are up/landed! So working
> on it :-).

I see the action! Excellent.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56675d0ea641
Enable OMTP by default on windows only. r=dvander

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/56675d0ea641

Comment 18

[Ionuț Goldan [:igoldan]]

When this first landed, these improvements were noticed:

== Change summary for alert #9751 (as of September 29 2017 17:49 UTC) ==

Improvements:

 34%  tsvgr_opacity summary windows7-32 opt e10s     437.97 -> 289.73
 21%  tsvgr_opacity summary windows10-64 opt e10s    290.61 -> 230.71
 10%  tsvgx summary windows7-32 opt e10s             522.98 -> 470.49
  9%  tsvgx summary windows10-64 opt e10s            265.09 -> 240.39

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9751

Comment 19

[Ionuț Goldan [:igoldan]]

The backout canceled these results:

== Change summary for alert #9756 (as of September 30 2017 23:03 UTC) ==

Regressions:

 64%  tsvgr_opacity summary windows7-32 opt e10s     265.71 -> 434.57
 32%  tsvgr_opacity summary windows10-64 opt e10s    219.76 -> 289.70
 14%  tsvgx summary windows7-32 opt e10s             458.25 -> 521.64
 12%  tsvgx summary windows10-64 opt e10s            235.52 -> 262.66

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9756

Comment 20

[Ionuț Goldan [:igoldan]]

The repush brought those improvements back. The PGO results properly confirm the perf boost!

== Change summary for alert #9770 (as of October 02 2017 02:21 UTC) ==

Improvements:

 39%  tsvgr_opacity summary windows7-32 opt e10s     435.66 -> 265.49
 26%  tsvgr_opacity summary windows10-64 opt e10s    290.31 -> 215.46
 19%  tsvgr_opacity summary windows10-64 pgo e10s    256.81 -> 206.84
 12%  tsvgx summary windows7-32 opt e10s             523.38 -> 461.34
 11%  tsvgx summary windows10-64 opt e10s            265.15 -> 235.37

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=9770

Comment 21

[Ionuț Goldan [:igoldan]]

Can you provide some docs for the OMTP feature? What does it stand for?

Comment 22

[Chris Peterson [:cpeterson]]

OMTP = Off Main Thread Painting, an architectural graphics change that should improve browser responsiveness by freeing up the main thread. AFAIK, OMTP is not expected to improve graphics performance.

Here is the OMTP design doc:

https://docs.google.com/document/d/19sZYWo_Fe1x8hA2FJPExijt5rZNA_I4EFzgnEShNqpY/edit

Comment 23

[Bas Schouten (:bas.schouten)]

(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #21)
> Can you provide some docs for the OMTP feature? What does it stand for?

David is supposed to write a blog post.. I can still do it as well.. David?

Comment 24

[David Anderson [:dvander] - inactive, e-mail if emergency]

We've got a draft blog post - I'll finish it up and send it around.

Comment 25

[David Anderson [:dvander] - inactive, e-mail if emergency]

The blog post, for those waiting: https://mozillagfx.wordpress.com/2017/12/05/off-main-thread-painting/