Bug 63482 – Allow non-main thread text drawing in ICU ports

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

Some ports may allow non main threads to draw text. In those
ports the ASSERT(isMainThread()) would fire when creating the
LineBreakIteratorPool. We can make the pool a thread specific
object to avoid this problem.

[Joseph Pecoraro]

Created attachment 98789 [details]
[PATCH] Put a Pool in ThreadGlobalData for ICU

Comment on attachment 98789 [details]
[PATCH] Put a Pool in ThreadGlobalData for ICU

View in context: https://bugs.webkit.org/attachment.cgi?id=98789&action=review

> Source/WebCore/platform/ThreadGlobalData.h:79
> +        LineBreakIteratorPool* m_lineBreakIteratorPool;

Can this be an OwnPtr?

[Joseph Pecoraro]

Comment on attachment 98789 [details]
[PATCH] Put a Pool in ThreadGlobalData for ICU

View in context: https://bugs.webkit.org/attachment.cgi?id=98789&action=review

> Source/WebCore/platform/ThreadGlobalData.cpp:89
> +        m_lineBreakIteratorPool = new LineBreakIteratorPool();

Normal style is probably to drop the parens. I'll drop them.

[Darin Adler]

Comment on attachment 98789 [details]
[PATCH] Put a Pool in ThreadGlobalData for ICU

View in context: https://bugs.webkit.org/attachment.cgi?id=98789&action=review

>> Source/WebCore/platform/ThreadGlobalData.cpp:89
>> +        m_lineBreakIteratorPool = new LineBreakIteratorPool();
> 
> Normal style is probably to drop the parens. I'll drop them.

I like it better without the parens, yes.

> Source/WebCore/platform/ThreadGlobalData.cpp:107
> +    if (m_lineBreakIteratorPool) {
> +        delete m_lineBreakIteratorPool;
> +        m_lineBreakIteratorPool = 0;
> +    }

No need for the if statement. The delete operator already checks for 0.

>> Source/WebCore/platform/ThreadGlobalData.h:79
>> +        LineBreakIteratorPool* m_lineBreakIteratorPool;
> 
> Can this be an OwnPtr?

I think using an OwnPtr for this would be good.

[Early Warning System Bot]

Comment on attachment 98789 [details]
[PATCH] Put a Pool in ThreadGlobalData for ICU

Attachment 98789 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8951313

[Joseph Pecoraro]

(In reply to comment #5)
> (From update of attachment 98789 [details])
> Attachment 98789 [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/8951313

Probably failed because I forgot to wrap ThreadGlobalData::lineBreakIteratorPool
in the USE(ICU_UNICODE) block. I'll put another patch up for the bots.

[Joseph Pecoraro]

Created attachment 98806 [details]
[PATCH] For Bots (Uses OwnPtr)

[Joseph Pecoraro]

Created attachment 98808 [details]
[PATCH] For Bots (Uses OwnPtr)

[Darin Adler]

Comment on attachment 98808 [details]
[PATCH] For Bots (Uses OwnPtr)

View in context: https://bugs.webkit.org/attachment.cgi?id=98808&action=review

> Source/WebCore/platform/ThreadGlobalData.cpp:90
> +    return *m_lineBreakIteratorPool.get();

No need for the .get() here.

> Source/WebCore/platform/ThreadGlobalData.h:80
> +        OwnPtr<LineBreakIteratorPool> m_lineBreakIteratorPool;

I think you still need to set this to nullptr in destroy().

> Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:49
>      LineBreakIteratorPool() { }

This can be private now. Or you can remove it if you want to leave it public.

[Joseph Pecoraro]

(In reply to comment #9)
> (From update of attachment 98808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98808&action=review
> 
> > Source/WebCore/platform/ThreadGlobalData.cpp:90
> > +    return *m_lineBreakIteratorPool.get();
> 
> No need for the .get() here.

Okay.

> > Source/WebCore/platform/ThreadGlobalData.h:80
> > +        OwnPtr<LineBreakIteratorPool> m_lineBreakIteratorPool;
> 
> I think you still need to set this to nullptr in destroy().

I thought about this, but decided it was best to keep this as is. If I cleared
the pool object inside of destroy it doesn't mean much if the object will
be recreated on access (because of the lazy loading). Also, the sharedPool
code never expects a null value and OwnPtr guarantees this whether or not
the destroy() happened or not.

Clearing this in destroy would likely mean:

  - no more lazy loading or state so we don't want to create after we are destroyed
  - handle a possible null value in sharedPool()?

Even the current approach has a possible issue where the GlobalData's could
deconstruct a LayerPool that was returned from sharedPool(). Clearing it earlier
in destroy() would make this slightly more possible.

Does either approach sound better to you?

[Joseph Pecoraro]

See my reply above.

[Darin Adler]

(In reply to comment #10)
> > I think you still need to set this to nullptr in destroy().
> 
> I thought about this, but decided it was best to keep this as is. If I cleared
> the pool object inside of destroy it doesn't mean much if the object will
> be recreated on access (because of the lazy loading).

I don’t really understand. Sure, if you try to use the global data after destroying it, there will be a problem. But that’s not specific to your new data member. You can’t use the global data after it has been destroyed.

> Also, the sharedPool code never expects a null value

The sharedPool code will never get a null value, because it calls a function that allocates a new pool if it is zero. This is not an issue.

> OwnPtr guarantees this whether or not the destroy() happened or not.

Sorry, I don’t understand what you are saying OwnPtr guarantees.

> Clearing this in destroy would likely mean:
> 
>   - no more lazy loading or state so we don't want to create after we are destroyed
>   - handle a possible null value in sharedPool()?

It wouldn’t mean either of those things. It’s not legal to call sharedPool after calling destroy.

> Even the current approach has a possible issue where the GlobalData's could
> deconstruct a LayerPool that was returned from sharedPool(). Clearing it earlier
> in destroy() would make this slightly more possible.

I think you are misunderstanding. The destroy function exists so it can be called on worker threads. On normal threads it will be called from inside the destructor. So this timing thing you are talking about, a window of time after the destroy function is called and before the destructor, does not really exist.

> Does either approach sound better to you?

I think your new data member needs to match the rest of the class, and do the actual deallocation in destroy, not just in the destructor. Unless we eliminate the destroy function, which might be good. I don’t understand why worker threads need a different way to destroy the thread data, and it would be better if they did it by actually deallocating.

[Joseph Pecoraro]

> I think you are misunderstanding. The destroy function exists so it can be called on worker threads. On normal threads it will be called from inside the destructor. So this timing thing you are talking about, a window of time after the destroy function is called and before the destructor, does not really exist.
> 
> I think your new data member needs to match the rest of the class, and do the actual deallocation in destroy, not just in the destructor. Unless we eliminate the destroy function, which might be good. I don’t understand why worker threads need a different way to destroy the thread data, and it would be better if they did it by actually deallocating.

You're right. The issues I was concerned with, "after destroy and before deallocating" don't really exist and is not something I should be worrying about. It looks like the bots are happy with the new #if guards. I'll land with the nullptr assignment in destroy (and an updated changelog).

Thanks for the reviews.

[Joseph Pecoraro]

Landed in r89878: <http://trac.webkit.org/changeset/89878>.