Last Comment Bug 751151 - Rip out harfbuzz from libxul
: Rip out harfbuzz from libxul
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-02 06:51 PDT by :Ehsan Akhgari (busy, don't ask for review please)
Modified: 2012-05-08 11:18 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (3.16 KB, patch)
2012-05-02 07:33 PDT, :Ehsan Akhgari (busy, don't ask for review please)
ted: review-
Details | Diff | Review
patch, v2 - move harfbuzz to gkmedias.dll on windows (3.35 KB, patch)
2012-05-04 13:20 PDT, Jonathan Kew (:jfkthame)
khuey: review+
Details | Diff | Review

Description :Ehsan Akhgari (busy, don't ask for review please) 2012-05-02 06:51:10 PDT
harfbuzz needs to go outside of libxul, too!
Comment 1 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-02 07:33:02 PDT
Created attachment 620310 [details] [diff] [review]
Patch (v1)
Comment 2 Ted Mielczarek [:ted.mielczarek] 2012-05-02 07:39:01 PDT
Comment on attachment 620310 [details] [diff] [review]
Patch (v1)

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

::: gfx/harfbuzz/src/Makefile.in
@@ +35,5 @@
>  MODULE         = harfbuzz
>  LIBRARY_NAME   = mozharfbuzz
> +ifeq ($(OS_ARCH),WINNT)
> +VISIBILITY_FLAGS =
> +endif

I don't understand this hunk. VISIBILITY_FLAGS is never set on Windows, AFAIK. Also, I don't think you have to remove LIBXUL_LIBRARY, do you? (It's probably not terribly relevant here, except that it forces it to be a static library.)
Comment 3 Jonathan Kew (:jfkthame) 2012-05-02 07:47:47 PDT
Is there a risk this'll regress text performance? Note that harfbuzz makes a bunch of (per-character/glyph) callbacks to thebes (for font info - char-to-glyph mappings, glyph metrics, etc) and intl (unicode properties); if we split it out from libxul, we may lose opportunities for the compiler to optimize some of these hot codepaths (e.g. by inlining some of the property accessors).

I'm sure we can do it - just concerned about the possible impact, as text-shaping performance is pretty important.
Comment 4 Mike Hommey [:glandium] 2012-05-02 07:52:32 PDT
Does harfbuzz really directly calls code from thebes? If so that means gkmedia would depend on libxul, and that would make a circular dependency, which is not possible.
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-02 08:04:27 PDT
(In reply to Mike Hommey [:glandium] from comment #4)
> Does harfbuzz really directly calls code from thebes? If so that means
> gkmedia would depend on libxul, and that would make a circular dependency,
> which is not possible.

As far as I can tell, it calls libxul back through callbacks registered at runtime, so that is not an issue.
Comment 6 Jonathan Kew (:jfkthame) 2012-05-02 08:05:04 PDT
It doesn't call *directly*, we give it pointers to callback functions in thebes, and it calls those. So it shouldn't be impossible. And maybe that kind of stuff doesn't get significantly optimized even by PGO - I don't know.
Comment 7 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-02 08:06:15 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> Is there a risk this'll regress text performance? Note that harfbuzz makes a
> bunch of (per-character/glyph) callbacks to thebes (for font info -
> char-to-glyph mappings, glyph metrics, etc) and intl (unicode properties);
> if we split it out from libxul, we may lose opportunities for the compiler
> to optimize some of these hot codepaths (e.g. by inlining some of the
> property accessors).
> 
> I'm sure we can do it - just concerned about the possible impact, as
> text-shaping performance is pretty important.

Hmm, yeah this might affect performance badly.  I could do a bunch of Talos measurements, but that will take... a day at least, which reduces the usefulness of this patch.

Therefore, RESOLVED WONTFIX for now.
Comment 8 Jonathan Kew (:jfkthame) 2012-05-02 08:14:02 PDT
Well, maybe it'd be worth trying the experiment and running the tests sometime, even if we don't do this right away. I was just concerned at the idea of rushing into it without considering the possible impact.... after all, if we're not worried about possible perf regressions, we could just switch off PGO and be happy. :)
Comment 9 Ted Mielczarek [:ted.mielczarek] 2012-05-02 08:15:30 PDT
Every one of these splits is going to have perf impact. It sucks, but it's not as bad as turning PGO off completely.

Also, yes, I'd expect that the PGO optimizer can't really optimize callbacks very effectively, but you'd still be missing out on inlining opportunities from thebes->harfbuzz calls.
Comment 10 Jonathan Kew (:jfkthame) 2012-05-02 08:42:13 PDT
(In reply to Ted Mielczarek [:ted] from comment #9)
> Also, yes, I'd expect that the PGO optimizer can't really optimize callbacks
> very effectively, but you'd still be missing out on inlining opportunities
> from thebes->harfbuzz calls.

Sure, though I'd expect there to be an order of magnitude fewer of those.
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2012-05-02 08:51:02 PDT
Jonathan, please feel free to take this patch and give it a shot by doing perf measurements on Talos (or any other benchmarking tool you think is useful).

I'm currently only focused on reopening the tree, so any work which I would not be able to land today is a distraction for me.  :/
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-02 15:14:20 PDT
I kinda doubt there would be any performance impact here.

One easy way to see would be to patch gfxHarfBuzzShaper::ShapeWord to do its thing, say, 10000 times every time it's called, and time how long it takes to load a page with harfbuzz in and out of libxul.
Comment 13 Jonathan Kew (:jfkthame) 2012-05-04 13:20:45 PDT
Created attachment 621154 [details] [diff] [review]
patch, v2 - move harfbuzz to gkmedias.dll on windows

I had tryserver make PGO builds of a recent mozilla-inbound changeset, and then the same with this patch added, and did a bunch of talos runs to see if there'd be any visible difference in performance.

In each of the 6 tests I checked (Tp5r, Tp5row, Tp4, all on both Win7 and WinXP), averaging the times across 5 runs of each, the builds with harfbuzz in gkmedias.dll came out very slightly *faster* (by amounts varying between 0.25% and 0.8%) than the libxul builds.

These differences are small enough that they may not have any real significance (I'm no statistician!), but IMO this looks like a good enough basis to go ahead with the move - it shouldn't hurt performance (perhaps even a fractional win), and will give the linker a little bit more headroom for libxul.

For reference, the tryserver runs are at:
https://tbpl.mozilla.org/?tree=Try&rev=518e5ff13a02 (harfbuzz in gkmedias)
https://tbpl.mozilla.org/?tree=Try&rev=96e891ba44cc (harfbuzz in libxul)
Comment 15 Ed Morley [:emorley] 2012-05-08 11:18:59 PDT
https://hg.mozilla.org/mozilla-central/rev/9212a3b98a66

Note You need to log in before you can comment on or make changes to this bug.