Last Comment Bug 585425 - implement threadsafety pieces of harfbuzz for os x
: implement threadsafety pieces of harfbuzz for os x
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 15 Branch
: x86 Windows 7
-- normal (vote)
: mozilla13
Assigned To: timeless
: Milan Sreckovic [:milan]
: 626124 (view as bug list)
Depends on:
Blocks: 585431
  Show dependency treegraph
Reported: 2010-08-08 05:13 PDT by timeless
Modified: 2012-05-27 19:50 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

impl for os x (2.73 KB, patch)
2010-08-08 05:15 PDT, timeless
no flags Details | Diff | Splinter Review
impl for os x (2.72 KB, patch)
2010-08-08 05:43 PDT, timeless
no flags Details | Diff | Splinter Review

Description User image timeless 2010-08-08 05:13:58 PDT
HB_REFERENCE_COUNT_SET_VALUE/hb_atomic_int_set can't be implemented using Darwin's atomic ops afaict. One could just try to use foo = bar, but that doesn't seem like a good idea. Given that it isn't used, I think removing it makes sense.

The volatile bit might not actually be necessary.

This code compiles, but I haven't run with it. I believe it's correct.
is the only reference I can find for PTHREAD_MUTEX_INITIALIZER (thanks apple!, I love you too!)
Comment 1 User image timeless 2010-08-08 05:15:43 PDT
Created attachment 463908 [details] [diff] [review]
impl for os x
Comment 2 User image timeless 2010-08-08 05:43:45 PDT
Created attachment 463913 [details] [diff] [review]
impl for os x

I've decided to only use one operation for both Set and Get, it's easier to think about it this way
Comment 3 User image Jonathan Kew (:jfkthame) 2010-08-14 06:19:13 PDT
I don't think we need to worry about this for now (as we don't do multi-threaded text layout/rendering). It's something we may want to investigate later.

Also, rather than take patches (that we don't currently need) to the harfbuzz source in the mozilla repository, any work on this should happen upstream as part of the harfbuzz-ng project. We should only patch harfbuzz locally as a temporary measure if we really need to fix something that hasn't been done upstream yet, but thread-safety is not currently an issue for us.

The same applies to bug 585431.
Comment 4 User image timeless 2010-08-14 16:56:23 PDT
jtfhame: so please take the patches upstream. some of us don't have infinite time to deal with dozens of upstream repositories. if you're involved upstream, then part of being r? involves you taking patches which belong upstream to upstream.
Comment 5 User image Jonathan Kew (:jfkthame) 2011-08-20 03:44:48 PDT
Comment on attachment 463913 [details] [diff] [review]
impl for os x

Clearing r? on this for now. We can look at it again if/when we actually need thread-safety here, and once the harfbuzz codebase is more stable.
Comment 6 User image Benoit Girard (:BenWa) 2011-10-07 22:36:28 PDT
*** Bug 626124 has been marked as a duplicate of this bug. ***
Comment 7 User image Behdad Esfahbod 2012-01-18 21:00:23 PST
Jonathan, can you please help me upstream this?
Comment 8 User image Behdad Esfahbod 2012-02-24 13:21:36 PST
I implemented these upstream now.  There was more work to be done as get() had to be compatible with const memory, which the patch here wasn't.  But it's all working now.

The code still prefer's glib implementations over native.  I know it's not relevant to Mozilla, but thought I ask anyway.  Jonathan, what's your thoughts on the order?  Shall we prefer native instead?

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