All users were logged out of Bugzilla on October 13th, 2018

Build display-list dumping without MOZ_DUMP_PAINTING

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
4 years ago
5 months ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 8539515 [details] [diff] [review]
patch

The display-list dump is now a central feature for graphics debugging and will now be used by the profiler to visualize painting. We should make this available by default.
Attachment #8539515 - Flags: review?(mstange)
Comment on attachment 8539515 [details] [diff] [review]
patch

Review of attachment 8539515 [details] [diff] [review]:
-----------------------------------------------------------------

What's the reason for the gfxUtils.cpp change? I think checking an env variable just once on startup should be fine, performance-wise.
Attachment #8539515 - Flags: review?(mstange) → review+
(Assignee)

Comment 2

4 years ago
I didn’t want to check an env variable for something that would never get used
(Assignee)

Comment 3

4 years ago
Created attachment 8539520 [details] [diff] [review]
patch v2

Forgot to remove the #ifdef for nsDisplayGeneric::mName.
Assignee: nobody → bgirard
Attachment #8539515 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8539520 - Flags: review+
(Assignee)

Comment 4

4 years ago
Created attachment 8539543 [details] [diff] [review]
patch v3

does what I said in v2 but properly this time.
Attachment #8539520 - Attachment is obsolete: true
Attachment #8539543 - Flags: review+
(Assignee)

Comment 5

4 years ago
Created attachment 8539584 [details] [diff] [review]
patch v4

minor tweaks
Attachment #8539543 - Attachment is obsolete: true
Attachment #8539584 - Flags: review+
(Assignee)

Comment 6

4 years ago
Created attachment 8539586 [details] [diff] [review]
patch v5
Attachment #8539584 - Attachment is obsolete: true
Attachment #8539586 - Flags: review+
(Assignee)

Comment 8

4 years ago
Created attachment 8539658 [details]
Valgrind malloc / delete mismatch

UniqueSelector calls operator new whichs gets translated to valgrind's memcheck malloc, but the UniquePtr reset() calls memchecks operator delete.

Could this be a bug in either memcheck or our memory shim? Outwardly we're calling both opeator new / operator delete.
Flags: needinfo?(jseward)
(In reply to Benoit Girard (:BenWa) from comment #8)
> Could this be a bug in either memcheck or our memory shim? Outwardly we're
> calling both opeator new / operator delete.

The Gecko code is fine; you can safely ignore this.  FWIW it happens because
|operator new| is inlined into its caller |detail::UniqueSelector|

==47856==    by 0x7625757: moz_xmalloc (mozalloc.cpp:52)
==47856==    by 0x9807C96: operator new (mozalloc.h:210)
==47856==    by 0x9807C96: mozilla::detail::UniqueSelector<std::basic_st[..]

whereas |operator delete| isn't

==47856==    at 0x4C2827E: operator delete(void*) (in /usr/lib64/valgrin[..]
==47856==    by 0x54F5D6C: std::basic_stringstream<char, std::char_trait[..]

so Memcheck can intercept calls to |delete| but there's no call to |new|
to intercept; instead it intercepts only the |malloc| called by |new|.

For now you can run with --show-mismatched-frees=no.  Alternatively try
building Fx with -O2, since I suspect that increases the inlining aggressiveness
and causes both |new| and |delete| to get inlined, removing the asymmetry.
Flags: needinfo?(jseward)
(Assignee)

Comment 10

4 years ago
Created attachment 8543043 [details] [diff] [review]
patch v6
Attachment #8539586 - Attachment is obsolete: true
Attachment #8543043 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/65adc56e14a9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1117817
No longer depends on: 1117817

Updated

5 months ago
Blocks: 1462177
You need to log in before you can comment on or make changes to this bug.