Make platform/font thread friendly |
||||||||
Issue descriptionAs an effort to bring font to workers, we need to make platform/font thread friendly. Tracking.
,
Jun 12 2017
TODO: Make FontGlobalContext::GetFontCache() work properly on global clean ups.
,
Jun 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f99a04b7cba08ec751f2e646a8180bbf7f870af commit 3f99a04b7cba08ec751f2e646a8180bbf7f870af Author: Fernando Serboncini <fserb@chromium.org> Date: Mon Jun 12 21:08:58 2017 Make FontBuilder receive a Document pointer, not reference This allows FontBuilder to work without a Document, as it was designed to. Bug: 730692 Change-Id: I43d3a67a2565ca7b4c199685f5c2b0bdcc42b79d Reviewed-on: https://chromium-review.googlesource.com/531666 Reviewed-by: Justin Novosad <junov@chromium.org> Reviewed-by: Rune Lillesveen <rune@opera.com> Commit-Queue: Fernando Serboncini <fserb@chromium.org> Cr-Commit-Position: refs/heads/master@{#478762} [modify] https://crrev.com/3f99a04b7cba08ec751f2e646a8180bbf7f870af/third_party/WebKit/Source/core/css/resolver/FontBuilder.cpp [modify] https://crrev.com/3f99a04b7cba08ec751f2e646a8180bbf7f870af/third_party/WebKit/Source/core/css/resolver/FontBuilder.h [modify] https://crrev.com/3f99a04b7cba08ec751f2e646a8180bbf7f870af/third_party/WebKit/Source/core/css/resolver/FontBuilderTest.cpp [modify] https://crrev.com/3f99a04b7cba08ec751f2e646a8180bbf7f870af/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp [modify] https://crrev.com/3f99a04b7cba08ec751f2e646a8180bbf7f870af/third_party/WebKit/Source/core/css/resolver/StyleResolverState.cpp [modify] https://crrev.com/3f99a04b7cba08ec751f2e646a8180bbf7f870af/third_party/WebKit/Source/core/dom/Document.cpp
,
Jun 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/96e2b7d0f820bfa4fb3f2b2aeb58debfcb5e5bf4 commit 96e2b7d0f820bfa4fb3f2b2aeb58debfcb5e5bf4 Author: Fernando Serboncini <fserb@chromium.org> Date: Mon Jun 12 22:57:17 2017 Adds FontGlobalContext to store static objects of platform/font Currently only being used on FontCache, to be used on other Font-related classes. Bug: 730692 Change-Id: I58f66d6cf59dca83be5133fc273136ce408e5b2e Reviewed-on: https://chromium-review.googlesource.com/531753 Commit-Queue: Fernando Serboncini <fserb@chromium.org> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Cr-Commit-Position: refs/heads/master@{#478802} [modify] https://crrev.com/96e2b7d0f820bfa4fb3f2b2aeb58debfcb5e5bf4/third_party/WebKit/Source/platform/BUILD.gn [modify] https://crrev.com/96e2b7d0f820bfa4fb3f2b2aeb58debfcb5e5bf4/third_party/WebKit/Source/platform/fonts/FontCache.cpp [modify] https://crrev.com/96e2b7d0f820bfa4fb3f2b2aeb58debfcb5e5bf4/third_party/WebKit/Source/platform/fonts/FontCache.h [add] https://crrev.com/96e2b7d0f820bfa4fb3f2b2aeb58debfcb5e5bf4/third_party/WebKit/Source/platform/fonts/FontGlobalContext.cpp [add] https://crrev.com/96e2b7d0f820bfa4fb3f2b2aeb58debfcb5e5bf4/third_party/WebKit/Source/platform/fonts/FontGlobalContext.h
,
Jun 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d8733fe8c6e16b9bd8605bb15b0003df39d11e4 commit 4d8733fe8c6e16b9bd8605bb15b0003df39d11e4 Author: Fernando Serboncini <fserb@chromium.org> Date: Tue Jun 20 21:55:34 2017 Creates FontStyleResolver for off-main thread font resolution This still shares code with StyleBuilderConverter and FontBuilder. For that, we create a StyleBuilderConverterBase that is independent of ComputedStyle. Bug: 730692 Change-Id: I7792ef340a5f95782f57a94e093c3c7d73ad11b1 Reviewed-on: https://chromium-review.googlesource.com/531655 Commit-Queue: Fernando Serboncini <fserb@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Cr-Commit-Position: refs/heads/master@{#480971} [modify] https://crrev.com/4d8733fe8c6e16b9bd8605bb15b0003df39d11e4/third_party/WebKit/Source/core/css/BUILD.gn [modify] https://crrev.com/4d8733fe8c6e16b9bd8605bb15b0003df39d11e4/third_party/WebKit/Source/core/css/resolver/FontBuilder.cpp [modify] https://crrev.com/4d8733fe8c6e16b9bd8605bb15b0003df39d11e4/third_party/WebKit/Source/core/css/resolver/FontBuilder.h [add] https://crrev.com/4d8733fe8c6e16b9bd8605bb15b0003df39d11e4/third_party/WebKit/Source/core/css/resolver/FontStyleResolver.cpp [add] https://crrev.com/4d8733fe8c6e16b9bd8605bb15b0003df39d11e4/third_party/WebKit/Source/core/css/resolver/FontStyleResolver.h [modify] https://crrev.com/4d8733fe8c6e16b9bd8605bb15b0003df39d11e4/third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.cpp [modify] https://crrev.com/4d8733fe8c6e16b9bd8605bb15b0003df39d11e4/third_party/WebKit/Source/core/css/resolver/StyleBuilderConverter.h [modify] https://crrev.com/4d8733fe8c6e16b9bd8605bb15b0003df39d11e4/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp
,
Jun 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5e2330c58ace31c217a12891436089e7084169e9 commit 5e2330c58ace31c217a12891436089e7084169e9 Author: Fernando Serboncini <fserb@chromium.org> Date: Wed Jun 21 22:36:14 2017 FontObjectThreadedTests to tests font creation/usage on threaded Those tests exercise: - FontStyleResolver CSS resolution to FontDescription - CreateTestFont (FontSelector + test Font loading) Bug: 730692 Change-Id: I3c68144eefeadda5568ab4977809dd36b6640d63 Reviewed-on: https://chromium-review.googlesource.com/541497 Commit-Queue: Fernando Serboncini <fserb@chromium.org> Reviewed-by: Justin Novosad <junov@chromium.org> Cr-Commit-Position: refs/heads/master@{#481335} [modify] https://crrev.com/5e2330c58ace31c217a12891436089e7084169e9/third_party/WebKit/Source/core/BUILD.gn [add] https://crrev.com/5e2330c58ace31c217a12891436089e7084169e9/third_party/WebKit/Source/core/css/threaded/FontObjectThreadedTest.cpp
,
Jun 22 2017
,
Jun 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/958810932f8b93831d919b8915c3c916db559636 commit 958810932f8b93831d919b8915c3c916db559636 Author: Fernando Serboncini <fserb@chromium.org> Date: Tue Jun 27 21:09:43 2017 Move FontCache static variables as members Since FontCache is already loaded as Singleton, this is mostly an obvious change. Also removes a DCHECK that is not needed anymore on FontFallbackList Bug: 730692 Change-Id: I6214c51c262b7bf55f042fc985d67ef932b04427 Reviewed-on: https://chromium-review.googlesource.com/544690 Reviewed-by: Dominik Röttsches <drott@chromium.org> Reviewed-by: Emil A Eklund <eae@chromium.org> Commit-Queue: Fernando Serboncini <fserb@chromium.org> Cr-Commit-Position: refs/heads/master@{#482744} [modify] https://crrev.com/958810932f8b93831d919b8915c3c916db559636/third_party/WebKit/Source/platform/fonts/FontCache.cpp [modify] https://crrev.com/958810932f8b93831d919b8915c3c916db559636/third_party/WebKit/Source/platform/fonts/FontCache.h [modify] https://crrev.com/958810932f8b93831d919b8915c3c916db559636/third_party/WebKit/Source/platform/fonts/FontDataCache.h [modify] https://crrev.com/958810932f8b93831d919b8915c3c916db559636/third_party/WebKit/Source/platform/fonts/FontFallbackList.h
,
Jun 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3e7a7ad59ed57298ca1797b0d56a1f66f41fe943 commit 3e7a7ad59ed57298ca1797b0d56a1f66f41fe943 Author: Fernando Serboncini <fserb@chromium.org> Date: Wed Jun 28 16:08:53 2017 Enables multi-threading compilation of HarfBuzz Using intel atomic primitives Bug: 730692 Change-Id: I23672efa391a7382e585882dd76fe95fc7d3ad6f Reviewed-on: https://chromium-review.googlesource.com/544526 Reviewed-by: Dominik Röttsches <drott@chromium.org> Commit-Queue: Fernando Serboncini <fserb@chromium.org> Cr-Commit-Position: refs/heads/master@{#483011} [modify] https://crrev.com/3e7a7ad59ed57298ca1797b0d56a1f66f41fe943/third_party/harfbuzz-ng/BUILD.gn
,
Jul 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ff48fa86d77941267bb8ab338f45282a1fabdd64 commit ff48fa86d77941267bb8ab338f45282a1fabdd64 Author: Fernando Serboncini <fserb@chromium.org> Date: Tue Jul 04 16:03:25 2017 Makes fonts/shaping thread safe Makes HarfBuzzFontCache and HarfBuzzFontFuncs parts of FontGlobalContext Bug: 730692 Change-Id: I4b547b93cfd25d8987cc0a800b84172e27f3f790 Reviewed-on: https://chromium-review.googlesource.com/544783 Reviewed-by: Dominik Röttsches <drott@chromium.org> Reviewed-by: Koji Ishii <kojii@chromium.org> Commit-Queue: Fernando Serboncini <fserb@chromium.org> Cr-Commit-Position: refs/heads/master@{#484122} [modify] https://crrev.com/ff48fa86d77941267bb8ab338f45282a1fabdd64/third_party/WebKit/Source/platform/fonts/FontGlobalContext.cpp [modify] https://crrev.com/ff48fa86d77941267bb8ab338f45282a1fabdd64/third_party/WebKit/Source/platform/fonts/FontGlobalContext.h [modify] https://crrev.com/ff48fa86d77941267bb8ab338f45282a1fabdd64/third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFace.cpp [add] https://crrev.com/ff48fa86d77941267bb8ab338f45282a1fabdd64/third_party/WebKit/Source/platform/fonts/shaping/HarfBuzzFontCache.h
,
Jul 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/146a8a2f14c63516cd1ba0b48bdfa32e19d07f09 commit 146a8a2f14c63516cd1ba0b48bdfa32e19d07f09 Author: Fernando Serboncini <fserb@chromium.org> Date: Tue Jul 04 18:12:15 2017 Make LocaleToStringMapping thread safe We remove the hash_map construction/lookup that is not thread safe with a linear constexpr search. Given that the mapping has 10-200 entries, and that the code is not hot, it shouldn't be a perf issue. Bug: 730692 Change-Id: I07d7e8edca4ad1f935e7e016041ca7b731e276e5 Reviewed-on: https://chromium-review.googlesource.com/544525 Reviewed-by: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Fernando Serboncini <fserb@chromium.org> Cr-Commit-Position: refs/heads/master@{#484139} [modify] https://crrev.com/146a8a2f14c63516cd1ba0b48bdfa32e19d07f09/third_party/WebKit/Source/platform/text/LocaleToScriptMapping.cpp
,
Jul 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/edad8c20cdaf925fe31a964a4c6cfc6e0d04fbed commit edad8c20cdaf925fe31a964a4c6cfc6e0d04fbed Author: Fernando Serboncini <fserb@chromium.org> Date: Thu Jul 06 17:01:08 2017 Make Language static thread-safe Bug: 730692 Change-Id: Ia2b696ece715903764ec9a477ac782b043ae860c Reviewed-on: https://chromium-review.googlesource.com/544523 Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Fernando Serboncini <fserb@chromium.org> Cr-Commit-Position: refs/heads/master@{#484640} [modify] https://crrev.com/edad8c20cdaf925fe31a964a4c6cfc6e0d04fbed/content/public/test/render_view_test.cc [modify] https://crrev.com/edad8c20cdaf925fe31a964a4c6cfc6e0d04fbed/content/renderer/render_thread_impl_browsertest.cc [modify] https://crrev.com/edad8c20cdaf925fe31a964a4c6cfc6e0d04fbed/third_party/WebKit/Source/core/css/threaded/FontObjectThreadedTest.cpp [modify] https://crrev.com/edad8c20cdaf925fe31a964a4c6cfc6e0d04fbed/third_party/WebKit/Source/core/testing/Internals.cpp [modify] https://crrev.com/edad8c20cdaf925fe31a964a4c6cfc6e0d04fbed/third_party/WebKit/Source/platform/Language.cpp [modify] https://crrev.com/edad8c20cdaf925fe31a964a4c6cfc6e0d04fbed/third_party/WebKit/Source/platform/Language.h [modify] https://crrev.com/edad8c20cdaf925fe31a964a4c6cfc6e0d04fbed/third_party/WebKit/Source/platform/exported/Platform.cpp [modify] https://crrev.com/edad8c20cdaf925fe31a964a4c6cfc6e0d04fbed/third_party/WebKit/Source/platform/fonts/FontGlobalContext.h [modify] https://crrev.com/edad8c20cdaf925fe31a964a4c6cfc6e0d04fbed/third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
,
Jul 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2f98242f3b670cafd1f6712143ed9306364376fc commit 2f98242f3b670cafd1f6712143ed9306364376fc Author: Fernando Serboncini <fserb@chromium.org> Date: Fri Jul 07 14:43:58 2017 Make LayoutLocale thread safe moves Locale related static to FontGlobalContext Bug: 730692 Change-Id: I0cf60421fbe07c13eacd1b8742920c052ccf255e Reviewed-on: https://chromium-review.googlesource.com/544909 Reviewed-by: Koji Ishii <kojii@chromium.org> Reviewed-by: Dominik Röttsches <drott@chromium.org> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Fernando Serboncini <fserb@chromium.org> Cr-Commit-Position: refs/heads/master@{#484919} [modify] https://crrev.com/2f98242f3b670cafd1f6712143ed9306364376fc/third_party/WebKit/Source/platform/LayoutLocale.cpp [modify] https://crrev.com/2f98242f3b670cafd1f6712143ed9306364376fc/third_party/WebKit/Source/platform/LayoutLocale.h [modify] https://crrev.com/2f98242f3b670cafd1f6712143ed9306364376fc/third_party/WebKit/Source/platform/LayoutLocaleTest.cpp [modify] https://crrev.com/2f98242f3b670cafd1f6712143ed9306364376fc/third_party/WebKit/Source/platform/fonts/AcceptLanguagesResolver.cpp [modify] https://crrev.com/2f98242f3b670cafd1f6712143ed9306364376fc/third_party/WebKit/Source/platform/fonts/FontCache.cpp [modify] https://crrev.com/2f98242f3b670cafd1f6712143ed9306364376fc/third_party/WebKit/Source/platform/fonts/FontGlobalContext.cpp [modify] https://crrev.com/2f98242f3b670cafd1f6712143ed9306364376fc/third_party/WebKit/Source/platform/fonts/FontGlobalContext.h [modify] https://crrev.com/2f98242f3b670cafd1f6712143ed9306364376fc/third_party/WebKit/Source/platform/text/HyphenationTest.cpp
,
Jul 9 2017
,
Jul 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e6d71be27379674d414e31e57faf951f561acf46 commit e6d71be27379674d414e31e57faf951f561acf46 Author: Fernando Serboncini <fserb@chromium.org> Date: Wed Jul 12 18:53:53 2017 Make alternate font family names thread safe We now make those AtomicString static. Bug: 730692 Change-Id: I37e4403bea7358bc852ff3b21a5afa5a25633428 Reviewed-on: https://chromium-review.googlesource.com/565901 Commit-Queue: Fernando Serboncini <fserb@chromium.org> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Cr-Commit-Position: refs/heads/master@{#486039} [modify] https://crrev.com/e6d71be27379674d414e31e57faf951f561acf46/third_party/WebKit/Source/build/scripts/make_names.py [modify] https://crrev.com/e6d71be27379674d414e31e57faf951f561acf46/third_party/WebKit/Source/platform/fonts/AlternateFontFamily.h [modify] https://crrev.com/e6d71be27379674d414e31e57faf951f561acf46/third_party/WebKit/Source/platform/fonts/FontFamilyNames.json5 [modify] https://crrev.com/e6d71be27379674d414e31e57faf951f561acf46/third_party/WebKit/Source/platform/fonts/skia/FontCacheSkia.cpp [modify] https://crrev.com/e6d71be27379674d414e31e57faf951f561acf46/third_party/WebKit/Source/platform/testing/TestingPlatformSupport.cpp
,
Jul 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d8e24d3bda7e4fc41a8a52e6094a0396e0cb702d commit d8e24d3bda7e4fc41a8a52e6094a0396e0cb702d Author: Fernando Serboncini <fserb@chromium.org> Date: Fri Jul 14 19:19:24 2017 Remove more static font data - Current Accept Languages (moved to FontGlobalContext) - FontVerticalDataCacheInstance (moved to FGC) - Linux/Android system_font_family (static on FontCache) - Font strings on SimpleFontData, FontCacheAndroid, FontCacheMac Bug: 730692 Change-Id: Ia831c0eee10396d9318bd509573b40bcc3f76775 Reviewed-on: https://chromium-review.googlesource.com/568782 Commit-Queue: Fernando Serboncini <fserb@chromium.org> Reviewed-by: Justin Novosad <junov@chromium.org> Cr-Commit-Position: refs/heads/master@{#486836} [modify] https://crrev.com/d8e24d3bda7e4fc41a8a52e6094a0396e0cb702d/third_party/WebKit/Source/platform/fonts/AcceptLanguagesResolver.cpp [modify] https://crrev.com/d8e24d3bda7e4fc41a8a52e6094a0396e0cb702d/third_party/WebKit/Source/platform/fonts/FontCache.cpp [modify] https://crrev.com/d8e24d3bda7e4fc41a8a52e6094a0396e0cb702d/third_party/WebKit/Source/platform/fonts/FontFamilyNames.json5 [modify] https://crrev.com/d8e24d3bda7e4fc41a8a52e6094a0396e0cb702d/third_party/WebKit/Source/platform/fonts/FontGlobalContext.h [modify] https://crrev.com/d8e24d3bda7e4fc41a8a52e6094a0396e0cb702d/third_party/WebKit/Source/platform/fonts/SimpleFontData.cpp [modify] https://crrev.com/d8e24d3bda7e4fc41a8a52e6094a0396e0cb702d/third_party/WebKit/Source/platform/fonts/android/FontCacheAndroid.cpp [modify] https://crrev.com/d8e24d3bda7e4fc41a8a52e6094a0396e0cb702d/third_party/WebKit/Source/platform/fonts/linux/FontCacheLinux.cpp [modify] https://crrev.com/d8e24d3bda7e4fc41a8a52e6094a0396e0cb702d/third_party/WebKit/Source/platform/fonts/mac/FontCacheMac.mm
,
Jul 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dab57a7968f8af581e5a267072e0990fa5d9828c commit dab57a7968f8af581e5a267072e0990fa5d9828c Author: Fernando Serboncini <fserb@chromium.org> Date: Tue Jul 18 20:57:23 2017 Make BidiContext thread safe Bug: 730692 Change-Id: Ia5133f2f31e0411e374a051e6f907df570c252c4 Reviewed-on: https://chromium-review.googlesource.com/576373 Reviewed-by: Dominik Röttsches <drott@chromium.org> Commit-Queue: Fernando Serboncini <fserb@chromium.org> Cr-Commit-Position: refs/heads/master@{#487598} [modify] https://crrev.com/dab57a7968f8af581e5a267072e0990fa5d9828c/third_party/WebKit/Source/platform/text/BidiContext.cpp [modify] https://crrev.com/dab57a7968f8af581e5a267072e0990fa5d9828c/third_party/WebKit/Source/platform/text/BidiContext.h
,
Jul 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/de5a958df5a58664d436ad931d23e6cd25c71f9e commit de5a958df5a58664d436ad931d23e6cd25c71f9e Author: Fernando Serboncini <fserb@chromium.org> Date: Wed Jul 19 20:24:57 2017 Adds TextIntercepts multi threaded tests Also, disabes HarfBuzz's atomic refptr TSan warning Bug: 730692 Change-Id: I69c7fd5ab20896b6abd864506ae394732f8aa6e1 Reviewed-on: https://chromium-review.googlesource.com/544625 Commit-Queue: Fernando Serboncini <fserb@chromium.org> Reviewed-by: Justin Novosad <junov@chromium.org> Cr-Commit-Position: refs/heads/master@{#487953} [modify] https://crrev.com/de5a958df5a58664d436ad931d23e6cd25c71f9e/build/sanitizers/tsan_suppressions.cc [modify] https://crrev.com/de5a958df5a58664d436ad931d23e6cd25c71f9e/third_party/WebKit/Source/core/css/threaded/FontObjectThreadedTest.cpp [modify] https://crrev.com/de5a958df5a58664d436ad931d23e6cd25c71f9e/third_party/WebKit/Source/platform/testing/FontTestHelpers.cpp
,
Jul 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e74d86106cfe369dda63a7e2228c4fa95e2c8a27 commit e74d86106cfe369dda63a7e2228c4fa95e2c8a27 Author: Fernando Serboncini <fserb@chromium.org> Date: Thu Jul 20 15:05:15 2017 Adds tests for off main thread text rendering This tests the two basic functionalitiles of Canvas text rendering: - measureText - drawText Bug: 730692 Change-Id: Ic6365f3b7f77856c09053e55d3a01d0015a744d6 Reviewed-on: https://chromium-review.googlesource.com/576360 Reviewed-by: Justin Novosad <junov@chromium.org> Reviewed-by: danakj <danakj@chromium.org> Commit-Queue: Fernando Serboncini <fserb@chromium.org> Cr-Commit-Position: refs/heads/master@{#488249} [modify] https://crrev.com/e74d86106cfe369dda63a7e2228c4fa95e2c8a27/third_party/WebKit/Source/core/BUILD.gn [add] https://crrev.com/e74d86106cfe369dda63a7e2228c4fa95e2c8a27/third_party/WebKit/Source/core/css/threaded/TextRendererThreadedTest.cpp [modify] https://crrev.com/e74d86106cfe369dda63a7e2228c4fa95e2c8a27/third_party/WebKit/Source/platform/BUILD.gn [add] https://crrev.com/e74d86106cfe369dda63a7e2228c4fa95e2c8a27/third_party/WebKit/Source/platform/graphics/test/MockPaintCanvas.h
,
Jul 20 2017
This is mostly done. :) \o/ #nevertellmetheodds I won't mark it fixed for now, because I don't want to jinx it. But yeah.
,
Dec 6 2017
,
Dec 6 2017
It is fixed it seems.
,
Dec 6 2017
Issue 722511 and Issue 722393 should be closed also. Cheers!
,
Mar 20 2018
Hi all, If I am understanding correctly, https://chromium-review.googlesource.com/c/chromium/src/+/544625 effectively disabled ThreadSanitizer for harfbuzz-ng, which of course made it look thread-safe, but didn't fix the problems (and masked all the new races, should they arrive). I am not sure if there's interest in actually making harfbuzz-ng thread-safe, but off the top of my head there are potential problems in the following places: https://cs.chromium.org/chromium/src/third_party/harfbuzz-ng/src/src/hb-atomic-private.hh?type=cs&q=hb_atomic_ptr_impl_get&l=69 (incorrect memory barrier placement) https://cs.chromium.org/chromium/src/third_party/harfbuzz-ng/src/src/hb-graphite2.cc?type=cs&q=thread+file:%5Esrc/third_party/harfbuzz-ng/src/src/+package:%5Echromium$&l=82 ("Not thread-safe, but fairly harmless") https://cs.chromium.org/chromium/src/third_party/harfbuzz-ng/src/src/hb-common.cc?type=cs&q=thread+file:%5Esrc/third_party/harfbuzz-ng/src/src/+package:%5Echromium$&l=54 ("idempotent and threadsafe") The static initialization here: https://cs.chromium.org/chromium/src/third_party/harfbuzz-ng/src/src/hb-shaper.cc?type=cs&q=lock-free+file:%5Esrc/third_party/harfbuzz-ng/src/src/+package:%5Echromium$&l=39 don't look that harmless either.
,
Mar 20 2018
,
Mar 20 2018
harfbuzz-ng is definitely threadsafe. If it doesn't behave that way, that's a bug that needs to be fixed. Please report them on github upstream. > https://cs.chromium.org/chromium/src/third_party/harfbuzz-ng/src/src/hb-atomic-private.hh?type=cs&q=hb_atomic_ptr_impl_get&l=69 (incorrect memory barrier placement) Can you suggest a fix? I was never fully satisfied with that codepath (which only affects Windows), but have copied that idiom from other libraries. > https://cs.chromium.org/chromium/src/third_party/harfbuzz-ng/src/src/hb-graphite2.cc?type=cs&q=thread+file:%5Esrc/third_party/harfbuzz-ng/src/src/+package:%5Echromium$&l=82 ("Not thread-safe, but fairly harmless") That's a bug that needs to reported and fixed. Note that we do NOT enable graphite so doesn't affect Chrome. > https://cs.chromium.org/chromium/src/third_party/harfbuzz-ng/src/src/hb-common.cc?type=cs&q=thread+file:%5Esrc/third_party/harfbuzz-ng/src/src/+package:%5Echromium$&l=54 ("idempotent and threadsafe") I can make that use atomic, but that sounds excessive. We don't care about exact order here, just atomicity. Are there any architectures that assigning an int is not atomic? What do you suggest? > The static initialization here: https://cs.chromium.org/chromium/src/third_party/harfbuzz-ng/src/src/hb-shaper.cc?type=cs&q=lock-free+file:%5Esrc/third_party/harfbuzz-ng/src/src/+package:%5Echromium$&l=39 That uses atomic cmpexch. What's wrong with that?
,
Mar 26 2018
> Can you suggest a fix? I was never fully satisfied with that codepath (which only affects Windows), but have copied that idiom from other libraries. IIUC this code is attempting to perform an acquire load, so the memory barrier should go after the load, not before it. > I can make that use atomic, but that sounds excessive. We don't care about exact order here, just atomicity. Are there any architectures that assigning an int is not atomic? What do you suggest? This is a question of correct behavior in terms of the C++ standard, not atomicity. Even for word-sized writes the compiler may assume they don't race with anything else and use them e.g. as scratch space. I'm suggesting using a true atomic write. It even won't cost you anything. > That uses atomic cmpexch. What's wrong with that? I didn't look close, but it occurs to me that free_static_shapers() may race with _hb_shapers_get(), as atexit() callbacks don't necessarily wait for other threads to stop. I cannot guarantee that the above list is complete - that's the reason we have ThreadSanitizer. My biggest concern is that https://chromium-review.googlesource.com/c/chromium/src/+/544625 has disabled race detection for harfbuzz-ng in Chrome, so we won't notice when something goes wrong.
,
Mar 26 2018
I have no idea what TSan warning the CL tried to mask to begin with. Any pointers? > IIUC this code is attempting to perform an acquire load, so the memory barrier should go after the load, not before it. I think the goal is that nothing before this read is reordered after it, so the order is correct. I checked again. The entire GNOME stack has always been running in this order. Not going to change. > This is a question of correct behavior in terms of the C++ standard, not atomicity. > Even for word-sized writes the compiler may assume they don't race with anything else and use them e.g. as scratch space. It's a global variable. What can compiler do again?! > I'm suggesting using a true atomic write. It even won't cost you anything. Fair enough. It's not free free, is it? I want a relaxed atomic. Without recent atomic.h (which I cannot rely on) how do I implement that? In all our atomic implementations (hb-atomic-private.h) the atomic_get is implemented as a memory-barrier then a normal read, and writes are a normal write then a memory-barrier. Removing memory-barrier leaves just the read / write. No? > I didn't look close, but it occurs to me that free_static_shapers() may race with _hb_shapers_get(), as atexit() callbacks don't necessarily wait for other threads to stop. That code is just to make valgrind happy. I added a HB_NO_ATEXIT for clients to opt-in to not free stuff. Filed issue to fix them properly: https://github.com/harfbuzz/harfbuzz/issues/923 > I cannot guarantee that the above list is complete - that's the reason we have ThreadSanitizer. While main users of HarfBuzz (Chrome, Android, Firefox) have historically used it single-threaded, other clients (GNOME, google3 Maps tile renderer, Ads ARROW server) have been using HarfBuzz in (massively) multithreaded setting for years. We have yet to receive a single bug report. > My biggest concern is that https://chromium-review.googlesource.com/c/chromium/src/+/544625 has disabled race detection for harfbuzz-ng in Chrome, so we won't notice when something goes wrong. I'm curious why that was done as well.
,
Mar 27 2018
Most certainly harfbuzz-ng doesn't work with ThreadSanitizer out of the box. For example, TSan doesn't comprehend memory barriers, because it reasons in the terms of the happens-before relation. In order to make them work together we'll need to annotate the synchronization primitives (e.g. the atomic load and the cmpxchg operation), or switch to C++ atomics (by the way there was a discussion about adding them to harfbuzz, right? https://bugs.freedesktop.org/show_bug.cgi?id=90305 is the thread) Without doing that, TSan probably reports a lot of random false positives, some of them could have inspired https://chromium-review.googlesource.com/c/chromium/src/+/544625. It may also report true positives, which are inseparable from the false reports, so nobody knows how many actual bugs are sitting there. Anecdotal evidence of not encountering bugs is a poor argument in this case. We've seen cases when the code with TSan errors used to work for years on X86 CPUs with strong memory model (probably the most common use case for desktop Linux machines), but instantly broke when used on multicore ARMs. Some of the properties of the code (e.g. atomicity of int accesses) may even remain on ARM, but from the C++ Standard point of view a racy nonatomic access to an integer variable is undefined behavior. Today, with the rapidly evolving compilers that start optimizing atomics you can't be on the safe side with that. If you want to look deeper into what TSan thinks about harfbuzz, I would suggest that we revert the incorrect suppressions and make the necessary annotations for TSan (or, preferably, switch to modern atomics if they are available).
,
Mar 27 2018
> I think the goal is that nothing before this read is reordered after it, so the order is correct. I checked again. The entire GNOME stack has always been running in this order. Not going to change.
I don't think this is working as intended. Could you please explain which accesses should not be reordered with this read?
Usually, a pair of a release-store and acquire-load work together, providing a happens-before arc that synchronizes accesses in two threads.
Consider you're initializing a structure Str {x, y}, publishing it via a shared pointer Str *shared, and accessing it from another thread like this:
------------------------
Str *shared = null;
T1:
Str *new = malloc()
new->x = 1;
new->y = 2; // (1)
shared = new; // (2)
T2:
Str *read = shared; // (3)
if (read) {
do_something(read->x, read->y); // (4)
}
------------------------
Without any memory barriers writes (1) and (2) can be reordered either by the CPU or by the compiler. As a result, T2 may see a partially initialized structure and use an incorrect value of shared->y. To avoid this, we need a memory barrier between (1) and (2), effectively making (2) a release-store.
If *shared contains some non-zero value before (2), and there is no barrier between (3) and (4), T2 may theoretically see an incorrect value of read->x (this is possible on DEC Alpha), therefore the standard mandates putting a matching barrier between (3) and (4), which makes (3) an acquire-load. Putting a barrier in T2 before (3) doesn't help much.
Because X86 has a strong memory model, all loads are actually acquire-loads, and all stores are release-stores. Therefore the code behaves as if it was synchronized.
,
Mar 27 2018
> Fair enough. It's not free free, is it? I want a relaxed atomic. Without recent atomic.h (which I cannot rely on) how do I implement that? In all our atomic implementations (hb-atomic-private.h) the atomic_get is implemented as a memory-barrier then a normal read, and writes are a normal write then a memory-barrier. Removing memory-barrier leaves just the read / write. No? If you don't have recent atomic.h implementation (i.e. you aren't targeting C11/C++11, correct?) it might be somewhat ok to keep plain memory accesses for relaxed atomics, but we'll still need to tell TSan that those are intended to be atomic.
,
Mar 28 2018
I'll try to implement atomic.h; but no we cannot fully rely on C11/C++11 as we support way older compilers as well (Sun, HP, ..)
,
Mar 28 2018
How do I annotate these for TSan? Any pointers?
,
Mar 28 2018
can you provide a small example of code that needs to be annotated? Also, it's much more preferable to write code that doesn't need annotations. tsan understands most of the sync primitives correctly (but it doesn't like memory barriers, IIRC).
,
Mar 28 2018
Regarding the get memory barrier, looks like we only use that get in conjunction with cmpexch() to implement one-time initializations. Ie.:
retry:
X *x = ptr_get (&static_x);
if (!x)
{
X *new_x = create_x();
if (!ptr_cmpexch (&static_x, x, new_x))
{
free_x (new_x);
goto retry;
}
}
In this case looks like the ptr_get implementation does NOT actually rely on a memory barrier being present at all. Right?
In glib, atomic int / ptr are implemented as:
get: __sync_synchronize(), plain memory read
set: plain memory write, __sync_synchronize()
It's not clear to me what memory model that's implementing.
,
Mar 28 2018
> can you provide a small example of code that needs to be annotated? I don't know. I'm not the one claiming those are needed. As far as I'm concerned HarfBuzz is fully threadsafe. There were two bugs that glider pointed out, which we are fixing. > Also, it's much more preferable to write code that doesn't need annotations. > tsan understands most of the sync primitives correctly (but it doesn't like > memory barriers, IIRC). This is what we use: #define hb_atomic_int_impl_add(AI, V)↦ ↦ __sync_fetch_and_add (&(AI), (V)) #define hb_atomic_ptr_impl_get(P)↦ ↦ (void *) (__sync_synchronize (), *(P)) #define hb_atomic_ptr_impl_cmpexch(P,O,N)↦ __sync_bool_compare_and_swap ((P), (O), (N)) I *think* the memory barrier is unneeded. So, this should be clean already? I haven't seen what false positives were raised. Fernando, can you please share what you were seeing that caused you to disable TSan for HarfBuzz?
,
Mar 28 2018
This looks quite messy. Messy enough that I don't want to give any recommendation -- good chance that I'll be wrong. If this is a one-time initialization, is there a good reason to not use a standard language feature (pthread_once or std::call_once) If you have to be compatible with VERY old compilers/etc that don't support pthread_once or std::call_once, you may put the new code under an #ifdef
,
Mar 28 2018
Yes we support Solaris, Windows, etc. No one implementation can support all of our needs. Might look messy. That is C library development. But I'm confident it's solid and tested and works well.
,
Mar 28 2018
We also don't link to libstdc++.
,
Mar 28 2018
pthread_once is a standard C, I highly recommend to use it whenever you can for one-time init. It's tsan-friendly
,
Mar 28 2018
We support systems without pthread. Like Windows.
,
Mar 28 2018
Of course. Add something like #ifdef whatever pthread_once(...); #else old code #endif This is way better than trying to annotate this code with tsan-friendly annotations. Also, any sane C implementation that has threads will have an equivalent of pthread_once. On Windows I think it's InitOnceExecuteOnce
,
Mar 28 2018
I also tried, before, to use C++ class statics that are atomically initialized. However, Chrome didn't like that this added initilizers to the library link time and forced me to remove that. That was the only portable option I have had found.
,
Mar 28 2018
Doesn't tsan understand cmpexch intrinsic? Then it should be fine on my code.
,
Mar 28 2018
IIRC, those were banned in Chrome because they were not supported by older versions of MSVC. But now Chrome has completely stopped using MSVC, and new versions of MSVC support this idiom anyway. Things have improved :)
,
Mar 28 2018
tsan doesn't understand the memory barriers, IIRC.
,
Apr 3 2018
(Sorry for the delay; long holidays) Assuming that HarfBuzz is really thread-safe, our task is to make TSan understand the happens-before relation between the concurrent memory accesses. For thread creation, deletion and mutexes this already works out of the box. For handwritten atomic synchronization one needs to call special TSan functions that are semantically equivalent to the corresponding C++11 atomics. E.g. the following line: __tsan_atomic32_load(ptr, __tsan_memory_order_acquire); can be used to define hb_atomic_ptr_impl_get(), if I am correctly understanding the point of hb_atomic_ptr_impl_get(). We used to have the full set of annotations in Chrome, take a look at https://chromium.googlesource.com/chromium/src/+/351c9c3e4820e4ff23fcdce43acfda1c83337cc2/base/atomicops_internals_tsan.h Those can be used under an #ifdef. Note that the most tricky part is to ensure that these annotations actually match those handwritten primitives used in harfbuzz. That's why it's always better to stick to something idiomatic, like the atomic definitions here: https://chromium.googlesource.com/chromium/src/+/57a4e4a50c673c25e9cdaab53e32f6e53aa0b574/base/atomicops_internals_x86_gcc.h > In glib, atomic int / ptr are implemented as: > > get: __sync_synchronize(), plain memory read > set: plain memory write, __sync_synchronize() This is really curious, because it's also incorrect for the same reason. Or, if you prefer, it may work on certain platforms (like X86 Linux) and certain compilers (for sure the old GCC versions you need to keep supporting, perhaps even modern GCC and Clang) till something changes (note that Chrome has been ported to several new arches and systems since its birth, and the Clang version used to build the browser is bumped every couple of weeks). > It's not clear to me what memory model that's implementing. Looking at /usr/include/glib-2.0/glib/gatomic.h on my machine I see two implementations of g_atomic_int_get(): - one for C++11 compilers: 90 #define g_atomic_int_get(atomic) \ 91 (G_GNUC_EXTENSION ({ \ 92 G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gint)); \ 93 (void) (0 ? *(atomic) ^ *(atomic) : 0); \ 94 (gint) __atomic_load_4 ((atomic), __ATOMIC_SEQ_CST); \ 95 })) - and another one for the older ones: 135 #define g_atomic_int_get(atomic) \ 136 (G_GNUC_EXTENSION ({ \ 137 G_STATIC_ASSERT (sizeof *(atomic) == sizeof (gint)); \ 138 (void) (0 ? *(atomic) ^ *(atomic) : 0); \ 139 __sync_synchronize (); \ 140 (gint) *(atomic); \ 141 })) So glib developers probably have the sequential consistent memory accesses in mind. Even if they just promoted an acquire load to __ATOMIC_SEQ_CST, the second implementation doesn't provide acquire guarantees. But unlike harfbuzz, glib is on the safe side as long as the C++11-based implementation is used. Fun fact: glib used to have a correct version of g_atomic_int_get(): http://web.mit.edu/ghudson/dev/nokrb/third/glib2/glib/gatomic.c Not sure why they decided to change it.
,
Apr 11 2018
Thanks. After your previous message I convinced myself that the glib implementation is indeed wrong / useless at least for pointer atomic set. I'll flie a bug there and follow up. We don't rely on that in HarfBuzz though. I'll look into annotations eventually. Also, regarding fixing the atexit callbacks, we made them use atomic access now, but that simply doesn't work. There's no way to fix that issue. If a thread calls exit() and other threads are working, there's a chance of use-after-free. I don't see any way to fix that.
,
Aug 2
To give an update: HarfBuzz now uses C++11-style atomic operations (preferably using gcc intrinsics; if not available, C++11 std calls; if not available fallback to platform-specific routines). I've done that even for relaxed atomic accesses. So, if TSan understands those, we should be in good shape. I'm confident that I got it right, but would love to get a review or other feedback. Last but not least, how can we enable TSan on HarfBuzz? Code as it stands now: https://github.com/harfbuzz/harfbuzz/blob/master/src/hb-atomic-private.hh
,
Aug 2
I'll re-enable tsan on harfbuzz and report back.
,
Aug 2
Moved to http://crbug.com/870379 to track harfbuzz + tsan. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by shend@chromium.org
, Jun 7 2017