The default bug view has changed. See this FAQ.

implement threadsafety pieces of harfbuzz for os x

RESOLVED FIXED in mozilla13

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: timeless, Assigned: timeless)

Tracking

15 Branch
mozilla13
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

7 years ago
Created attachment 463908 [details] [diff] [review]
impl for os x
Assignee: nobody → timeless
Status: NEW → ASSIGNED
Attachment #463908 - Flags: review?(jfkthame)
(Assignee)

Comment 2

7 years ago
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
Attachment #463908 - Attachment is obsolete: true
Attachment #463913 - Flags: review?(jfkthame)
Attachment #463908 - Flags: review?(jfkthame)
(Assignee)

Updated

7 years ago
Blocks: 585431
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.
OS: Mac OS X → Windows 7
(Assignee)

Comment 4

7 years ago
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 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.
Attachment #463913 - Flags: review?(jfkthame)

Updated

6 years ago
Duplicate of this bug: 626124

Comment 7

5 years ago
Jonathan, can you please help me upstream this?

Comment 8

5 years ago
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?

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Version: Trunk → 15 Branch
You need to log in before you can comment on or make changes to this bug.