+++ This bug was initially created as a clone of Bug #938970 +++

Before: real 0m4.809s user 0m25.868s sys 0m2.396s after: real 0m5.192s user 0m15.688s sys 0m1.412s

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?

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.

(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?

(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

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.

Sure! looking.

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.

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.

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.

benwa, doesn't this latest patch remove logging for the OSX fontlist and fontloader?

Opps!

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 :)

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.

> 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.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/18eadd057bee because neither opt Mac build liked it, https://tbpl.mozilla.org/php/getParsedLog.php?id=31027291&tree=Mozilla-Inbound or https://tbpl.mozilla.org/php/getParsedLog.php?id=31027404&tree=Mozilla-Inbound

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.

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

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.

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.