Created attachment 80539 [details] patch <rdar://problem/8921729> REGRESSION: Membuster accumulates 1GB+ memory due to autorelease pools not being drained. Add observers to push and pop autorelease pools on entry and exit from the main CFRunLoop. This will prevent memory from accumulating.
Comment on attachment 80539 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=80539&action=review Looks good. I have many style nitpicks though, seems worth another quick review round. > Source/WebKit2/Platform/RunLoop.h:33 > #include <wtf/PassOwnPtr.h> PLease fix alphabetic sorting. > Source/WebKit2/Platform/RunLoop.h:143 > + RetainPtr<CFRunLoopObserverRef> m_CurrentRunLoopEnterObserverRef; > + RetainPtr<CFRunLoopObserverRef> m_CurrentRunLoopExitObserverRef; WebKit style is m_currentRunLoopEnter/ExitObserver (lower case first letter, and there is no need for "Ref" suffix. > Source/WebKit2/Platform/mac/RunLoopMac.mm:30 > +#define WRAP_RUN_LOOP_WITH_AUTORELEASE_POOLS_OBSERVER_ORDER 1000000 We prefer consts to macros. > Source/WebKit2/Platform/mac/RunLoopMac.mm:36 > + switch (activity) { > + case kCFRunLoopEntry: We don't indent case statements. > Source/WebKit2/Platform/mac/RunLoopMac.mm:43 > + const NSAutoreleasePool* pool = static_cast<const NSAutoreleasePool*>(CFArrayGetValueAtIndex(autoreleasePoolStack, count-1)); Please also use const_cast to remove the const. There should be a space around "-" in count-1. > Source/WebKit2/Platform/mac/RunLoopMac.mm:45 > + CFArrayRemoveValueAtIndex(autoreleasePoolStack, count-1); There should be a space around "-" in count-1. > Source/WebKit2/Platform/mac/RunLoopMac.mm:118 > +// Set up an Autorelease Pool on the main RunLoop > +// FIXME: remove when <rdar://problem/8929426> is fixed. > +void RunLoop::installAutoreleasePoolsOnCurrentRunLoop() Does this function work on the main, or on current run loop? Please bring comments and function name in sync. > Source/WebKit2/Platform/mac/RunLoopMac.mm:123 > + CFRunLoopObserverRef newRunLoopEnterObserverRef = 0; Please don't use local CF objects without RetainPtr. I think that here, you could just assign to m_currentRunLoopEnterObserver immediately. > Source/WebKit2/Platform/mac/RunLoopMac.mm:125 > + CFRunLoopObserverRef newRunLoopExitObserverRef = 0; Ditto. > Source/WebKit2/Platform/mac/RunLoopMac.mm:131 > + newRunLoopEnterObserverContext.info = CFArrayCreateMutable(kCFAllocatorSystemDefault /* Don't retain the autorelease pool tokens. */,0,0); > + newRunLoopEnterObserverContext.retain = CFRetain; // retain the array > + newRunLoopEnterObserverContext.release = CFRelease; // release the array when the context info goes away. > + newRunLoopEnterObserverContext.copyDescription = CFCopyDescription; Please don't align "=" signs or comments. There should be spaces in ",0,0". > Source/WebKit2/Platform/mac/RunLoopMac.mm:134 > + // Install a repeating observer to fire on exit of the current run loop with an very late ordering like kWrapRunLoopWithAutoreleasePoolObserverOrder. CFRunLoop gives lowest priority to those observers close to positive infinity. We don't put two spaces between sentences. > Source/WebKit2/Platform/mac/RunLoopMac.mm:138 > + newRunLoopExitObserverContext.version = 0; > + newRunLoopExitObserverContext.info = newRunLoopEnterObserverContext.info; > + newRunLoopExitObserverContext.retain = CFRetain; // retain the array > + newRunLoopExitObserverContext.release = CFRelease; // release the array when the context info goes away. Please don't align. > Source/WebKit2/Platform/mac/RunLoopMac.mm:142 > + CFRelease((CFMutableArrayRef)newRunLoopEnterObserverContext.info); // owned by the observer. (only one release is needed since Enter/Exit point to the same instance). One space before "//". Please fix grammar in the comment, too. I would have put newRunLoopEnterObserverContext.info into a local RetainPtr variable to avoid the need for explicit CFRelease.
Dave, do you know why the style bot had no comments?
Attachment 80539 [details] did not build on qt: Build output: http://queues.webkit.org/results/7597447
Comment on attachment 80539 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=80539&action=review > Source/WebKit2/Platform/mac/RunLoopMac.mm:61 > + installAutoreleasePoolsOnCurrentRunLoop(); I'm surprised that this is needed or helpful. RunLoop::run claims that -[NSRunLoop run] (and, presumably, +[NSApplication run]) will take care of setting up an autorelease pool. Is that not true?
AppKit creates Autorelease pools at the beginning and end of the user event loop. When testing Membuster, stress test, etc. there are no user events so the top pool isn't getting drained.
Created attachment 80703 [details] new patch
Comment on attachment 80703 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=80703&action=review > AppKit creates Autorelease pools at the beginning and end of the user event loop. When testing Membuster, stress test, etc. there are no user events so the top pool isn't getting drained. Should we switch to CFRunLoop now that this more powerful mechanism is used? > Source/WebKit2/Platform/mac/RunLoopMac.mm:43 > + NSAutoreleasePool* pool = const_cast<NSAutoreleasePool*>(static_cast<const NSAutoreleasePool*>(CFArrayGetValueAtIndex(autoreleasePoolStack, count - 1))); Misplaced star - should be "NSAutoreleasePool *" in all three cases. > Source/WebKit2/Platform/mac/RunLoopMac.mm:45 > + CFArrayRemoveValueAtIndex(autoreleasePoolStack, count-1); There should be spaces around "-". > Source/WebKit2/Platform/mac/RunLoopMac.mm:124 > + RetainPtr<CFMutableArrayRef> autoreleasePoolStack = CFArrayCreateMutable(kCFAllocatorSystemDefault, 0, 0); // Use system Allocator to avoid retaining the autorelease pool tokens; I don't understand this comment. It's the last parameter (null callbacks) that avoids retaining array items, so what's the importance of using kCFAllocatorSystemDefault? Also, it's grammatically incorrect ("Allocator" and a semicolon at the end). > Source/WebKit2/Platform/mac/RunLoopMac.mm:126 > + // Install a repeating observer to fire on entry to the run current run loop with a early ordering like -wrapRunLoopWithAutoreleasePoolsOrder. CFRunLoop gives highest priority to those observers close to minus infinity. Please don't use two spaces between sentences. > Source/WebKit2/Platform/mac/RunLoopMac.mm:144 > + autoreleasePoolStack.clear(); // owned by the observer. No need for this line - it's the same as RetainPtr destruction.
In fact, can't we fix this by simply calling NSApplicationLoad() some time early in WebProcessMain() (before NSApp is initialized)? That way, you can avoid adding custom code.
Comment on attachment 80703 [details] new patch View in context: https://bugs.webkit.org/attachment.cgi?id=80703&action=review > Source/WebKit2/Platform/mac/RunLoopMac.mm:30 > +static const int wrapRunLoopWithAutoreleasePoolsPriority = 1000000; Alexey has pointed out to me many times that we can just use "const int" for such things, because these constants have internal linkage even if you don’t specify static. So now I point it out to you. The type of this constant should be CFIndex, not int. We need a comment here explaining why this is a good value to use for priority. > Source/WebKit2/Platform/mac/RunLoopMac.mm:50 > + default: > + break; Can you leave this out? What good does the default here do? > Source/WebKit2/Platform/mac/RunLoopMac.mm:121 > + if (!m_currentRunLoopEnterObserver) { We prefer early return to nesting the entire function inside an if. So this should just say: if (m_currentRunLoopEnterObserver) { ASSERT(m_currentRunLoopExitObserver); return; } And then the rest of the function should not be nested. >> Source/WebKit2/Platform/mac/RunLoopMac.mm:126 >> + // Install a repeating observer to fire on entry to the run current run loop with a early ordering like -wrapRunLoopWithAutoreleasePoolsOrder. CFRunLoop gives highest priority to those observers close to minus infinity. > > Please don't use two spaces between sentences. It would be clearer to have two priority constants, one for enter and one for exit, rather than using a single constant, sometimes with a minus sign. > Source/WebKit2/Platform/mac/RunLoopMac.mm:148 > +void RunLoop::uninstallAutoreleasePoolsOnCurrentRunLoop(void) No need for the (void) here. That’s a plain C idiom, and never needs to be used in C++. Just () works. > Source/WebKit2/Platform/mac/RunLoopMac.mm:153 > + if (m_currentRunLoopEnterObserver) > + m_currentRunLoopEnterObserver.clear(); > + if (m_currentRunLoopExitObserver) > + m_currentRunLoopExitObserver.clear(); The best style here would be: m_currentRunLoopEnterObserver = nullptr; m_currentRunLoopExitObserver = nullptr; There is no need to check for null before calling clear(), since it does so already, and assignment to nullptr is preferred over calls to clear(). This function is named “uninstall” but the code here simply releases the observers, and does not remove them from the run loop. I expect that won’t work correctly. I believe we need calls to CFRunLoopRemoveObserver here.
(In reply to comment #2) > Dave, do you know why the style bot had no comments? It looks like the style checker doesn't check .mm or .m files. It looks has been true since before r53675 (Jan 2010) -- I stopped trying to backtrace further at that point. To get this to happen, one should modify Tools/Scripts/webkitpy/style/checker.py and perhaps expand _CPP_FILE_EXTENSIONS to have the extensions mm and m (or add a new variable for this). Before such a change is checked in, one should run the style checker on the checked in mm/m files and see how many false positives it gives. For checks that are wrong/gave false positives for Objective C, one could add "file_state.is_objective_c()" checks to disable the check in Tools/Scripts/webkitpy/style/checkers/cpp.py I'm willing to generously advise anyone who would like to take this on.
Created attachment 80883 [details] patch
Comment on attachment 80883 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=80883&action=review > Source/WebKit2/Platform/mac/RunLoopMac.mm:31 > +@interface NSApplication (PeekAtAppKitSecrets) Please rename the category to WebNSApplicationDetails. > Source/WebKit2/Platform/mac/RunLoopMac.mm:45 > RunLoop::RunLoop() > { > + // Installs autorelease pools on the current CFRunLoop which prevents memory from accumulating between user events. > + // FIXME: Remove when <rdar://problem/8929426> is fixed. > + [[NSApplication sharedApplication] _installAutoreleasePoolsOnCurrentThreadIfNecessary]; Do we want this on non-main run loops? What are the other WebKit2 run loops, in fact?
> > + [[NSApplication sharedApplication] _installAutoreleasePoolsOnCurrentThreadIfNecessary]; > > Do we want this on non-main run loops? What are the other WebKit2 run loops, in fact? There's one run loop per thread. So, the question is, do we want this behavior for worker threads, and possibly database and icon database threads, too? I think we probably do, since those threads are even more likely to receive callbacks from other libraries without an explicit user event.
We've intentionally left worker threads without global autorelease pools at all, as that helped catch running main thread only code that got accidentally executed in a worker thread.
(In reply to comment #14) > We've intentionally left worker threads without global autorelease pools at all, as that helped catch running main thread only code that got accidentally executed in a worker thread. Seems like a very strange and roundabout way to detect a threading error. There's no guarantee that temporary autorelease pools won't defeat this roundabout assertion technique. And a "no autorelease pool in place" message is a very unclear way to say "you shouldn't have run this code on a non-main thread." If running main-thread-only code on worker threads has been a problem in the past, a more direct and reliable solution is to add ASSERT(isMainThread()) to common choke points.
I don't know about workers or databases, but Anders said I shouldn't limit this to the main thread because we want it to work when WebKit2 is running in threaded mode.
> Seems like a very strange and roundabout way It's strange and roundabout, but it helped us detect real problems, see bug 40655. Do you have a better suggestion? We shouldn't be adding autorelease pools to threads that should never run any Objective C. But I'm not sure why we are discussing worker, database and icon database threads. These aren't using WebKit2 RunLoop code, as far as I know. What is using it, besides the vaporware threaded mode? Should this class be renamed to not look more general than it is, or should we be calling -_installAutoreleasePoolsOnCurrentThreadIfNecessary in some other code?
> It's strange and roundabout, but it helped us detect real problems, see bug 40655. Do you have a better suggestion? Yes. Put a real ASSERT in a reasonable choke point. For bugs like 40655 (messaging a delegate on a non-main thread), a reasonable choke point is any place in WebKit1 matching the regexp .*Client::dispatch.*. Such an ASSERT already seems to exist for WebKit2: // We only allow sending sync messages from the client run loop. ASSERT(RunLoop::current() == m_clientRunLoop); > We shouldn't be adding autorelease pools to threads that should never run any Objective C. There is no such thing as a thread that should never run any Objective-C. But if you believe there is such a thing, you can try adding an ASSERT to WTF::String::operator NSString*, and/or HardAutorelease. For what it's worth, that ASSERT would have fired in the case of bug 40655. Personally, I think it is irrational to assume that just because an accident helped identify one bug, we should keep committing the same accident. That seems like a recipe for paralysis in software development. > But I'm not sure why we are discussing worker, database and icon database threads. These aren't using WebKit2 RunLoop code, as far as I know. What is using it, besides the vaporware threaded mode? Threaded mode is not vaporware. It exists today, and Anders asked Stephanie to keep it working. > Should this class be renamed to not look more general than it is, or should we be calling -_installAutoreleasePoolsOnCurrentThreadIfNecessary in some other code? If you want to rename RunLoop, please talk to Anders and Sam. Personally, I don't think it's a good idea to name a class after its current set of clients -- then you have to rename it every time it acquires a new client! Anyway, this is pretty far afield from the question at hand, which is, does this patch solve the problem of gigabytes of memory leaking in Safari? I think it does, so I'm inclined to say r+, with the caveat of the WebNSApplicationDetails rename you suggested.
Comment on attachment 80883 [details] patch r=me, but please rename your category to WebNSApplicationDetails like Alexey suggested.
We talked about this more, and the current thinking is that -_installAutoreleasePoolsOnCurrentThreadIfNecessary class should not be added to RunLoop, but to code that calls RunLoop.
Created attachment 81014 [details] patch moving code to WebProcessMain
Comment on attachment 81014 [details] patch moving code to WebProcessMain r=me
Committed http://trac.webkit.org/projects/webkit/changeset/77452