Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Rip out harfbuzz from libxul

RESOLVED FIXED in mozilla15

Status

()

Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ehsan, Assigned: jfkthame)

Tracking

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

harfbuzz needs to go outside of libxul, too!
Created attachment 620310 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #620310 - Flags: review?(ted.mielczarek)
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.)
Attachment #620310 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 3

5 years ago
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.
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.
(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.
(Assignee)

Comment 6

5 years ago
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.
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 8

5 years ago
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. :)
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.
(Assignee)

Comment 10

5 years ago
(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.
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.  :/
Assignee: ehsan → nobody
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
No longer blocks: 750661
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.
(Assignee)

Comment 13

5 years ago
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)
Assignee: nobody → jfkthame
Attachment #620310 - Attachment is obsolete: true
Attachment #621154 - Flags: review?(khuey)
Attachment #621154 - Flags: review?(khuey) → review+
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9212a3b98a66
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/9212a3b98a66
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.