[core] Immutable style Impls by jfirebaugh · Pull Request #8929 · mapbox/mapbox-gl-native

jfirebaugh · 2017-05-09T00:46:20Z

Implements immutability for:

Light::Impl
Source::Impl and subclasses
Layer::Impl and subclasses

jfirebaugh · 2017-05-09T00:54:48Z

+            == std::tie(rhs->id, lhs->type);
+    };
+
+    longest_common_subsequence(a.begin(), a.end(), b.begin(), b.end(), std::back_inserter(lcs), eq);


This LCS implementation is the unrefined O(ND) space algorithm from the classic paper. I'm not sure if we need a full LCS, versus a more basic unordered set difference -- it probably depends if we want to implement a "move layer" operation rather than the current "remove and re-add" implementation (see mapbox/mapbox-gl-js#4142).

If we do end up keeping LCS, we should probably implement the O(N) space refinement presented in the paper. I haven't yet invested the time to understand it or write a high-quality implementation.

ivovandongen · 2017-05-09T09:19:39Z

Starting to look good! One thing I'm wondering about is wether we should add the option to mutate multiple properties at once (in layers for example) to avoid unnecessary copies of the implementations.

ivovandongen · 2017-05-09T10:58:30Z


    float dist = length(v_pos - gl_FragCoord.xy);
-    float alpha = smoothstep(1.0, 0.0, dist);
+    float alpha = 1.0 - smoothstep(0.0, 1.0, dist);


Did these snuck in by accident?

See #8751 and #8760

jfirebaugh · 2017-05-10T21:24:37Z

This is ready for review. I checked for conflicts with #8937; they are minimal (added a header file in the same place).

jfirebaugh · 2017-05-10T21:26:02Z

zoomChanged is missing here, and none of our regression tests caught it. Will investigate.

Nice catch 👍

ivovandongen

Looks good!

Ran the Android integration tests as well, no regressions. Although the run time (locally) increased from ~5:30min to ~8:00min. No immediately obvious cause for this, it is spread fairly evenly over the style tests.
Ran through all the Android test app activities manually, no regressions I could spot.

kkaefer · 2017-05-11T08:48:57Z

    // Private implementation
-    const std::unique_ptr<Impl> baseImpl;
+    class Impl;
+    Immutable<Impl> baseImpl;


I realize this has been public before, but is there a way we can make it private now?

Style at a minimum would need to be friended. GeometryTile also uses it; it's possible that use could be eliminated. I'd like to leave this for a followup change though.

kkaefer · 2017-05-11T08:49:46Z


 <% } -%>
+    // Visibility
+    void setVisibility(VisibilityType) final;


Nit: Can we add override to be consistent with the rest of the codebase?

You want to write override final or final override? Neither of those appear elsewhere in the codebase.

I'm specifically interested in override (final override reads better), to make it fully clear that this is a virtual function override.

Let's discuss making that style change separately. We already have quite a few uses of final without override.

kkaefer · 2017-05-11T08:50:30Z

-    <%- camelize(type) %>Layer(const <%- camelize(type) %>Layer&) = delete;
+    Mutable<Impl> mutableImpl() const;
+    <%- camelize(type) %>Layer(Immutable<Impl>);
+    std::unique_ptr<Layer> cloneRef(const std::string& id) const final;


Same note about privatization here. I know they've been public before, so this may be a follow-up change.

kkaefer · 2017-05-11T08:51:19Z

    }

+    LayerType getType() const;
    const std::string& getID() const;


This returns a reference to a string, which is stored in the immutable implementation. Now that we're switching this frequently, it means that the reference can become invalid when you do something like this:

auto& id = layer->getID(); layer->setMaxZoom(2); std::cout << id;

Instead of returning a reference to objects inside the frequently-changing Immutable, we should return the ID as a value.

kkaefer · 2017-05-11T08:51:47Z

+
+    template <class S> friend class Immutable;
+    template <class S, class... Args> friend Mutable<S> makeMutable(Args&&...);
+};


Mutable/Immutable is vulnerable to the following scenario: Contained type inherits from std::enable_shared_from_this. It's now possible to obtain a shared_ptr from the Mutable object, convert that to an Immutable, and continue to mutate the object with the previously obtained shared_ptr handle. The same goes for EnableImmutableFromThis. It seems like a promise that it can't keep...

Well, sure. But you could also just hold onto the raw pointer from Mutable::get(), a reference from Mutable::operator*, const_cast, .... There's no way to 100% prevent misuse in C++; that would require language support in the style of Rust borrow checking.

Yup, but shared_ptr indicates shared ownership, whereas the others don't.

kkaefer · 2017-05-11T13:33:32Z

 * Members that are private in `FooLayer::Impl` are internal to "foo" layers.
 */
-class Layer::Impl {
+class Layer::Impl : public EnableImmutableFromThis<Layer::Impl> {


This allows converting an existing object to an Immutable, but this object has many mutable fields that cut be mutated from the original handle.

EnableImmutableFromThis is required only for Source::Impl::createRenderSource and Layer::Impl::createRenderLayer. It's probably a good idea to change how those operations are performed so that EnableImmutableFromThis isn't necessary. I see two options:

Require passing an Immutable<Impl> self reference to those methods (3e7ce6ba27b31a73a9242e4e14e8eaacefe42a5e). I'm not crazy about this because it allows the possibility of passing some other reference that doesn't match this.

Replace the virtual dispatch with a static dispatch -- switch statement or Layer::accept.

kkaefer · 2017-05-11T14:52:52Z

+    return longest_common_subsequence(a, endA, b, endB, outIt, std::equal_to<>());
+}
+
+}


This file could use a lot more documentation

