Last Comment Bug 585425 - implement threadsafety pieces of harfbuzz for os x
: implement threadsafety pieces of harfbuzz for os x
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 15 Branch
: x86 Windows 7
: -- normal (vote)
: mozilla13
Assigned To: timeless
:
Mentors:
: 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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

http://developer.apple.com/mac/library/documentation/Darwin/Reference/ManPages/man3/atomic.3.html
http://developer.apple.com/mac/library/documentation/Darwin/Reference/ManPages/man3/pthread.3.html

http://developer.apple.com/macosx/multithreadedprogramming.html
is the only reference I can find for PTHREAD_MUTEX_INITIALIZER (thanks apple!, I love you too!)
Comment 1 timeless 2010-08-08 05:15:43 PDT
Created attachment 463908 [details] [diff] [review]
impl for os x
Comment 2 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 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 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 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 Benoit Girard (:BenWa) 2011-10-07 22:36:28 PDT
*** Bug 626124 has been marked as a duplicate of this bug. ***
Comment 7 Behdad Esfahbod 2012-01-18 21:00:23 PST
Jonathan, can you please help me upstream this?
Comment 8 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.