Closed Bug 939585 Opened 6 years ago Closed 6 years ago

Build gfx/thebes in unified mode

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: BenWa, Assigned: ehsan)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 10 obsolete files)

+++ This bug was initially created as a clone of Bug #938970 +++
Attached patch patch - Saves 10 CPU seconds (obsolete) — Splinter Review
Before:
real	0m4.809s
user	0m25.868s
sys	0m2.396s
after:
real	0m5.192s
user	0m15.688s
sys	0m1.412s
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #8333563 - Flags: review?(ehsan)
Attachment #8333563 - Flags: review?(bjacob)
Comment on attachment 8333563 [details] [diff] [review]
patch - Saves 10 CPU seconds

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

I am confused by the removal of the gfx:: namespace qualifications in gfxUtils.cpp. Why is that needed? Do we have another gfx namespace somewhere?

::: gfx/thebes/gfxUtils.cpp
@@ +727,5 @@
>    }
>    if (aSuggestedFormat == gfxImageFormatRGB24) {
>      /* ScaleYCbCrToRGB32 does not support a picture offset, nor 4:4:4 data.
>         See bugs 639415 and 640073. */
> +    if (aData.mPicX != 0 || aData.mPicY != 0 || yuvtype == YV24)

Why did you need to remove namespace qualification here and elsewhere in this file?
Attachment #8333563 - Flags: review?(bjacob) → review+
Comment on attachment 8333563 [details] [diff] [review]
patch - Saves 10 CPU seconds

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

::: gfx/thebes/gfxFontUtils.cpp
@@ -5,5 @@
>  
> -#ifdef MOZ_LOGGING
> -#define FORCE_PR_LOG /* Allow logging in the release build */
> -#include "prlog.h"
> -#endif

I assume you have a good reason for removing these?

::: gfx/thebes/moz.build
@@ +234,5 @@
>  
>  SOURCES += [
> +    'gfxASurface.cpp',
> +    'gfxPlatform.cpp',
> +]

Please add a comment saying why you're excluding these two files from the unified build.
Attachment #8333563 - Flags: review?(ehsan) → review+
Summary: Switch gfx/thebes to UNIFIED_SOURCES → Build gfx/thebes in unified mode
(In reply to Benoit Jacob [:bjacob] from comment #2)
> I am confused by the removal of the gfx:: namespace qualifications in
> gfxUtils.cpp. Why is that needed? Do we have another gfx namespace somewhere?
> 

http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/transport_dib.h#20

Actually I just noticed that it's really small. Maybe we should just kill it.
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #3)
> Comment on attachment 8333563 [details] [diff] [review]
> patch - Saves 10 CPU seconds
> 
> Review of attachment 8333563 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/gfxFontUtils.cpp
> @@ -5,5 @@
> >  
> > -#ifdef MOZ_LOGGING
> > -#define FORCE_PR_LOG /* Allow logging in the release build */
> > -#include "prlog.h"
> > -#endif
> 
Yes, the problem with FORCE_PR_LOG is that it defines PR_LOGGING and then bad stuff happens:
* Some .h conditionally compiles some method based on PR_LOGGING and from the implementation it's not defined so the implementation doesn't get generated.
* Some unrelated cpp forces PR and causes PR_LOGGING to be defined, then some .cpp has PR_LOGGING defined when it processes the .h so it believes it can and does call the method we never compiled.

... and we get a link error. 

Anyways I want to use this to check if this logging is ever used in production. I haven't seen someone use PR_LOG since 2010. I'm sure people still use it but I suspect people aren't using the gfx PR_LOG.
Comment on attachment 8333563 [details] [diff] [review]
patch - Saves 10 CPU seconds

:jdt do you still use release time logging in gfxFontInfo.cpp?
Attachment #8333563 - Flags: review?(jdaggett)
(In reply to Benoit Girard (:BenWa) from comment #6)
> Comment on attachment 8333563 [details] [diff] [review]
> patch - Saves 10 CPU seconds
> 
> :jdt do you still use release time logging in gfxFontInfo.cpp?

Yes! The logging is there in both the userfonts and fontlist case to be able to debug problems that users report.  Without it we'd need to always resort to feeding them trybuilds and that's much harder than simply enabling the existing logging.  I wish the way logging was implemented wasn't so funky but, meh, not sure it's worth putting in a lot of effort to rework it.
Alright, I'll make sure to preserve this functionality before landing.
Comment on attachment 8333563 [details] [diff] [review]
patch - Saves 10 CPU seconds

r- because of the removed logging
Attachment #8333563 - Flags: review?(jdaggett) → review-
Attached patch patch v2 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=0617f2057904
Attachment #8333563 - Attachment is obsolete: true
Attachment #8334222 - Flags: review+
I can't reproduce the failures on OSX or B2G. bjacob can you look into the linux issues? I'll gladly look at some of your OSX issues. If not we can disallow gfxFont.cpp for now.
Flags: needinfo?(bjacob)
Sure! looking.
Flags: needinfo?(bjacob)
Attached patch Fix the build on X11 (obsolete) — Splinter Review
The X11 build failures were due to gfxDrawable.cpp dragging in X headers to implement a work-around. I read comments in the code and on bugs linked from there and it appears that the work-around is needed on X servers older than version 1.7. Looking at http://distrowatch.com/table.php?distribution=ubuntu , it appears that that affects Ubuntus strictly older than 10.04 LTS. I think that's just barely recent enough that we'd still vaguely care, although a look at crashdata linux kernel versions suggests that very few users reporting crashes to us are on such old linux distros.
Attachment #8334771 - Flags: review?(ehsan)
Comment on attachment 8334771 [details] [diff] [review]
Fix the build on X11

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

::: gfx/thebes/moz.build
@@ +277,5 @@
> +    ]
> +else:
> +    UNIFIED_SOURCES += [
> +        'gfxDrawable.cpp',
> +    ]

Please don't do this kind of thing.  This will cause people building on Linux to get a different set of unified files than other platforms and vice versa.  We should try to make unified builds as similar as possible everywhere.

Please just add this to SOURCES unconditionally.
Attachment #8334771 - Flags: review?(ehsan) → review-
Attached patch unified-sources-thebes-fix-x11 (obsolete) — Splinter Review
Attachment #8334771 - Attachment is obsolete: true
Attachment #8334792 - Flags: review?(ehsan)
Attached patch Fix the build on X11 (obsolete) — Splinter Review
Attachment #8334792 - Attachment is obsolete: true
Attachment #8334792 - Flags: review?(ehsan)
Attachment #8334797 - Flags: review?(ehsan)
Comment on attachment 8334797 [details] [diff] [review]
Fix the build on X11

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

::: gfx/thebes/moz.build
@@ +232,5 @@
>  
>  SOURCES += [
> +    # Includes mac system header conflicting with point/size,
> +    # and includes glxXlibSurface.h which drags in Xrender.h
> +    'gfxASurface.cpp', 

Nit: trailing space.
Attachment #8334797 - Flags: review?(ehsan) → review+
benwa, doesn't this latest patch remove logging for the OSX fontlist and fontloader?
Opps!
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #8334222 - Attachment is obsolete: true
Attachment #8334797 - Attachment is obsolete: true
Attachment #8336240 - Flags: review+
Attached patch patch v4 (obsolete) — Splinter Review
Attachment #8336240 - Attachment is obsolete: true
Attachment #8336274 - Flags: review+
Depends on: 941450
Pushed again to try with depends bug:
https://tbpl.mozilla.org/?tree=Try&rev=bd61ddee653a
Bjacob when you have time see if you can help fix the linux issue. I'll owe you two patches :)
Flags: needinfo?(bjacob)
Note that bug 941450 was backed out for now.
This should take care of your linux/Opt gfx/thebes build error, althought it doesn't reproduce for me locally.

Your linux/debug build errors seem to all be in dom/plugins/base, not in gfx/thebes.
Attachment #8336964 - Flags: review?(bgirard)
Flags: needinfo?(bjacob)
Attachment #8336964 - Flags: review?(bgirard) → review+
Attached patch patch v5 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=15154d69f3a2
Attachment #8336274 - Attachment is obsolete: true
Attachment #8336964 - Attachment is obsolete: true
Attachment #8337416 - Flags: review+
>  static PLDHashOperator
> -AddFeature(const uint32_t& aTag, uint32_t& aValue, void *aUserArg)
> +AddHartBuzzFeature(const uint32_t& aTag, uint32_t& aValue, void *aUserArg)
>  {
>      nsTArray<hb_feature_t>* features = static_cast<nsTArray<hb_feature_t>*> (aUserArg);
>  
>      hb_feature_t feat = { 0, 0, 0, UINT_MAX };
>      feat.tag = aTag;
>      feat.value = aValue;
>      features->AppendElement(feat);
>      return PL_DHASH_NEXT;
> @@ -927,17 +927,17 @@ gfxHarfBuzzShaper::ShapeText(gfxContext 
>  
>      if (MergeFontFeatures(style,
>                            entry->mFeatureSettings,
>                            aShapedText->DisableLigatures(),
>                            entry->FamilyName(),
>                            mergedFeatures))
>      {
>          // enumerate result and insert into hb_feature array
> -        mergedFeatures.Enumerate(AddFeature, &features);
> +        mergedFeatures.Enumerate(AddHartBuzzFeature, &features);
>      }

Why was this change needed?  If you're going to change the name of the
static method, it should probably be "AddOpenTypeFeature", certainly
not "AddHartBuzzFeature" (!?!).

> -    bool common = true;
>      gfxFontFamily *fallbackFamily = nullptr;
>      fontEntry = CommonFontFallback(aCh, aRunScript, aStyle, &fallbackFamily);
>   
>      // if didn't find a font, do system-wide fallback (except for specials)
>      uint32_t cmapCount = 0;
>      if (!fontEntry) {
> -        common = false;
>          fontEntry = GlobalFontFallback(aCh, aRunScript, aStyle, cmapCount,
>                                         &fallbackFamily);
>      }
>      TimeDuration elapsed = TimeStamp::Now() - start;
>  
>  #ifdef PR_LOGGING
>      PRLogModuleInfo *log = gfxPlatform::GetLog(eGfxLog_textrun);
>  
>      if (MOZ_UNLIKELY(log)) {
>          uint32_t unicodeRange = FindCharUnicodeRange(aCh);
>          int32_t script = mozilla::unicode::GetScriptCode(aCh);
>          PR_LOG(log, PR_LOG_WARNING,\
>                 ("(textrun-systemfallback-%s) char: u+%6.6x "
>                   "unicode-range: %d script: %d match: [%s]"
>                  " time: %dus cmaps: %d\n",
> -                (common ? "common" : "global"), aCh,
> +                (fontEntry ? "common" : "global"), aCh,
>                   unicodeRange, script,
>                  (fontEntry ? NS_ConvertUTF16toUTF8(fontEntry->Name()).get() :
>                      "<none>"),
>                  int32_t(elapsed.ToMicroseconds()),
>                  cmapCount));
>      }
>  #endif

Why was this necessary?  Here you're actually altering the logic
incorrectly.  The 'common' flag distinguishes between where a font was
found via the CommonFontFallback method or via GlobalFontFallback
method.  Your change makes this whether a font was found or not.
WTF, my try build passed.

Ohh well, it will give me a chance to address :jtd's comments.
Might be clobber-needed, then. Several other platforms agreed that they couldn't build it, but at least one did build.
Not clobber, https://tbpl.mozilla.org/php/getParsedLog.php?id=31029201&tree=Mozilla-Inbound is a retriggered clobber that failed the same way; it's opt-only, so maybe your try run was debug-only?
According to grep, there are several more .cpp files in gfx/thebes that should probably be omitted from UNIFIED_SOURCES because they use "#define FORCE_PR_LOG".

And with that fixed, I don't think you'll need the (incorrect) change to delete the 'common' variable any more.
Once bug 941854 lands we will know for sure we're not doing this by accident.
Attached patch Patch (v6) (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=10796b89c6df
Assignee: bgirard → ehsan
Attachment #8337416 - Attachment is obsolete: true
Attachment #8338987 - Flags: review?(bgirard)
Attachment #8338987 - Attachment is obsolete: true
Attachment #8338987 - Flags: review?(bgirard)
Comment on attachment 8339002 [details] [diff] [review]
Build gfx/thebes in unified mode; r=BenWa

https://tbpl.mozilla.org/?tree=Try&rev=cc47c4c652ea
Attachment #8339002 - Flags: review?(bgirard)
Comment on attachment 8339002 [details] [diff] [review]
Build gfx/thebes in unified mode; r=BenWa

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

::: gfx/thebes/moz.build
@@ +202,5 @@
>          'gfxWindowsNativeDrawing.h',
>          'gfxWindowsPlatform.h',
>          'gfxWindowsSurface.h',
>      ]
> +    # gfxGDIFontList.cpp and gfxGDIShaper.cpp force NSPR logging, so they cannot be built in unified mode.

What about the other files? Have you simply not tried? It's confusing to leave this comment for a few files in this section but not the rest.
Attachment #8339002 - Flags: review?(bgirard) → review+
I added these comments for files that #defined FORCE_PR_LOG.  I haven't yet tried to unify those files, since I haven't built on Windows myself.
https://hg.mozilla.org/mozilla-central/rev/32aa215becb4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.