jfirebaugh · 2017-05-11T20:58:23Z

Bizarre link error on linux builds:

[3/3] Linking CXX executable mbgl-offline
FAILED: mbgl-offline 
: && /usr/bin/ccache  clang++-3.9 -Qunused-arguments -fcolor-diagnostics  -std=c++14 -ftemplate-depth=1024 -Wall -Wextra -Wshadow -Werror -Wno-variadic-macros -Wno-unknown-pragmas -g   CMakeFiles/mbgl-offline.dir/bin/offline.cpp.o CMakeFiles/mbgl-offline.dir/platform/default/mbgl/util/default_styles.cpp.o  -o mbgl-offline  -rdynamic libmbgl-core.a ../../../mason_packages/linux-x86_64/boost_libprogram_options/1.62.0/lib/libboost_program_options.a -L/home/travis/build/mapbox/mapbox-gl-native/mason_packages/linux-x86_64/boost_libprogram_options/1.62.0/lib -lboost_program_options libmbgl-loop-uv.a -L/home/travis/build/mapbox/mapbox-gl-native/mason_packages/linux-x86_64/mesa/13.0.4/lib -lGLESv2 -lEGL -lgbm ../../../mason_packages/linux-x86_64/sqlite/3.14.2/lib/libsqlite3.a -ldl -lpthread ../../../mason_packages/linux-x86_64/nunicode/1.7.1/lib/libnu.a ../../../mason_packages/linux-x86_64/libpng/1.6.25/lib/libpng.a -lz -lm ../../../mason_packages/linux-x86_64/libjpeg-turbo/1.5.0/lib/libjpeg.a ../../../mason_packages/linux-x86_64/webp/0.5.1/lib/libwebp.a -pthread -lm ../../../mason_packages/linux-x86_64/icu/58.1-min-size/lib/libicuuc.a -lz -lcurl ../../../mason_packages/linux-x86_64/libuv/1.9.1/lib/libuv.a -lrt -lpthread -lnsl -ldl && :
libmbgl-loop-uv.a(run_loop.cpp.o): In function `operator()':
/home/travis/build/mapbox/mapbox-gl-native/build/linux-x86_64/Debug/../../../include/mbgl/util/run_loop.hpp:86: undefined reference to `mbgl::Mailbox::maybeReceive(std::weak_ptr<mbgl::Mailbox>)'

Seems totally unrelated to any of the changes in cebc5f11cd834a6cf478b267df5f436ab5fd7362.

Avoid dangling references in the following sequence: auto& id = layer->getID(); layer->setMaxZoom(2); std::cout << id; The reference would be dangling because mutating the layer allocates a new Immutable impl, and there may be no references to the prior impl, which held the id.

…create * Eliminates the need for EnableImmutableFromThis * Eliminates the dependency of {Source,Layer}::Impl on corresponding Render class (circular dependency)

jfirebaugh requested review from ivovandongen and kkaefer May 9, 2017 00:46

jfirebaugh commented May 9, 2017

View reviewed changes

ivovandongen reviewed May 9, 2017

View reviewed changes

jfirebaugh force-pushed the immutable branch 3 times, most recently from a696800 to bac2476 Compare May 9, 2017 21:32

kkaefer added the Core The cross-platform C++ core, aka mbgl label May 10, 2017

ivovandongen mentioned this pull request May 10, 2017

Asynchronous rendering #8820

Closed

28 tasks

jfirebaugh force-pushed the immutable branch from bac2476 to 2fc4c22 Compare May 10, 2017 21:23

jfirebaugh requested a review from ivovandongen May 10, 2017 21:23

jfirebaugh commented May 10, 2017

View reviewed changes

jfirebaugh mentioned this pull request May 10, 2017

Camera functions not reevaluated when zoom changes #8952

Closed

jfirebaugh force-pushed the immutable branch from 2fc4c22 to 8f8710e Compare May 10, 2017 22:03

jfirebaugh mentioned this pull request May 10, 2017

[core, node, darwin, qt] Remove support for paint classes #8953

Merged

ivovandongen approved these changes May 11, 2017

View reviewed changes

kkaefer reviewed May 11, 2017

View reviewed changes

jfirebaugh force-pushed the immutable branch 2 times, most recently from 3db6ac8 to 38d1677 Compare May 11, 2017 20:07

jfirebaugh force-pushed the immutable branch from 9ce3c92 to d7cb621 Compare May 12, 2017 16:39

jfirebaugh added 4 commits May 12, 2017 10:07

[core] Immutable Impls

6ca4af2

[core] Remove unused RenderLayer::clone

7d6f79b

[core] Replace createRender{Source,Layer} with Render{Source,Layer}::…

31f68b2

…create * Eliminates the need for EnableImmutableFromThis * Eliminates the dependency of {Source,Layer}::Impl on corresponding Render class (circular dependency)

jfirebaugh force-pushed the immutable branch 2 times, most recently from 9fffcd3 to e859904 Compare May 12, 2017 17:40

[linux] loop-uv depends on mbgl-core for linking

2228ad9

jfirebaugh force-pushed the immutable branch from e859904 to 2228ad9 Compare May 12, 2017 17:55

jfirebaugh merged commit c80f3e9 into master May 12, 2017

jfirebaugh deleted the immutable branch May 12, 2017 18:29

jfirebaugh mentioned this pull request May 12, 2017

[core] Immutable<Impl> for Image #8985

Merged

LukasPaczos mentioned this pull request Mar 6, 2018

Layer setMinZoom and l.setMaxZoom not working after 5.5.x? #11386

Closed

Conversation

jfirebaugh commented May 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivovandongen commented May 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfirebaugh commented May 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivovandongen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfirebaugh commented May 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants