From 31a0d18ca1961b1cfb0785a1921b1f093167ebda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= Date: Mon, 19 Jan 2015 17:40:24 +0100 Subject: [PATCH] remove circular shared_ptr and a few other memory leaks --- gyp/version.gypi | 2 +- include/mbgl/storage/default/request.hpp | 4 ++ macosx/mapboxgl-app.gyp | 3 ++ platform/darwin/http_request_cocoa.mm | 64 +++++++++++++++--------- platform/darwin/log_nslog.mm | 30 ++++++----- platform/darwin/string_nsstring.mm | 26 ++++++---- src/mbgl/map/map.cpp | 5 +- src/mbgl/map/vector_tile_data.cpp | 7 +++ src/mbgl/storage/request.cpp | 51 ++++++++++--------- src/mbgl/style/style.hpp | 2 +- src/mbgl/style/style_bucket.cpp | 12 ++++- src/mbgl/style/style_bucket.hpp | 31 +++++++++--- test/headless.cpp | 4 +- test/test.gyp | 3 +- 14 files changed, 159 insertions(+), 85 deletions(-) diff --git a/gyp/version.gypi b/gyp/version.gypi index 341d9d3a2..68f777f0a 100644 --- a/gyp/version.gypi +++ b/gyp/version.gypi @@ -13,7 +13,7 @@ 'outputs': [ '<(SHARED_INTERMEDIATE_DIR)/include/mbgl/util/version.hpp', ], - 'action': ['<@(python)', 'scripts/build-version.py', '<(SHARED_INTERMEDIATE_DIR)', ' #include -dispatch_once_t request_initialize = 0; -NSURLSession *session = nullptr; -NSString *userAgent = nil; - namespace mbgl { enum class ResponseStatus : uint8_t { @@ -84,12 +80,39 @@ private: class HTTPCocoaContext : public HTTPContext { public: HTTPCocoaContext(uv_loop_t *loop); + ~HTTPCocoaContext(); + + NSURLSession *session = nil; + NSString *userAgent = nil; }; template<> pthread_key_t HTTPContext::key{}; template<> pthread_once_t HTTPContext::once = PTHREAD_ONCE_INIT; -HTTPCocoaContext::HTTPCocoaContext(uv_loop_t *loop_) : HTTPContext(loop_) {} +HTTPCocoaContext::HTTPCocoaContext(uv_loop_t *loop_) : HTTPContext(loop_) { + @autoreleasepool { + NSURLSessionConfiguration *sessionConfig = + [NSURLSessionConfiguration defaultSessionConfiguration]; + sessionConfig.timeoutIntervalForResource = 30; + sessionConfig.HTTPMaximumConnectionsPerHost = 8; + sessionConfig.requestCachePolicy = NSURLRequestReloadIgnoringLocalCacheData; + sessionConfig.URLCache = nil; + + session = [NSURLSession sessionWithConfiguration:sessionConfig]; + [session retain]; + + // Write user agent string + userAgent = @"MapboxGL"; + } +} + +HTTPCocoaContext::~HTTPCocoaContext() { + [session release]; + session = nullptr; + + [userAgent release]; + userAgent = nullptr; +} // ------------------------------------------------------------------------------------------------- @@ -128,18 +151,22 @@ void HTTPRequestImpl::start() { } } - [req addValue:userAgent forHTTPHeaderField:@"User-Agent"]; + [req addValue:context->userAgent forHTTPHeaderField:@"User-Agent"]; - task = [session dataTaskWithRequest:req + task = [context->session dataTaskWithRequest:req completionHandler:^(NSData *data, NSURLResponse *res, NSError *error) { handleResult(data, res, error); }]; [req release]; + [task retain]; [task resume]; } } void HTTPRequestImpl::handleResponse() { - task = nullptr; + if (task) { + [task release]; + task = nullptr; + } if (request) { if (status == ResponseStatus::TemporaryError && attempts < maxAttempts) { @@ -171,8 +198,11 @@ void HTTPRequestImpl::cancel() { context->removeRequest(request); request = nullptr; - [task cancel]; - task = nullptr; + if (task) { + [task cancel]; + [task release]; + task = nullptr; + } } HTTPRequestImpl::~HTTPRequestImpl() { @@ -340,20 +370,6 @@ void HTTPRequestImpl::restart(uv_timer_t *timer, int) { HTTPRequest::HTTPRequest(DefaultFileSource *source, const Resource &resource) : SharedRequestBase(source, resource) { - // Global initialization. - dispatch_once(&request_initialize, ^{ - NSURLSessionConfiguration *sessionConfig = - [NSURLSessionConfiguration defaultSessionConfiguration]; - sessionConfig.timeoutIntervalForResource = 30; - sessionConfig.HTTPMaximumConnectionsPerHost = 8; - sessionConfig.requestCachePolicy = NSURLRequestReloadIgnoringLocalCacheData; - sessionConfig.URLCache = nil; - - session = [NSURLSession sessionWithConfiguration:sessionConfig]; - - // Write user agent string - userAgent = @"MapboxGL"; - }); } HTTPRequest::~HTTPRequest() { diff --git a/platform/darwin/log_nslog.mm b/platform/darwin/log_nslog.mm index b196930b2..ea5fddf0b 100644 --- a/platform/darwin/log_nslog.mm +++ b/platform/darwin/log_nslog.mm @@ -7,14 +7,13 @@ namespace mbgl { void NSLogBackend::record(EventSeverity severity, Event event, const std::string &msg) { - NSLog(@"[%s] %s: %@", EventSeverityClass(severity).c_str(), EventClass(event).c_str(), - [[NSString alloc] initWithBytes:msg.data() - length:msg.size() - encoding:NSUTF8StringEncoding]); + NSString *message = + [[NSString alloc] initWithBytes:msg.data() length:msg.size() encoding:NSUTF8StringEncoding]; + NSLog(@"[%s] %s: %@", EventSeverityClass(severity).c_str(), EventClass(event).c_str(), message); + [message release]; } - -void NSLogBackend::record(EventSeverity severity, Event event, const char* format, ...) { +void NSLogBackend::record(EventSeverity severity, Event event, const char *format, ...) { va_list args; va_start(args, format); const size_t len = vsnprintf(NULL, 0, format, args); @@ -23,17 +22,22 @@ void NSLogBackend::record(EventSeverity severity, Event event, const char* forma va_start(args, format); vsnprintf(buffer.get(), len + 1, format, args); va_end(args); - NSLog(@"[%s] %s: %s", EventSeverityClass(severity).c_str(), EventClass(event).c_str(), buffer.get()); + NSLog(@"[%s] %s: %s", EventSeverityClass(severity).c_str(), EventClass(event).c_str(), + buffer.get()); } void NSLogBackend::record(EventSeverity severity, Event event, int64_t code) { - NSLog(@"[%s] %s: (%lld)", EventSeverityClass(severity).c_str(), EventClass(event).c_str(), code); + NSLog(@"[%s] %s: (%lld)", EventSeverityClass(severity).c_str(), EventClass(event).c_str(), + code); } -void NSLogBackend::record(EventSeverity severity, Event event, int64_t code, const std::string &msg) { - NSLog(@"[%s] %s: (%lld) %@", EventSeverityClass(severity).c_str(), EventClass(event).c_str(), code, - [[NSString alloc] initWithBytes:msg.data() - length:msg.size() - encoding:NSUTF8StringEncoding]); +void NSLogBackend::record(EventSeverity severity, Event event, int64_t code, + const std::string &msg) { + NSString *message = + [[NSString alloc] initWithBytes:msg.data() length:msg.size() encoding:NSUTF8StringEncoding]; + NSLog(@"[%s] %s: (%lld) %@", EventSeverityClass(severity).c_str(), EventClass(event).c_str(), + code, message); + [message release]; } + } diff --git a/platform/darwin/string_nsstring.mm b/platform/darwin/string_nsstring.mm index ef6b96062..86c2c07ed 100644 --- a/platform/darwin/string_nsstring.mm +++ b/platform/darwin/string_nsstring.mm @@ -6,20 +6,26 @@ namespace mbgl { namespace platform { std::string uppercase(const std::string &string) { - NSString *nsstring = [[NSString alloc] initWithBytesNoCopy:const_cast(string.data()) length:string.size() encoding:NSUTF8StringEncoding freeWhenDone:NO]; - NSString *nsstring2 = [nsstring uppercaseString]; - [nsstring release]; - const std::string result { [nsstring2 cStringUsingEncoding:NSUTF8StringEncoding], [nsstring2 lengthOfBytesUsingEncoding:NSUTF8StringEncoding] }; - [nsstring2 release]; + NSString *original = [[NSString alloc] initWithBytesNoCopy:const_cast(string.data()) + length:string.size() + encoding:NSUTF8StringEncoding + freeWhenDone:NO]; + NSString *uppercase = [original uppercaseString]; + const std::string result{[uppercase cStringUsingEncoding : NSUTF8StringEncoding], + [uppercase lengthOfBytesUsingEncoding:NSUTF8StringEncoding]}; + [original release]; return result; } std::string lowercase(const std::string &string) { - NSString *nsstring = [[NSString alloc] initWithBytesNoCopy:const_cast(string.data()) length:string.size() encoding:NSUTF8StringEncoding freeWhenDone:NO]; - NSString *nsstring2 = [nsstring lowercaseString]; - [nsstring release]; - const std::string result { [nsstring2 cStringUsingEncoding:NSUTF8StringEncoding], [nsstring2 lengthOfBytesUsingEncoding:NSUTF8StringEncoding] }; - [nsstring2 release]; + NSString *original = [[NSString alloc] initWithBytesNoCopy:const_cast(string.data()) + length:string.size() + encoding:NSUTF8StringEncoding + freeWhenDone:NO]; + NSString *lowercase = [original lowercaseString]; + const std::string result{[lowercase cStringUsingEncoding : NSUTF8StringEncoding], + [lowercase lengthOfBytesUsingEncoding:NSUTF8StringEncoding]}; + [original release]; return result; } diff --git a/src/mbgl/map/map.cpp b/src/mbgl/map/map.cpp index 112499a48..69056cb57 100644 --- a/src/mbgl/map/map.cpp +++ b/src/mbgl/map/map.cpp @@ -192,8 +192,6 @@ void Map::start(bool startPaused) { pthread_setname_np("Map"); #endif - workers = util::make_unique(**loop, 4, "Tile Worker"); - run(); #ifndef NDEBUG @@ -276,6 +274,9 @@ void Map::run() { } view.activate(); + + workers = util::make_unique(**loop, 4, "Tile Worker"); + setup(); prepare(); diff --git a/src/mbgl/map/vector_tile_data.cpp b/src/mbgl/map/vector_tile_data.cpp index ca98d2695..646ad7318 100644 --- a/src/mbgl/map/vector_tile_data.cpp +++ b/src/mbgl/map/vector_tile_data.cpp @@ -36,6 +36,10 @@ void VectorTileData::parse() { } try { + if (!style) { + throw std::runtime_error("style isn't present in VectorTileData object anymore"); + } + // Parsing creates state that is encapsulated in TileParser. While parsing, // the TileParser object writes results into this objects. All other state // is going to be discarded afterwards. @@ -43,6 +47,9 @@ void VectorTileData::parse() { glyphAtlas, glyphStore, spriteAtlas, sprite, texturePool); + // Clear the style so that we don't have a cycle in the shared_ptr references. + style.reset(); + parser.parse(); } catch (const std::exception& ex) { Log::Error(Event::ParseTile, "Parsing [%d/%d/%d] failed: %s", id.z, id.x, id.y, ex.what()); diff --git a/src/mbgl/storage/request.cpp b/src/mbgl/storage/request.cpp index c771acb92..1bd58571a 100644 --- a/src/mbgl/storage/request.cpp +++ b/src/mbgl/storage/request.cpp @@ -20,26 +20,29 @@ Request::Request(const Resource &resource_, uv_loop_t *loop, Callback callback_) if (loop) { notify_async = new uv_async_t; notify_async->data = this; - uv_async_init(loop, notify_async, [](uv_async_t *async, int) { - auto request = reinterpret_cast(async->data); - uv::close(async); - - if (!request->destruct_async) { - // We haven't created a cancel request, so we can safely delete this Request object - // since it won't be accessed in the future. - assert(request->response); - request->callback(*request->response); - delete request; - } else { - // Otherwise, we're waiting for for the destruct notification to be delivered in order - // to delete the Request object. We're doing this since we can't know whether the - // DefaultFileSource is still sending a cancel event, which means this object must still - // exist. - } - }); + uv_async_init(loop, notify_async, notifyCallback); } } +void Request::notifyCallback(uv_async_t *async, int) { + auto request = reinterpret_cast(async->data); + uv::close(async); + + if (!request->destruct_async) { + // We haven't created a cancel request, so we can safely delete this Request object + // since it won't be accessed in the future. + assert(request->response); + request->callback(*request->response); + delete request; + } else { + // Otherwise, we're waiting for for the destruct notification to be delivered in order + // to delete the Request object. We're doing this since we can't know whether the + // DefaultFileSource is still sending a cancel event, which means this object must still + // exist. + } +} + + Request::~Request() { if (notify_async) { // Request objects can be destructed in other threads when the user didn't supply a loop. @@ -64,12 +67,14 @@ void Request::cancel() { assert(!destruct_async); destruct_async = new uv_async_t; destruct_async->data = this; - uv_async_init(notify_async->loop, destruct_async, [](uv_async_t *async, int) { - // The destruct_async will be invoked *after* the notify_async callback has already run. - auto request = reinterpret_cast(async->data); - uv::close(async); - delete request; - }); + uv_async_init(notify_async->loop, destruct_async, cancelCallback); +} + +void Request::cancelCallback(uv_async_t *async, int) { + // The destruct_async will be invoked *after* the notify_async callback has already run. + auto request = reinterpret_cast(async->data); + uv::close(async); + delete request; } // This gets called from the FileSource thread, and will only ever be invoked after cancel() was called diff --git a/src/mbgl/style/style.hpp b/src/mbgl/style/style.hpp index 5517a56a7..751a91be6 100644 --- a/src/mbgl/style/style.hpp +++ b/src/mbgl/style/style.hpp @@ -21,7 +21,7 @@ class Sprite; class StyleLayer; class StyleLayerGroup; -class Style { +class Style : public util::noncopyable { public: struct exception : std::runtime_error { exception(const char *msg) : std::runtime_error(msg) {} }; diff --git a/src/mbgl/style/style_bucket.cpp b/src/mbgl/style/style_bucket.cpp index 20d5d1147..6e866f303 100644 --- a/src/mbgl/style/style_bucket.cpp +++ b/src/mbgl/style/style_bucket.cpp @@ -2,12 +2,20 @@ namespace mbgl { -StyleBucket::StyleBucket(StyleLayerType type_) :type(type_) {} - template<> const StyleBucketFill &defaultLayoutProperties() { static StyleBucketFill p; return p; } template<> const StyleBucketLine &defaultLayoutProperties() { static StyleBucketLine p; return p; } template<> const StyleBucketSymbol &defaultLayoutProperties() { static StyleBucketSymbol p; return p; } template<> const StyleBucketRaster &defaultLayoutProperties() { static StyleBucketRaster p; return p; } template<> const StyleBucketBackground &defaultLayoutProperties() { static StyleBucketBackground p; return p; } +StyleBucket::StyleBucket(StyleLayerType type_) : type(type_) { + switch (type) { + case StyleLayerType::Fill: render.set(); break; + case StyleLayerType::Line: render.set(); break; + case StyleLayerType::Symbol: render.set(); break; + case StyleLayerType::Raster: render.set(); break; + default: break; + } +} + } \ No newline at end of file diff --git a/src/mbgl/style/style_bucket.hpp b/src/mbgl/style/style_bucket.hpp index 08884c2e0..eac899cb9 100644 --- a/src/mbgl/style/style_bucket.hpp +++ b/src/mbgl/style/style_bucket.hpp @@ -19,10 +19,23 @@ class Source; class StyleBucketFill { public: + // Make movable only. + StyleBucketFill() = default; + StyleBucketFill(StyleBucketFill &&) = default; + StyleBucketFill& operator=(StyleBucketFill &&) = default; + StyleBucketFill(const StyleBucketFill &) = delete; + StyleBucketFill& operator=(const StyleBucketFill &) = delete; }; class StyleBucketLine { public: + // Make movable only. + StyleBucketLine() = default; + StyleBucketLine(StyleBucketLine &&) = default; + StyleBucketLine& operator=(StyleBucketLine &&) = default; + StyleBucketLine(const StyleBucketLine &) = delete; + StyleBucketLine& operator=(const StyleBucketLine &) = delete; + CapType cap = CapType::Butt; JoinType join = JoinType::Miter; float miter_limit = 2.0f; @@ -32,11 +45,11 @@ public: class StyleBucketSymbol { public: // Make movable only. - inline StyleBucketSymbol() = default; - inline StyleBucketSymbol(StyleBucketSymbol &&) = default; - inline StyleBucketSymbol& operator=(StyleBucketSymbol &&) = default; - inline StyleBucketSymbol(const StyleBucketSymbol &) = delete; - inline StyleBucketSymbol& operator=(const StyleBucketSymbol &) = delete; + StyleBucketSymbol() = default; + StyleBucketSymbol(StyleBucketSymbol &&) = default; + StyleBucketSymbol& operator=(StyleBucketSymbol &&) = default; + StyleBucketSymbol(const StyleBucketSymbol &) = delete; + StyleBucketSymbol& operator=(const StyleBucketSymbol &) = delete; PlacementType placement = PlacementType::Point; float min_distance = 250.0f; @@ -79,6 +92,12 @@ public: class StyleBucketRaster { public: + // Make movable only. + StyleBucketRaster() = default; + StyleBucketRaster(StyleBucketRaster &&) = default; + StyleBucketRaster& operator=(StyleBucketRaster &&) = default; + StyleBucketRaster(const StyleBucketRaster &) = delete; + StyleBucketRaster& operator=(const StyleBucketRaster &) = delete; }; class StyleBucketBackground { @@ -89,7 +108,7 @@ typedef mapbox::util::variant StyleBucketRender; -class StyleBucket { +class StyleBucket : public util::noncopyable { public: typedef util::ptr Ptr; diff --git a/test/headless.cpp b/test/headless.cpp index 7d18ca451..c39e5b74a 100644 --- a/test/headless.cpp +++ b/test/headless.cpp @@ -11,7 +11,7 @@ #include #include -#include +#include #include "./fixtures/fixture_log.hpp" @@ -191,7 +191,7 @@ TEST_P(HeadlessTest, render) { } HeadlessView view(env->display); - CachingHTTPFileSource fileSource(platform::defaultCacheDatabase()); + mbgl::DefaultFileSource fileSource(nullptr); Map map(view, fileSource); map.setClasses(classes); diff --git a/test/test.gyp b/test/test.gyp index 406e0b23a..85958b1f0 100644 --- a/test/test.gyp +++ b/test/test.gyp @@ -8,6 +8,7 @@ 'ldflags': [ '<@(uv_ldflags)', '<@(sqlite3_static_libs)', + '<@(uv_static_libs)', '<@(sqlite3_ldflags)', '<@(curl_ldflags)', '<@(png_ldflags)', @@ -210,7 +211,7 @@ ], 'dependencies': [ '../deps/gtest/gtest.gyp:gtest', - '../mapboxgl.gyp:mbgl-standalone', + '../mapboxgl.gyp:mbgl-core', '../mapboxgl.gyp:mbgl-headless', '<(platform_library)', ],