Summary: | Allow non-main thread text drawing in ICU ports | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, joepeck, mitz | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2011-06-27 14:38:17 PDT
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? 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. 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. 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 (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. Created attachment 98806 [details]
[PATCH] For Bots (Uses OwnPtr)
Created attachment 98808 [details]
[PATCH] For Bots (Uses OwnPtr)
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. (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? See my reply above. (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. > 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.
Landed in r89878: <http://trac.webkit.org/changeset/89878>. |