Closed Bug 1403935 Opened 7 years ago Closed 7 years ago

Enable OMTP by Default on Windows nightly

Categories

(Core :: Graphics, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

(Keywords: perf, Whiteboard: [gfx-noted])

Attachments

(1 file)

OMTP should be enabled by default!
Let's attach blockers to this bug, if we don't want it on nightly by default until they're fixed.
Priority: -- → P1
Summary: Enable OMTP by Default → Enable OMTP by Default on nightly
Whiteboard: [gfx-noted]
Comment on attachment 8913297 [details]
Bug 1403935: Enable OMTP by default on windows only.

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

\o/
Attachment #8913297 - Flags: review?(dvander) → review+
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/843b90c48664
Enable OMTP by default on windows only. r=dvander
A feature as big as this deserves a blog post with motivation, basic design, basic implementation detail and perf improvements.
(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.
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?
https://hg.mozilla.org/mozilla-central/rev/843b90c48664
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Summary: Enable OMTP by Default on nightly → Enable OMTP by Default on Windows nightly
Depends on: 1404587
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.
Depends on: 1404627
Backout by archaeopteryx@coole-files.de:
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
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
Status: RESOLVED → REOPENED
Flags: needinfo?(bas)
Resolution: FIXED → ---
Target Milestone: mozilla58 → ---
Depends on: 1404742
Depends on: 1404749
No longer depends on: 1404742
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! :)
(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 :-).
Flags: needinfo?(bas)
> (What we believe to be) fixes for all those bugs are up/landed! So working
> on it :-).

I see the action! Excellent.
Pushed by bschouten@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56675d0ea641
Enable OMTP by default on windows only. r=dvander
https://hg.mozilla.org/mozilla-central/rev/56675d0ea641
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
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
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
Depends on: 1405518
Depends on: 1405536
Depends on: 1405589
Can you provide some docs for the OMTP feature? What does it stand for?
Flags: needinfo?(bas)
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
(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?
Flags: needinfo?(bas) → needinfo?(dvander)
No longer depends on: 1415325
We've got a draft blog post - I'll finish it up and send it around.
Flags: needinfo?(dvander)
Regressions: 1552687
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: