Closed Bug 939629 Opened 11 years ago Closed 11 years ago

Build skia in unified mode

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 3 obsolete files)

On my Thinkpad W520, I get near 3x compile time improvements even though my skia port is not 100% complete and I'm leaving several source files in regular SOURCES.

Before:

real    0m14.397s
user    1m30.950s
sys     0m9.689s

After:

real    0m5.466s
user    0m35.990s
sys     0m2.328s
Can you take care of upstreaming this?
Attachment #8333812 - Flags: review?(gwright)
Attached patch Build Skia in unified mode (obsolete) — Splinter Review
Several Skia files remain in regular sources: this is not an "extremist" UNIFIED_SOURCES port, there will be a time to get back to this later and consider moving remaining files to UNIFIED_SOURCES. But this is as much as I could put in UNIFIED_SOURCES without any change to Skia itself aside from the above patch adding missing include guards, and it gives the above-mentioned 3x build time speedup.
Attachment #8333814 - Flags: review?(gwright)
Attachment #8333814 - Flags: review?(ehsan)
The linux build error on TBPL is something that somehow I didn't get locally; it looks like we need to consistently pass -msse2 when building unified sources:

Unified_cpp_gfx_skia16.o
In file included from /builds/slave/try-lx-00000000000000000000000/build/gfx/skia/src/opts/SkBlitRect_opts_SSE2.cpp:12:0,
                 from /builds/slave/try-lx-00000000000000000000000/build/obj-firefox/gfx/skia/Unified_cpp_gfx_skia15.cpp:11:
/tools/gcc-4.7.3-0moz1/lib/gcc/x86_64-unknown-linux-gnu/4.7.3/include/emmintrin.h:32:3: error: #error "SSE2 instruction set not enabled"
/usr/bin/ccache /tools/gcc-4.7.3-0moz1/bin/g++ -m32 -march=pentiumpro -o FrozenImage.o -c -I../../dist/stl_wrappers -I../../dist/system_wrappers -include /builds/slave/try-lx-00000000000000000000000/build/config/gcc_hidden.h -DOS_POSIX=1 -DOS_LINUX=1 -DMOZ_GLUE_IN_PROGRAM -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL  -DSTATIC_EXPORTABLE_JS_API -DNO_NSPR_10_SUPPORT -I/builds/slave/try-lx-00000000000000000000000/build/image/decoders -I/builds/slave/try-lx-00000000000000000000000/build/netwerk/base/src -I/builds/slave/try-lx-00000000000000000000000/build/content/svg/content/src -I/builds/slave/try-lx-00000000000000000000000/build/content/base/src  -I/builds/slave/try-lx-00000000000000000000000/build/layout/svg -I/builds/slave/try-lx-00000000000000000000000/build/ipc/chromium/src -I/builds/slave/try-lx-00000000000000000000000/build/ipc/glue -I/builds/slave/try-lx-00000000000000000000000/build/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/slave/try-lx-00000000000000000000000/build/image/src -I. -I../../dist/include  -I/builds/slave/try-lx-00000000000000000000000/build/obj-firefox/dist/include/nspr -I/builds/slave/try-lx-00000000000000000000000/build/obj-firefox/dist/include/nss      -fPIC   -DMOZILLA_CLIENT -include ../../mozilla-config.h -MD -MP -MF .deps/FrozenImage.o.pp  -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -Wno-error=uninitialized -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe  -DNDEBUG -DTRIMMED -g -Os -freorder-blocks  -fno-omit-frame-pointer  -Werror -I/builds/slave/try-lx-00000000000000000000000/build/obj-firefox/dist/include/cairo   /builds/slave/try-lx-00000000000000000000000/build/image/src/FrozenImage.cpp
Image.o
In file included from /builds/slave/try-lx-00000000000000000000000/build/obj-firefox/gfx/skia/Unified_cpp_gfx_skia15.cpp:11:0:
/builds/slave/try-lx-00000000000000000000000/build/gfx/skia/src/opts/SkBlitRect_opts_SSE2.cpp: In function 'void BlitRect32_OpaqueWide_SSE2(SkPMColor*, int, int, size_t, uint32_t)':
/builds/slave/try-lx-00000000000000000000000/build/gfx/skia/src/opts/SkBlitRect_opts_SSE2.cpp:57:5: error: '__m128i' was not declared in this scope
Oh, I didn't get it locally, because I build 64bit locally, where sse2 is implicit.
Needed one more after reshuffling UNIFIED_SOURCES.
Attachment #8333812 - Attachment is obsolete: true
Attachment #8333812 - Flags: review?(gwright)
Attachment #8333852 - Flags: review?(gwright)
Attached patch Build Skia in unified mode (obsolete) — Splinter Review
Now with the SSE files taken out of UNIFIED_SOURCES.
Attachment #8333814 - Attachment is obsolete: true
Attachment #8333814 - Flags: review?(gwright)
Attachment #8333814 - Flags: review?(ehsan)
Attachment #8333853 - Flags: review?(gwright)
Attachment #8333853 - Flags: review?(ehsan)
Now with SkCondVar built separately on Windows, with a comment explaining why (that's the build failure on Windows on the above try pushes).
Attachment #8333853 - Attachment is obsolete: true
Attachment #8333853 - Flags: review?(gwright)
Attachment #8333853 - Flags: review?(ehsan)
Attachment #8333857 - Flags: review?(gwright)
Attachment #8333857 - Flags: review?(ehsan)
(In reply to Benoit Jacob [:bjacob] from comment #4)
> The linux build error on TBPL is something that somehow I didn't get
> locally; it looks like we need to consistently pass -msse2 when building
> unified sources:

Is that bug 939615?  If not, please file it.
Comment on attachment 8333857 [details] [diff] [review]
Build Skia in unified mode

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

Please add comments describing why you're leaving some stuff in SOURCES, where possible, similar to the ones you already have.
Attachment #8333857 - Flags: review?(ehsan) → review+
Comment on attachment 8333852 [details] [diff] [review]
Add missing include guards to a couple of Skia headers

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

Please add this diff as a file in the patches/ directory.
Attachment #8333852 - Flags: review?(gwright) → review+
Attachment #8333857 - Flags: review?(gwright) → review+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: