585425 - implement threadsafety pieces of harfbuzz for os x

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[timeless]

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]

Attachment: impl for os x

Comment 2

[timeless]

Attachment: 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]]

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]

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]]

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 7

[Behdad Esfahbod]

Jonathan, can you please help me upstream this?

Comment 8

[Behdad Esfahbod]

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?