-
Notifications
You must be signed in to change notification settings - Fork 1.3k
De-mutex GlyphAtlas; expose glyph information to workers via message. #8341
Conversation
@ChrisLoer, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @1ec5 and @ansis to be potential reviewers. |
c8c20a5
to
e40429b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach looks good. My suggestions are focused on simplifying the datastructures and algorithms used in GlyphAtlas
.
src/mbgl/tile/geometry_tile.cpp
Outdated
} | ||
|
||
GeometryTile::~GeometryTile() { | ||
glyphAtlas.removeGlyphs(reinterpret_cast<uintptr_t>(this)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uintptr_t
was a hack to avoid a circular dependency. You can change the signature to GlyphAtlas::removeGlyphs(GlyphRequestor&)
and remove reinterpret_cast
.
src/mbgl/tile/geometry_tile.cpp
Outdated
} | ||
|
||
void GeometryTile::getGlyphs(GlyphDependencies glyphDependencies) { | ||
glyphAtlas.getGlyphs(reinterpret_cast<uintptr_t>(this), std::move(glyphDependencies), *this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here too, remove the reinterpret_cast<uintptr_t>(this)
argument in favor of GlyphRequestor&
/*this
. Probably makes sense to swap the argument order so that it's GlyphAtlas::getGlyphs(GlyphRequestor&, GlyphDependencies)
.
src/mbgl/text/glyph_atlas.hpp
Outdated
|
||
util::WorkQueue workQueue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove #include <mbgl/util/work_queue.hpp>
along with this.
src/mbgl/text/glyph_atlas.cpp
Outdated
tileDependencies.emplace(std::piecewise_construct, | ||
std::forward_as_tuple(tileUID), | ||
std::forward_as_tuple(missing, std::move(glyphDependencies), &requestor)); | ||
for (auto fontStack : missing) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this loop be merged with the generation of missing
above?
src/mbgl/text/glyph_atlas.hpp
Outdated
}; | ||
std::unordered_map<uintptr_t,TileDependency> tileDependencies; | ||
typedef std::pair<FontStack,GlyphRange> PendingGlyphRange; | ||
std::map<PendingGlyphRange, std::set<uintptr_t>> pendingGlyphRanges; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding a new (FontStack, GlyphRange)
-keyed map, can we add a std::set<GlyphRequestor*>
to GlyphPBF
and perform a FontStack
⇢ GlyphRange
⇢ GlyphPBF
lookup via GlyphAtlas::entries
and Entry::ranges
?
src/mbgl/text/glyph_atlas.cpp
Outdated
for (const auto& fontStack : glyphDependencies) { | ||
for (auto glyphID : fontStack.second) { | ||
GlyphRange range = getGlyphRange(glyphID); | ||
if (!hasGlyphRange(fontStack.first,range)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop should perform direct lookups in entries
rather than calling hasGlyphRange
. That way, the entries.find(fontStack)
step can be hoisted out of the inner loop.
src/mbgl/text/glyph_atlas.cpp
Outdated
void GlyphAtlas::getGlyphs(uintptr_t tileUID, GlyphDependencies glyphDependencies, GlyphRequestor& requestor) { | ||
// Figure out which glyph ranges need to be fetched | ||
GlyphRangeDependencies missing; | ||
for (const auto& fontStack : glyphDependencies) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop variable is a pair whose first member is a FontStack
, so this name is misleading.
e40429b
to
c921020
Compare
I'm moving forward with making matching changes in One performance consideration I didn't consider until now is that we won't be able to start on If we think we need to keep the incremental preparation capability, I'll need to change the /cc @mourner |
My guess is that incremental preparation is not critical. It looks like GL JS does not use it. But I don't think it's ever been benchmarked or otherwise empirically measured.
The other thing that |
Pushed a few more suggested simplifications in d3c91db. Further possible refactoring I see:
|
c921020
to
688f34e
Compare
I do not have the ASCII diagramming skills necessary to include the updated I'm much less familiar with the
|
I'm a bit baffled by the bitrise failure on text-font/chinese: http://mapbox.s3.amazonaws.com/mapbox-gl-native/render-tests/9980/index.html It looks like four symbols in that test get partially distorted. It doesn't seem to be obviously timing related since the same four symbols had the problem three builds in a row. The problem doesn't show up when I run test locally (xcode 8.2, macOS 10.12.3), or on any of the other platforms that run the node tests.... Maybe some kind of difference in how the platform handles some of the math? |
504af48
to
6efbca6
Compare
I did some basic benchmarking by running the macosapp in release mode, with the map centered over greater asia and using a style that loads
These numbers make me feel fairly confident that this branch isn't introducing a major performance regression, although there are definitely theoretical cases where these changes could slow us down. |
162c97a
to
3dce1ca
Compare
The test failure might be an instance of #6863 -- it's hard to tell exactly because the icon is partially obscured by text. |
Interesting... yeah it looks sort of similar -- although the screenshots in #6863 looks like a sort of alternating pixel-on/pixel-off pattern, whereas in this case a whole bunch of pixels just seem to be shifted one pixel right. |
2feb36d
to
fe34d16
Compare
The behavior looks very similar using basketballs instead of dogs (just to change the index into the sprite texture): http://mapbox.s3.amazonaws.com/mapbox-gl-native/render-tests/10009/index.html |
And it still happens even with bigger basketballs: http://mapbox.s3.amazonaws.com/mapbox-gl-native/render-tests/10010/index.html It kind of looks like the vertices for the lower right triangle are ending up shifted one pixel right from the vertices for the upper left triangle. |
b340de5
to
8570a88
Compare
I did an experiment with the fragment shader doing its own rounding... the results (http://mapbox.s3.amazonaws.com/mapbox-gl-native/render-tests/10015/index.html) show lots of little changes for icons, but for the text-font/chinese test the broken behavior is totally unaffected. |
On the other hand, making the symbol icon vertex shader round |
@@ -143,24 +154,46 @@ void GeometryTileWorker::setPlacementConfig(PlacementConfig placementConfig_, ui | |||
} | |||
} | |||
|
|||
void GeometryTileWorker::onGlyphsAvailable(GlyphPositionMap glyphs) { | |||
glyphPositions = std::move(glyphs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add assert(waitingForGlyphs);
} | ||
|
||
void GeometryTileWorker::onIconsAvailable(IconAtlasMap icons_) { | ||
icons = std::move(icons_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add assert(waitingForIcons);
case Idle: | ||
if (hasPendingSymbolDependencies()) { | ||
case NeedPlacement: | ||
if (!hasPendingSymbolDependencies()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setPlacementConfig
needs this guard as well for case Idle
. Or alternatively (my preference): remove the places coalesce
is currently called in favor of inlining it at the end of attemptPlacement
. attemptPlacement
already has an early exit for the hasPendingSymbolDependencies
case, so it can then be called unconditionally.
Along with this, I also suggest factoring the followup to redoLayout
that's currently repeated in four places:
if (!hasPendingSymbolDependencies()) {
coalesce();
} else {
state = NeedPlacement;
}
Move that into redoLayout
while relying on the fact that attemptPlacement
now itself calls coalesce
:
void GeometryTileWorker::redoLayout() {
...
if (hasPendingSymbolDependencies()) {
state = NeedPlacement;
} else {
attemptPlacement();
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are good suggestions. Making the changes caused some problems with a part of the logic I hadn't fully worked through before (that is: what happens when messages arrive but the placement config or data isn't set yet). I addressed those and everything looked great aside from the state transition diagram getting more complicated... but now I'm seeing an intermittent memory corruption problem. So that's reassuring.
I'll take another stab at this tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you push these changes?
@jfirebaugh or @kkaefer, maybe you can help me figure out how to handle this tricky case:
The previous behavior was that since we were in the Now that symbol dependencies are being satisfied asynchronously, the behavior is a little different. The simplest approach is just to keep doing pretty much the same thing:
The problem with this approach is that it opens up room for starvation -- where we keep getting new layout requests and we never finish making placement calls. The starvation possibility technically existed before but it would be very hard to trigger since each layout call would have to introduce a new symbol dependency. An alternative approach I've been trying is:
But the problem with this approach is that if someone's calling After writing this all down, it occurs to me there's a third approach that probably makes sense here -- hold on to symbol dependencies in the |
I'm not terribly worried about the problems with either of the first two approaches:
|
The I'm happy with the solution of holding on to symbols between layout calls -- it's an easy and sensible optimization. It does make me realize there's a kind of funny case where a tile with lots of changing text (on some map hooked up to a live data feed?) wouldn't garbage collect glyphs until the tile was removed. But nothing here changes that behavior. The changes I just pushed pass all my tests locally, but I don't have a fully up-to-date state transition diagram for them and I think there might still be some edge cases hiding out around what happens when you have symbol-dependent layout happening before the initial call to |
01405cb
to
0494d2f
Compare
@jfirebaugh / @kkaefer This is just about ready to go from my point of view, but I'm still waiting for further input/perspective from you guys. I'd like to get it in soon-ish if it's reasonable so I can stop worrying about keeping the PR in sync with master. |
|
I thought that it would be a simplification to get rid of the I don't think the two-step rendering actually adds that much complication on the side of It seems to me the extra complication comes from:
|
I went to remove the code I had that tracked individual icon dependencies, but in doing so realized we have a problem with I'm not sure what the right solution is here:
It looks like @ivovandongen touched on a closely related problem at #6735... and I see there's a proposal for a separate |
I think the right fix is a combination of the first and third points, but let's defer this for now. |
|
f705697
to
669024d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Phew! Nice work.
669024d
to
e76edc4
Compare
- Expose glyph and icon information to workers via message interface. - Glyph/SpriteAtlas track which tiles have outstanding requests and send messages to them when glyphs/icons become available. - Remove obsolete "updateSymbolDependentTiles" pathway - Symbol preparation for a tile now depends on all glyphs becoming available before it can start. - Start tracking individual icons needed for a tile, although we don't do anything with the information yet. - Introduce typedef for GlyphID
presumed to stem from issue #6863.
e76edc4
to
0c0beaf
Compare
Hallelujah! Thanks for all the help, @jfirebaugh. For anyone reading this PR in the future, ignore all the state transition diagrams in the conversation here. The end result matches what was in the code comment all along, although I've included an updated graphic that spells it out in a little more detail. |
This PR ports the gl-js
GlyphAtlas
architecture (roughly) to gl-native. Instead of using a mutex-protectedGlyphAtlas
shared across all worker threads, we restrict access to theGlyphAtlas
to the main thread, and use a message passing interface for worker threads to request glyph information from the atlas.Working on Harfbuzz/Freetype code motivated me to make these changes -- as the shaping code grew more complex/expensive, holding onto the
GlyphAtlas
mutex became more of a point of resource contention. Also, reasoning about mutexes is hard.I haven't done any performance benchmarking on this PR yet. In theory it should reduce contention, but it also introduces some extra copying and the extra round trip of always having to send a message to the main thread for glyphs even if they're already locally available.
I'm putting up the PR early to get feedback on the approach. If the approach looks good, we can also use it for
SpriteAtlas
, and at that point the worker threads should (?) be running independent of shared state the same way they do in gl-js. If we're doing the same changes for both atlases, we could also update all the "symbol dependency" code to share one common pathway. Right now this new pathway is just kind of glued on to thesymbolDependenciesChanged
code from #6928.I started out by porting gl-js callbacks more-or-less directly to C++ lambdas, but tracking ownership quickly got... complicated. Instead, I made
GlyphAtlas
directly responsible for tracking who needs to get sent what data when glyphs become available. There's more code than I'd like added toGlyphAtlas
, but hopefully it's a simplification overall./cc @kkaefer @jfirebaugh @lucaswoj @mollymerp